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 packages/react-bindings/src/hooks/useStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const defaultContext: StylesContextValue<{ renderRule: RendererRenderRule }> = {
performance: {
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 @@ -109,12 +142,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 @@ -70,6 +70,7 @@ export type Renderer = Omit<FelaRenderer, 'renderRule'> & {
export interface StylesContextPerformance {
enableStylesCaching: boolean
enableVariablesCaching: boolean
enableHardVariablesCaching: boolean
}

export type StylesContextPerformanceInput = Partial<StylesContextPerformance>
Expand Down
85 changes: 75 additions & 10 deletions packages/react-bindings/test/styles/resolveStyles-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ const resolvedVariables: ComponentVariablesObject = {
const defaultPerformanceOptions: StylesContextPerformance = {
enableStylesCaching: true,
enableVariablesCaching: true,
enableHardVariablesCaching: false,
}

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

return {
theme: {
Expand All @@ -45,7 +45,7 @@ const resolveStylesOptions = (options?: {
renderer: {
renderRule: () => '',
},
performance,
performance: { ...defaultPerformanceOptions, ...performance },
saveDebug: () => {},
}
}
Expand Down Expand Up @@ -158,9 +158,10 @@ describe('resolveStyles', () => {
props: { primary: true },
})
const { resolvedStyles } = resolveStyles(options, resolvedVariables)

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

expect(resolvedStyles.root).toMatchObject({ color: 'red' })
expect(componentStyles.root).toHaveBeenCalledTimes(1)
Expand Down Expand Up @@ -188,7 +189,7 @@ describe('resolveStyles', () => {
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)
Expand All @@ -201,7 +202,7 @@ describe('resolveStyles', () => {
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)
Expand Down Expand Up @@ -231,7 +232,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 @@ -258,7 +259,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 @@ -269,4 +270,68 @@ describe('resolveStyles', () => {

expect(renderStyles).toHaveBeenCalledTimes(propsInlineOverridesSize * 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/utils/mergeProviderContexts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const mergeProviderContexts = (
performance: {
enableStylesCaching: true,
enableVariablesCaching: true,
enableHardVariablesCaching: true,
},
telemetry: undefined,
renderer: undefined,
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/utils/renderComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ const renderComponent = <P extends {}>(
performance: {
...context.performance,
enableStylesCaching: false, // we cannot enable caching for class components
enableHardVariablesCaching: false, // we cannot enable caching for class components
},
})

Expand Down