Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

perf: add variables caching #2373

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Performance
- Add styles caching when there aren't inline overrides defined @mnajdova ([#2309](https://github.com/microsoft/fluent-ui-react/pull/2309))
- Styles for `Animation` component are computed again only on prop changes @layershifter ([#2258](https://github.com/microsoft/fluent-ui-react/pull/2258))
- Add `enableHardVariablesCaching` to have styles caching for primitive `variables` overrides @layershifter ([#2373](https://github.com/microsoft/fluent-ui-react/pull/2373))

<!--------------------------------[ v0.44.0 ]------------------------------- -->
## [v0.44.0](https://github.com/microsoft/fluent-ui-react/tree/v0.44.0) (2020-02-05)
Expand Down
1 change: 1 addition & 0 deletions packages/react-bindings/src/hooks/useStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const defaultContext: StylesContextValue<{ renderRule: RendererRenderRule }> = {
enableSanitizeCssPlugin: process.env.NODE_ENV !== 'production',
enableStylesCaching: true,
enableVariablesCaching: true,
enableHardVariablesCaching: false,
},
renderer: { renderRule: () => '' },
theme: emptyTheme,
Expand Down
54 changes: 43 additions & 11 deletions packages/react-bindings/src/styles/resolveStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,53 @@ const resolveStyles = (
disableAnimations,
renderer,
performance,
} = options || {}
} = options

const { className, design, styles, variables, ...stylesProps } = props
const noInlineOverrides = !(design || styles || variables)

const cacheEnabled = performance.enableStylesCaching && noInlineOverrides
const noInlineStylesOverrides = !(design || styles)
const noVariableOverrides = performance.enableHardVariablesCaching || !variables

/* istanbul ignore else */
if (process.env.NODE_ENV !== 'production') {
if (!performance.enableStylesCaching && performance.enableHardVariablesCaching) {
throw new Error(
'@fluentui/react: Please check your "performance" settings on "Provider", to enable "enableHardVariablesCaching" you need to enable "enableStylesCaching"',
)
}

if (performance.enableHardVariablesCaching && variables) {
if (!_.isPlainObject(variables)) {
throw new Error(
'@fluentui/react: With "enableHardVariablesCaching" only plain objects can be passed to "variables" prop.',
)
}

const hasOnlyBooleanVariables = Object.keys(variables).every(variableName => {
return (
variables[variableName] === null ||
typeof variables[variableName] === 'boolean' ||
typeof variables[variableName] === 'undefined'
)
})

if (!hasOnlyBooleanVariables) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just not cache for this instances of components which donot have only boolean variables instead of throwing an error? We are not throwing if caching is enabled and there are inline overrides, I don't see how this is different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main goal to throw:

  • easy detect broken cases
  • avoid having these conditions in production as here we are iterating via object keys for example

throw new Error(
'@fluentui/react: With "enableHardVariablesCaching" only boolean or nil properties can bepassed to "variables" prop.',
)
}
}
}

const cacheEnabled =
performance.enableStylesCaching && noInlineStylesOverrides && noVariableOverrides

// Merge theme styles with inline overrides if any
let mergedStyles: ComponentSlotStylesPrepared = theme.componentStyles[displayName] || {
root: () => ({}),
}
const hasInlineStylesOverrides = !_.isNil(props.design) || !_.isNil(props.styles)

if (hasInlineStylesOverrides) {
if (!noInlineStylesOverrides) {
mergedStyles = mergeComponentStyles(
mergedStyles,
props.design && withDebugId({ root: props.design }, 'props.design'),
Expand Down Expand Up @@ -110,12 +143,11 @@ const resolveStyles = (
}
}

const componentCacheKey =
cacheEnabled && displayName && stylesProps
? `${displayName}:${JSON.stringify(stylesProps)}${styleParam.rtl}${
styleParam.disableAnimations
}`
: ''
const propsCacheKey = cacheEnabled ? JSON.stringify(stylesProps) : ''
const variablesCacheKey = performance.enableHardVariablesCaching ? JSON.stringify(variables) : ''
const componentCacheKey = cacheEnabled
? `${displayName}:${propsCacheKey}:${variablesCacheKey}:${styleParam.rtl}${styleParam.disableAnimations}`
: ''

Object.keys(mergedStyles).forEach(slotName => {
// resolve/render slot styles once and cache
Expand Down
1 change: 1 addition & 0 deletions packages/react-bindings/src/styles/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export interface StylesContextPerformance {
enableSanitizeCssPlugin: boolean
enableStylesCaching: boolean
enableVariablesCaching: boolean
enableHardVariablesCaching: boolean
}

export type StylesContextPerformanceInput = Partial<StylesContextPerformance>
Expand Down
145 changes: 118 additions & 27 deletions packages/react-bindings/test/styles/resolveStyles-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ import resolveStyles from '../../src/styles/resolveStyles'
import { ResolveStylesOptions, StylesContextPerformance } from '../../src/styles/types'

const componentStyles: ComponentSlotStylesPrepared<{}, { color: string }> = {
root: ({ variables: v }): ICSSInJSStyle => ({
root: ({ variables: v, rtl }): ICSSInJSStyle => ({
color: v.color,
content: `"rtl:${rtl.toString()}"`,
}),
}

Expand All @@ -22,15 +23,16 @@ const defaultPerformanceOptions: StylesContextPerformance = {
enableSanitizeCssPlugin: true,
enableStylesCaching: true,
enableVariablesCaching: true,
enableHardVariablesCaching: false,
}

const resolveStylesOptions = (options?: {
displayName?: ResolveStylesOptions['displayName']
performance?: ResolveStylesOptions['performance']
performance?: Partial<ResolveStylesOptions['performance']>
props?: ResolveStylesOptions['props']
rtl?: ResolveStylesOptions['rtl']
}): ResolveStylesOptions => {
const { displayName = 'Test', performance = defaultPerformanceOptions, props = {} } =
options || {}
const { displayName = 'Test', performance, props = {}, rtl = false } = options || {}

return {
theme: {
Expand All @@ -41,12 +43,12 @@ const resolveStylesOptions = (options?: {
},
displayName,
props,
rtl: false,
rtl,
disableAnimations: false,
renderer: {
renderRule: () => '',
},
performance,
performance: { ...defaultPerformanceOptions, ...performance },
saveDebug: () => {},
}
}
Expand Down Expand Up @@ -85,15 +87,15 @@ describe('resolveStyles', () => {
const { classes } = resolveStyles(resolveStylesOptions(), resolvedVariables, renderStyles)

expect(classes['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledWith({ color: 'red' })
expect(renderStyles).toHaveBeenCalledWith(expect.objectContaining({ color: 'red' }))
})

test('caches rendered classes', () => {
const renderStyles = jest.fn().mockReturnValue('a')
const { classes } = resolveStyles(resolveStylesOptions(), resolvedVariables, renderStyles)

expect(classes['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledWith({ color: 'red' })
expect(renderStyles).toHaveBeenCalledWith(expect.objectContaining({ color: 'red' }))
expect(classes['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledTimes(1)
})
Expand All @@ -104,9 +106,9 @@ describe('resolveStyles', () => {
const { resolvedStyles } = resolveStyles(options, resolvedVariables)
const { resolvedStyles: secondResolvedStyles } = resolveStyles(options, resolvedVariables)

expect(resolvedStyles.root).toMatchObject({ color: 'red' })
expect(resolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(componentStyles.root).toHaveBeenCalledTimes(1)
expect(secondResolvedStyles.root).toMatchObject({ color: 'red' })
expect(secondResolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(componentStyles.root).toHaveBeenCalledTimes(1)
})

Expand All @@ -117,7 +119,7 @@ describe('resolveStyles', () => {
const { classes: secondClasses } = resolveStyles(options, resolvedVariables, renderStyles)

expect(classes['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledWith({ color: 'red' })
expect(renderStyles).toHaveBeenCalledWith(expect.objectContaining({ color: 'red' }))
expect(secondClasses['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledTimes(1)
})
Expand All @@ -131,9 +133,9 @@ describe('resolveStyles', () => {
const { resolvedStyles } = resolveStyles(options, resolvedVariables)
const { resolvedStyles: secondResolvedStyles } = resolveStyles(options, resolvedVariables)

expect(resolvedStyles.root).toMatchObject({ color: 'red' })
expect(resolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(componentStyles.root).toHaveBeenCalledTimes(1)
expect(secondResolvedStyles.root).toMatchObject({ color: 'red' })
expect(secondResolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(componentStyles.root).toHaveBeenCalledTimes(1)
})

Expand All @@ -147,7 +149,7 @@ describe('resolveStyles', () => {
const { classes: secondClasses } = resolveStyles(options, resolvedVariables, renderStyles)

expect(classes['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledWith({ color: 'red' })
expect(renderStyles).toHaveBeenCalledWith(expect.objectContaining({ color: 'red' }))
expect(secondClasses['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledTimes(1)
})
Expand All @@ -159,13 +161,14 @@ describe('resolveStyles', () => {
props: { primary: true },
})
const { resolvedStyles } = resolveStyles(options, resolvedVariables)
const { resolvedStyles: secondResolvedStyles } = resolveStyles(
{ ...options, props: { primary: false } },
resolvedVariables,
)

options.props = { primary: false }
const { resolvedStyles: secondResolvedStyles } = resolveStyles(options, resolvedVariables)

expect(resolvedStyles.root).toMatchObject({ color: 'red' })
expect(resolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(componentStyles.root).toHaveBeenCalledTimes(1)
expect(secondResolvedStyles.root).toMatchObject({ color: 'red' })
expect(secondResolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(componentStyles.root).toHaveBeenCalledTimes(2)
})

Expand All @@ -181,34 +184,34 @@ describe('resolveStyles', () => {
const { classes: secondClasses } = resolveStyles(options, resolvedVariables, renderStyles)

expect(classes['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledWith({ color: 'red' })
expect(renderStyles).toHaveBeenCalledWith(expect.objectContaining({ color: 'red' }))
expect(secondClasses['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledTimes(2)
})

test('does not cache styles if caching is disabled', () => {
spyOn(componentStyles, 'root').and.callThrough()
const options = resolveStylesOptions({
performance: { ...defaultPerformanceOptions, enableStylesCaching: false },
performance: { enableStylesCaching: false },
})
const { resolvedStyles } = resolveStyles(options, resolvedVariables)
const { resolvedStyles: secondResolvedStyles } = resolveStyles(options, resolvedVariables)

expect(resolvedStyles.root).toMatchObject({ color: 'red' })
expect(secondResolvedStyles.root).toMatchObject({ color: 'red' })
expect(resolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(secondResolvedStyles.root).toMatchObject(expect.objectContaining({ color: 'red' }))
expect(componentStyles.root).toHaveBeenCalledTimes(2)
})

test('does not cache classes if caching is disabled', () => {
const renderStyles = jest.fn().mockReturnValue('a')
const options = resolveStylesOptions({
performance: { ...defaultPerformanceOptions, enableStylesCaching: false },
performance: { enableStylesCaching: false },
})
const { classes } = resolveStyles(options, resolvedVariables, renderStyles)
const { classes: secondClasses } = resolveStyles(options, resolvedVariables, renderStyles)

expect(classes['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledWith({ color: 'red' })
expect(renderStyles).toHaveBeenCalledWith(expect.objectContaining({ color: 'red' }))
expect(secondClasses['root']).toBeDefined()
expect(renderStyles).toHaveBeenCalledTimes(2)
})
Expand All @@ -232,7 +235,7 @@ describe('resolveStyles', () => {
_.forEach(propsInlineOverrides, (props, idx) => {
const options = resolveStylesOptions({
props,
performance: { ...defaultPerformanceOptions, enableStylesCaching: false },
performance: { enableStylesCaching: false },
})

const { resolvedStyles } = resolveStyles(options, resolvedVariables)
Expand All @@ -259,7 +262,7 @@ describe('resolveStyles', () => {
_.forEach(propsInlineOverrides, props => {
const options = resolveStylesOptions({
props,
performance: { ...defaultPerformanceOptions, enableStylesCaching: false },
performance: { enableStylesCaching: false },
})
const { classes } = resolveStyles(options, resolvedVariables, renderStyles)
const { classes: secondClasses } = resolveStyles(options, resolvedVariables, renderStyles)
Expand All @@ -270,4 +273,92 @@ describe('resolveStyles', () => {

expect(renderStyles).toHaveBeenCalledTimes(propsInlineOverridesSize * 2)
})

test('computes new styles when "rtl" changes', () => {
const renderStyles = jest.fn().mockImplementation((style: ICSSInJSStyle) => style.content)

const ltrOptions = resolveStylesOptions({ rtl: false })
const rtlOptions = resolveStylesOptions({ rtl: true })

const ltrStyles = resolveStyles(ltrOptions, resolvedVariables, renderStyles)
const rtlStyles = resolveStyles(rtlOptions, resolvedVariables, renderStyles)

expect(ltrStyles).toHaveProperty(
'resolvedStyles.root.content',
expect.stringMatching(/rtl:false/),
)
expect(ltrStyles).toHaveProperty('classes.root', expect.stringMatching(/rtl:false/))
expect(renderStyles).toHaveBeenCalledTimes(1)

expect(rtlStyles).toHaveProperty(
'resolvedStyles.root.content',
expect.stringMatching(/rtl:true/),
)
expect(rtlStyles).toHaveProperty('classes.root', expect.stringMatching(/rtl:true/))
expect(renderStyles).toHaveBeenCalledTimes(2)
})

describe('enableHardVariablesCaching', () => {
test('avoids "classes" computation when enabled', () => {
const renderStyles = jest.fn().mockReturnValue('a')
const options = resolveStylesOptions({
props: { variables: { isFoo: true, isBar: null, isBaz: undefined } },
performance: { enableHardVariablesCaching: true },
})

expect(resolveStyles(options, resolvedVariables, renderStyles)).toHaveProperty(
'classes.root',
'a',
)
expect(resolveStyles(options, resolvedVariables, renderStyles)).toHaveProperty(
'classes.root',
'a',
)
expect(renderStyles).toHaveBeenCalledTimes(1)
})

test('avoids "styles" computation when enabled', () => {
spyOn(componentStyles, 'root').and.callThrough()
const options = resolveStylesOptions({
props: { variables: { isFoo: true, isBar: null, isBaz: undefined } },
performance: { enableHardVariablesCaching: true },
})

expect(resolveStyles(options, resolvedVariables)).toHaveProperty('resolvedStyles.root')
expect(resolveStyles(options, resolvedVariables)).toHaveProperty('resolvedStyles.root')
expect(componentStyles.root).toHaveBeenCalledTimes(1)
})

test('requires "enableStylesCaching" to be enabled', () => {
const options = resolveStylesOptions({
performance: { enableStylesCaching: false, enableHardVariablesCaching: true },
})

expect(() => resolveStyles(options, resolvedVariables)).toThrowError(
/Please check your "performance" settings on "Provider"/,
)
})

test('when enabled only plain objects can be passed as "variables"', () => {
const options = resolveStylesOptions({
props: { variables: () => {} },
performance: { enableHardVariablesCaching: true },
})

expect(() => resolveStyles(options, resolvedVariables)).toThrowError(
/With "enableHardVariablesCaching" only plain objects/,
)
})

test('when enabled only boolean or nil properties can be passed to "variables"', () => {
const options = resolveStylesOptions({
props: { variables: { foo: 'bar' } },
performance: { enableHardVariablesCaching: true },
})

expect(() => resolveStyles(options, resolvedVariables)).toThrowError(
/With "enableHardVariablesCaching" only boolean or nil properties/,
)
})
})
})
1 change: 1 addition & 0 deletions packages/react/src/components/Animation/Animation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ const Animation: React.FC<AnimationProps> & {
enableSanitizeCssPlugin: false,
enableStylesCaching: false,
enableVariablesCaching: false,
enableHardVariablesCaching: false,
},
saveDebug: _.noop,
theme: context.theme,
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/utils/mergeProviderContexts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const mergeProviderContexts = (
enableSanitizeCssPlugin: process.env.NODE_ENV !== 'production',
enableStylesCaching: true,
enableVariablesCaching: true,
enableHardVariablesCaching: false,
},
telemetry: undefined,
renderer: undefined,
Expand Down
Loading