diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a92f142f8..938167245d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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](https://github.com/microsoft/fluent-ui-react/tree/v0.44.0) (2020-02-05) diff --git a/packages/react-bindings/src/hooks/useStyles.ts b/packages/react-bindings/src/hooks/useStyles.ts index 0a53d37592..91ba729aed 100644 --- a/packages/react-bindings/src/hooks/useStyles.ts +++ b/packages/react-bindings/src/hooks/useStyles.ts @@ -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, diff --git a/packages/react-bindings/src/styles/resolveStyles.ts b/packages/react-bindings/src/styles/resolveStyles.ts index 8f93c602f2..caf1f31c5b 100644 --- a/packages/react-bindings/src/styles/resolveStyles.ts +++ b/packages/react-bindings/src/styles/resolveStyles.ts @@ -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) { + 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'), @@ -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 diff --git a/packages/react-bindings/src/styles/types.ts b/packages/react-bindings/src/styles/types.ts index 867aa8a2cc..19c6189b5f 100644 --- a/packages/react-bindings/src/styles/types.ts +++ b/packages/react-bindings/src/styles/types.ts @@ -72,6 +72,7 @@ export interface StylesContextPerformance { enableSanitizeCssPlugin: boolean enableStylesCaching: boolean enableVariablesCaching: boolean + enableHardVariablesCaching: boolean } export type StylesContextPerformanceInput = Partial diff --git a/packages/react-bindings/test/styles/resolveStyles-test.ts b/packages/react-bindings/test/styles/resolveStyles-test.ts index 463a745ffc..d7a7ac95de 100644 --- a/packages/react-bindings/test/styles/resolveStyles-test.ts +++ b/packages/react-bindings/test/styles/resolveStyles-test.ts @@ -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()}"`, }), } @@ -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 props?: ResolveStylesOptions['props'] + rtl?: ResolveStylesOptions['rtl'] }): ResolveStylesOptions => { - const { displayName = 'Test', performance = defaultPerformanceOptions, props = {} } = - options || {} + const { displayName = 'Test', performance, props = {}, rtl = false } = options || {} return { theme: { @@ -41,12 +43,12 @@ const resolveStylesOptions = (options?: { }, displayName, props, - rtl: false, + rtl, disableAnimations: false, renderer: { renderRule: () => '', }, - performance, + performance: { ...defaultPerformanceOptions, ...performance }, saveDebug: () => {}, } } @@ -85,7 +87,7 @@ 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', () => { @@ -93,7 +95,7 @@ describe('resolveStyles', () => { 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) }) @@ -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) }) @@ -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) }) @@ -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) }) @@ -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) }) @@ -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) }) @@ -181,7 +184,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(2) }) @@ -189,26 +192,26 @@ 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) - 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) }) @@ -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) @@ -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) @@ -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/, + ) + }) + }) }) diff --git a/packages/react/src/components/Animation/Animation.tsx b/packages/react/src/components/Animation/Animation.tsx index 6155ef0917..29ced800fd 100644 --- a/packages/react/src/components/Animation/Animation.tsx +++ b/packages/react/src/components/Animation/Animation.tsx @@ -209,6 +209,7 @@ const Animation: React.FC & { enableSanitizeCssPlugin: false, enableStylesCaching: false, enableVariablesCaching: false, + enableHardVariablesCaching: false, }, saveDebug: _.noop, theme: context.theme, diff --git a/packages/react/src/utils/mergeProviderContexts.ts b/packages/react/src/utils/mergeProviderContexts.ts index dd4ff872a0..56a8148eeb 100644 --- a/packages/react/src/utils/mergeProviderContexts.ts +++ b/packages/react/src/utils/mergeProviderContexts.ts @@ -72,6 +72,7 @@ const mergeProviderContexts = ( enableSanitizeCssPlugin: process.env.NODE_ENV !== 'production', enableStylesCaching: true, enableVariablesCaching: true, + enableHardVariablesCaching: false, }, telemetry: undefined, renderer: undefined, diff --git a/packages/react/src/utils/renderComponent.tsx b/packages/react/src/utils/renderComponent.tsx index c82ebd700f..d0e0739168 100644 --- a/packages/react/src/utils/renderComponent.tsx +++ b/packages/react/src/utils/renderComponent.tsx @@ -94,6 +94,7 @@ const renderComponent =

( performance: { ...context.performance, enableStylesCaching: false, // we cannot enable caching for class components + enableHardVariablesCaching: false, // we cannot enable caching for class components }, })