From 3e6dae2a70916bdd1ffbe000053fc6b48fc33851 Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Tue, 18 Feb 2020 13:54:36 +0100 Subject: [PATCH 01/10] perf: add variables caching --- packages/react-bindings/src/hooks/useStyles.ts | 8 +++++++- packages/react-bindings/src/styles/types.ts | 1 + packages/react/src/utils/renderComponent.tsx | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/react-bindings/src/hooks/useStyles.ts b/packages/react-bindings/src/hooks/useStyles.ts index 3765e22ba1..16d86f29a1 100644 --- a/packages/react-bindings/src/hooks/useStyles.ts +++ b/packages/react-bindings/src/hooks/useStyles.ts @@ -48,6 +48,7 @@ const defaultContext: StylesContextValue<{ renderRule: RendererRenderRule }> = { performance: { enableStylesCaching: true, enableVariablesCaching: true, + enableHardVariablesCaching: false, }, renderer: { renderRule: () => '' }, theme: emptyTheme, @@ -60,7 +61,11 @@ const useStyles = ( const context: StylesContextValue<{ renderRule: RendererRenderRule }> = React.useContext(ThemeContext) || defaultContext - const { enableStylesCaching = true, enableVariablesCaching = true } = context.performance || {} + const { + enableStylesCaching = true, + enableVariablesCaching = true, + enableHardVariablesCaching = false, + } = context.performance || {} const { className = process.env.NODE_ENV === 'production' ? '' : 'no-classname-🙉', @@ -89,6 +94,7 @@ const useStyles = ( performance: { enableStylesCaching, enableVariablesCaching, + enableHardVariablesCaching, }, }) diff --git a/packages/react-bindings/src/styles/types.ts b/packages/react-bindings/src/styles/types.ts index fa4fc1a169..29ed41815f 100644 --- a/packages/react-bindings/src/styles/types.ts +++ b/packages/react-bindings/src/styles/types.ts @@ -70,6 +70,7 @@ export type Renderer = Omit & { export interface StylesContextPerformance { enableStylesCaching?: boolean enableVariablesCaching?: boolean + enableHardVariablesCaching?: boolean } export type StylesContextInputValue = { diff --git a/packages/react/src/utils/renderComponent.tsx b/packages/react/src/utils/renderComponent.tsx index 72198378d7..0795ab7e2b 100644 --- a/packages/react/src/utils/renderComponent.tsx +++ b/packages/react/src/utils/renderComponent.tsx @@ -96,6 +96,7 @@ const renderComponent =

( enableVariablesCaching: typeof enableVariablesCaching === 'boolean' ? enableVariablesCaching : true, enableStylesCaching: false, // we cannot enable caching for class components + enableHardVariablesCaching: false, // we cannot enable caching for class components }, }) From 2d69b4e4fdf2b7bcb3dbd886b29b751fede780fe Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Tue, 11 Feb 2020 12:22:25 +0100 Subject: [PATCH 02/10] chore: refactor perf flags --- .../react-bindings/src/hooks/useStyles.ts | 7 +---- .../src/styles/resolveStyles.ts | 2 +- packages/react-bindings/src/styles/types.ts | 14 ++++++--- .../test/styles/resolveStyles-test.ts | 29 ++++++++++--------- .../src/components/Animation/Animation.tsx | 5 +++- .../src/components/Provider/Provider.tsx | 12 ++++---- packages/react/src/types.ts | 8 +---- packages/react/src/utils/renderComponent.tsx | 4 +-- 8 files changed, 41 insertions(+), 40 deletions(-) diff --git a/packages/react-bindings/src/hooks/useStyles.ts b/packages/react-bindings/src/hooks/useStyles.ts index 3765e22ba1..fe0b9a1e9e 100644 --- a/packages/react-bindings/src/hooks/useStyles.ts +++ b/packages/react-bindings/src/hooks/useStyles.ts @@ -60,8 +60,6 @@ const useStyles = ( const context: StylesContextValue<{ renderRule: RendererRenderRule }> = React.useContext(ThemeContext) || defaultContext - const { enableStylesCaching = true, enableVariablesCaching = true } = context.performance || {} - const { className = process.env.NODE_ENV === 'production' ? '' : 'no-classname-🙉', mapPropsToStyles = () => ({} as StyleProps), @@ -86,10 +84,7 @@ const useStyles = ( rtl, saveDebug: fluentUIDebug => (debug.current = { fluentUIDebug }), theme: context.theme, - performance: { - enableStylesCaching, - enableVariablesCaching, - }, + performance: context.performance, }) return { classes, styles: resolvedStyles } diff --git a/packages/react-bindings/src/styles/resolveStyles.ts b/packages/react-bindings/src/styles/resolveStyles.ts index 8d9dcda323..831e619133 100644 --- a/packages/react-bindings/src/styles/resolveStyles.ts +++ b/packages/react-bindings/src/styles/resolveStyles.ts @@ -51,7 +51,7 @@ const resolveStyles = ( rtl, disableAnimations, renderer, - performance = {}, + performance, } = options || {} const { className, design, styles, variables, ...stylesProps } = props diff --git a/packages/react-bindings/src/styles/types.ts b/packages/react-bindings/src/styles/types.ts index fa4fc1a169..b076473419 100644 --- a/packages/react-bindings/src/styles/types.ts +++ b/packages/react-bindings/src/styles/types.ts @@ -68,18 +68,24 @@ export type Renderer = Omit & { } export interface StylesContextPerformance { - enableStylesCaching?: boolean - enableVariablesCaching?: boolean + enableSanitizeCssPlugin: boolean + enableStylesCaching: boolean + enableVariablesCaching: boolean } +export type StylesContextPerformanceInput = Partial + export type StylesContextInputValue = { disableAnimations?: boolean - performance?: StylesContextPerformance + performance?: StylesContextPerformanceInput renderer?: R theme?: ThemeInput } -export type StylesContextValue = Required> & { +export type StylesContextValue = { + disableAnimations: boolean + performance: StylesContextPerformanceInput + renderer: R theme: ThemePrepared } diff --git a/packages/react-bindings/test/styles/resolveStyles-test.ts b/packages/react-bindings/test/styles/resolveStyles-test.ts index ae8216cef1..5d1c4f4fcf 100644 --- a/packages/react-bindings/test/styles/resolveStyles-test.ts +++ b/packages/react-bindings/test/styles/resolveStyles-test.ts @@ -6,7 +6,7 @@ import { ICSSInJSStyle, } from '@fluentui/styles' import resolveStyles from '../../src/styles/resolveStyles' -import { ResolveStylesOptions } from '../../src/styles/types' +import { ResolveStylesOptions, StylesContextPerformance } from '../../src/styles/types' const componentStyles: ComponentSlotStylesPrepared<{}, { color: string }> = { root: ({ variables: v }): ICSSInJSStyle => ({ @@ -18,19 +18,18 @@ const resolvedVariables: ComponentVariablesObject = { color: 'red', } +const defaultPerformanceOptions: StylesContextPerformance = { + enableStylesCaching: true, + enableVariablesCaching: true, +} + const resolveStylesOptions = (options?: { displayName?: ResolveStylesOptions['displayName'] performance?: ResolveStylesOptions['performance'] props?: ResolveStylesOptions['props'] }): ResolveStylesOptions => { - const { - displayName = 'Test', - performance = { - enableVariablesCaching: true, - enableStylesCaching: true, - }, - props = {}, - } = options || {} + const { displayName = 'Test', performance = defaultPerformanceOptions, props = {} } = + options || {} return { theme: { @@ -188,7 +187,9 @@ describe('resolveStyles', () => { test('does not cache styles if caching is disabled', () => { spyOn(componentStyles, 'root').and.callThrough() - const options = resolveStylesOptions({ performance: { enableStylesCaching: false } }) + const options = resolveStylesOptions({ + performance: { ...defaultPerformanceOptions, enableStylesCaching: false }, + }) const { resolvedStyles } = resolveStyles(options, resolvedVariables) const { resolvedStyles: secondResolvedStyles } = resolveStyles(options, resolvedVariables) @@ -199,7 +200,9 @@ describe('resolveStyles', () => { test('does not cache classes if caching is disabled', () => { const renderStyles = jest.fn().mockReturnValue('a') - const options = resolveStylesOptions({ performance: { enableStylesCaching: false } }) + const options = resolveStylesOptions({ + performance: { ...defaultPerformanceOptions, enableStylesCaching: false }, + }) const { classes } = resolveStyles(options, resolvedVariables, renderStyles) const { classes: secondClasses } = resolveStyles(options, resolvedVariables, renderStyles) @@ -228,7 +231,7 @@ describe('resolveStyles', () => { _.forEach(propsInlineOverrides, (props, idx) => { const options = resolveStylesOptions({ props, - performance: { enableStylesCaching: false }, + performance: { ...defaultPerformanceOptions, enableStylesCaching: false }, }) const { resolvedStyles } = resolveStyles(options, resolvedVariables) @@ -255,7 +258,7 @@ describe('resolveStyles', () => { _.forEach(propsInlineOverrides, props => { const options = resolveStylesOptions({ props, - performance: { enableStylesCaching: false }, + performance: { ...defaultPerformanceOptions, enableStylesCaching: false }, }) const { classes } = resolveStyles(options, resolvedVariables, renderStyles) const { classes: secondClasses } = resolveStyles(options, resolvedVariables, renderStyles) diff --git a/packages/react/src/components/Animation/Animation.tsx b/packages/react/src/components/Animation/Animation.tsx index 51df690685..94a0c56033 100644 --- a/packages/react/src/components/Animation/Animation.tsx +++ b/packages/react/src/components/Animation/Animation.tsx @@ -204,7 +204,10 @@ const Animation: React.FC & { disableAnimations: context.disableAnimations, renderer: context.renderer, rtl: context.rtl, - performance: {}, + performance: { + enableStylesCaching: false, + enableVariablesCaching: false, + }, saveDebug: _.noop, theme: context.theme, }) diff --git a/packages/react/src/components/Provider/Provider.tsx b/packages/react/src/components/Provider/Provider.tsx index d37648b657..eb63487d76 100644 --- a/packages/react/src/components/Provider/Provider.tsx +++ b/packages/react/src/components/Provider/Provider.tsx @@ -4,7 +4,7 @@ import { getElementType, getUnhandledProps, Renderer, - StylesContextPerformance, + StylesContextPerformanceInput, Telemetry, unstable_getStyles, useIsomorphicLayoutEffect, @@ -44,7 +44,7 @@ export interface ProviderProps extends ChildrenComponentProps, UIComponentProps renderer?: Renderer rtl?: boolean disableAnimations?: boolean - performance?: StylesContextPerformance + performance?: StylesContextPerformanceInput overwrite?: boolean target?: Document theme?: ThemeInput @@ -135,10 +135,10 @@ const Provider: React.FC> & { } const consumedContext: ProviderContextPrepared = React.useContext(ThemeContext) - const incomingContext: ProviderContextPrepared = overwrite ? ({} as any) : consumedContext + const incomingContext: ProviderContextInput | ProviderContextPrepared = overwrite + ? {} + : consumedContext - // rehydration disabled to avoid leaking styles between renderers - // https://github.com/rofrischmann/fela/blob/master/docs/api/fela-dom/rehydrate.md const outgoingContext = mergeContexts(incomingContext, inputContext) const rtlProps: { dir?: 'rtl' | 'ltr' } = {} @@ -199,6 +199,8 @@ const Provider: React.FC> & { ...unhandledProps, } + // rehydration disabled to avoid leaking styles between renderers + // https://github.com/rofrischmann/fela/blob/master/docs/api/fela-dom/rehydrate.md return ( ( const { setStart, setEnd } = useTelemetry(displayName, context.telemetry) const rtl = context.rtl || false - const enableVariablesCaching = context.performance?.enableVariablesCaching setStart() @@ -93,8 +92,7 @@ const renderComponent =

( saveDebug, theme: context.theme || emptyTheme, performance: { - enableVariablesCaching: - typeof enableVariablesCaching === 'boolean' ? enableVariablesCaching : true, + ...context.performance, enableStylesCaching: false, // we cannot enable caching for class components }, }) From 11666a84f29213cabb15a8c6a24c8ace3163ad87 Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Tue, 18 Feb 2020 15:45:26 +0100 Subject: [PATCH 03/10] wip! --- .../src/styles/resolveStyles.ts | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/packages/react-bindings/src/styles/resolveStyles.ts b/packages/react-bindings/src/styles/resolveStyles.ts index 8d9dcda323..1d7668b0bc 100644 --- a/packages/react-bindings/src/styles/resolveStyles.ts +++ b/packages/react-bindings/src/styles/resolveStyles.ts @@ -52,12 +52,33 @@ const resolveStyles = ( disableAnimations, renderer, performance = {}, - } = options || {} - + } = options const { className, design, styles, variables, ...stylesProps } = props - const noInlineOverrides = !(design || styles || variables) - const cacheEnabled = performance.enableStylesCaching && noInlineOverrides + if (process.env.NODE_ENV !== 'production') { + if (!performance.enableStylesCaching && performance.enabledHardVariablesCaching) { + throw new Error('111') + } + + if (performance.enabledHardVariablesCaching) { + if (!_.isPlainObject(variables)) { + throw new Error() + } + + if ( + !Object.keys(variables).every(variableName => { + return variables[variableName] // TODO + }) + ) { + } + } + } + + const noInlineStylesOverrides = !(design || styles || variables) + const noVariableOverrides = performance.enabledHardVariablesCaching || !variables + + const cacheEnabled = + performance.enableStylesCaching && noInlineStylesOverrides && noVariableOverrides // Merge theme styles with inline overrides if any let mergedStyles: ComponentSlotStylesPrepared = theme.componentStyles[displayName] || { @@ -109,12 +130,12 @@ const resolveStyles = ( } } - const componentCacheKey = - cacheEnabled && displayName && stylesProps - ? `${displayName}:${JSON.stringify(stylesProps)}${styleParam.rtl}${ - styleParam.disableAnimations - }` - : '' + const propsCacheKey = cacheEnabled + ? `${JSON.stringify(stylesProps)}:${JSON.stringify(variables)}` + : '' + const componentCacheKey = cacheEnabled + ? `${displayName}:${propsCacheKey}${styleParam.rtl}${styleParam.disableAnimations}` + : '' Object.keys(mergedStyles).forEach(slotName => { // resolve/render slot styles once and cache From 67c4dbbda3611666cd56fad68e6e6aff4063a4cd Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Tue, 18 Feb 2020 15:53:11 +0100 Subject: [PATCH 04/10] fix UT --- packages/react-bindings/test/hooks/useStyles-test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-bindings/test/hooks/useStyles-test.tsx b/packages/react-bindings/test/hooks/useStyles-test.tsx index 6eba9f4953..7f4f28f8ca 100644 --- a/packages/react-bindings/test/hooks/useStyles-test.tsx +++ b/packages/react-bindings/test/hooks/useStyles-test.tsx @@ -38,6 +38,7 @@ const TestProvider: React.FC<{ theme: ThemeInput }> = props => { return ( From cef5dc3e935a29e9eaed2235354cebabfd5a51c7 Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Tue, 18 Feb 2020 17:02:40 +0100 Subject: [PATCH 05/10] remove option --- packages/react-bindings/src/styles/types.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react-bindings/src/styles/types.ts b/packages/react-bindings/src/styles/types.ts index b076473419..f152c39946 100644 --- a/packages/react-bindings/src/styles/types.ts +++ b/packages/react-bindings/src/styles/types.ts @@ -68,7 +68,6 @@ export type Renderer = Omit & { } export interface StylesContextPerformance { - enableSanitizeCssPlugin: boolean enableStylesCaching: boolean enableVariablesCaching: boolean } From 1ef81bcbedd0119d6634f5f9e742eb8f65c2de7b Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Wed, 19 Feb 2020 16:49:06 +0100 Subject: [PATCH 06/10] wip --- .../src/styles/resolveStyles.ts | 46 ++++++---- .../test/styles/resolveStyles-test.ts | 85 ++++++++++++++++--- .../src/components/Provider/Provider.tsx | 2 +- .../react/src/utils/mergeProviderContexts.ts | 1 + 4 files changed, 105 insertions(+), 29 deletions(-) diff --git a/packages/react-bindings/src/styles/resolveStyles.ts b/packages/react-bindings/src/styles/resolveStyles.ts index cdc6db7e6a..55167a7c8e 100644 --- a/packages/react-bindings/src/styles/resolveStyles.ts +++ b/packages/react-bindings/src/styles/resolveStyles.ts @@ -56,28 +56,40 @@ const resolveStyles = ( const { className, design, styles, variables, ...stylesProps } = props + const noInlineStylesOverrides = !(design || styles) + const noVariableOverrides = performance.enableHardVariablesCaching || !variables + + /* istanbul ignore else */ if (process.env.NODE_ENV !== 'production') { - if (!performance.enableStylesCaching && performance.enabledHardVariablesCaching) { - throw new Error('111') + 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.enabledHardVariablesCaching) { + if (performance.enableHardVariablesCaching && variables) { if (!_.isPlainObject(variables)) { - throw new Error() + throw new Error( + '@fluentui/react: With "enableHardVariablesCaching" only plain objects can be passed to "variables" prop.', + ) } - if ( - !Object.keys(variables).every(variableName => { - return variables[variableName] // TODO - }) - ) { + 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 noInlineStylesOverrides = !(design || styles || variables) - const noVariableOverrides = performance.enabledHardVariablesCaching || !variables - const cacheEnabled = performance.enableStylesCaching && noInlineStylesOverrides && noVariableOverrides @@ -85,9 +97,8 @@ const resolveStyles = ( 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'), @@ -131,11 +142,10 @@ const resolveStyles = ( } } - const propsCacheKey = cacheEnabled - ? `${JSON.stringify(stylesProps)}:${JSON.stringify(variables)}` - : '' + const propsCacheKey = cacheEnabled ? JSON.stringify(stylesProps) : '' + const variablesCacheKey = performance.enableHardVariablesCaching ? JSON.stringify(variables) : '' const componentCacheKey = cacheEnabled - ? `${displayName}:${propsCacheKey}${styleParam.rtl}${styleParam.disableAnimations}` + ? `${displayName}:${propsCacheKey}:${variablesCacheKey}:${styleParam.rtl}${styleParam.disableAnimations}` : '' Object.keys(mergedStyles).forEach(slotName => { diff --git a/packages/react-bindings/test/styles/resolveStyles-test.ts b/packages/react-bindings/test/styles/resolveStyles-test.ts index 5d1c4f4fcf..a77c9b758f 100644 --- a/packages/react-bindings/test/styles/resolveStyles-test.ts +++ b/packages/react-bindings/test/styles/resolveStyles-test.ts @@ -21,6 +21,7 @@ const resolvedVariables: ComponentVariablesObject = { const defaultPerformanceOptions: StylesContextPerformance = { enableStylesCaching: true, enableVariablesCaching: true, + enableHardVariablesCaching: false, } const resolveStylesOptions = (options?: { @@ -28,8 +29,7 @@ const resolveStylesOptions = (options?: { performance?: ResolveStylesOptions['performance'] props?: ResolveStylesOptions['props'] }): ResolveStylesOptions => { - const { displayName = 'Test', performance = defaultPerformanceOptions, props = {} } = - options || {} + const { displayName = 'Test', performance, props = {} } = options || {} return { theme: { @@ -45,7 +45,7 @@ const resolveStylesOptions = (options?: { renderer: { renderRule: () => '', }, - performance, + performance: { ...defaultPerformanceOptions, ...performance }, saveDebug: () => {}, } } @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) @@ -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/, + ) + }) + }) }) diff --git a/packages/react/src/components/Provider/Provider.tsx b/packages/react/src/components/Provider/Provider.tsx index eb63487d76..7412b0c03f 100644 --- a/packages/react/src/components/Provider/Provider.tsx +++ b/packages/react/src/components/Provider/Provider.tsx @@ -198,7 +198,7 @@ const Provider: React.FC> & { ...rtlProps, ...unhandledProps, } - + console.log(outgoingContext.performance) // rehydration disabled to avoid leaking styles between renderers // https://github.com/rofrischmann/fela/blob/master/docs/api/fela-dom/rehydrate.md return ( diff --git a/packages/react/src/utils/mergeProviderContexts.ts b/packages/react/src/utils/mergeProviderContexts.ts index f3a630d2ba..e428e84f70 100644 --- a/packages/react/src/utils/mergeProviderContexts.ts +++ b/packages/react/src/utils/mergeProviderContexts.ts @@ -64,6 +64,7 @@ const mergeProviderContexts = ( performance: { enableStylesCaching: true, enableVariablesCaching: true, + enableHardVariablesCaching: true, }, telemetry: undefined, renderer: undefined, From b7c14a6032e2ea2f0935d486100cf9bdd85e4be7 Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Mon, 24 Feb 2020 10:31:09 +0100 Subject: [PATCH 07/10] fix variable --- packages/react/src/components/Animation/Animation.tsx | 1 + 1 file changed, 1 insertion(+) 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, From 5684add2ca518a26617900bd2eac5a7b8457fe0e Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Mon, 24 Feb 2020 12:28:21 +0100 Subject: [PATCH 08/10] add styles to RTL, fix default settings --- .../test/styles/resolveStyles-test.ts | 60 +++++++++++++------ .../react/src/utils/mergeProviderContexts.ts | 2 +- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/packages/react-bindings/test/styles/resolveStyles-test.ts b/packages/react-bindings/test/styles/resolveStyles-test.ts index af6534f42e..d6910de1da 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()}"`, }), } @@ -29,8 +30,9 @@ const resolveStylesOptions = (options?: { displayName?: ResolveStylesOptions['displayName'] performance?: ResolveStylesOptions['performance'] props?: ResolveStylesOptions['props'] + rtl?: ResolveStylesOptions['rtl'] }): ResolveStylesOptions => { - const { displayName = 'Test', performance, props = {} } = options || {} + const { displayName = 'Test', performance, props = {}, rtl = false } = options || {} return { theme: { @@ -41,7 +43,7 @@ const resolveStylesOptions = (options?: { }, displayName, props, - rtl: false, + rtl, disableAnimations: false, renderer: { renderRule: () => '', @@ -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) }) @@ -164,9 +166,9 @@ describe('resolveStyles', () => { 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) }) @@ -182,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) }) @@ -195,8 +197,8 @@ describe('resolveStyles', () => { 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) }) @@ -209,7 +211,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) }) @@ -272,6 +274,30 @@ 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') diff --git a/packages/react/src/utils/mergeProviderContexts.ts b/packages/react/src/utils/mergeProviderContexts.ts index 6aa3117c4a..56a8148eeb 100644 --- a/packages/react/src/utils/mergeProviderContexts.ts +++ b/packages/react/src/utils/mergeProviderContexts.ts @@ -72,7 +72,7 @@ const mergeProviderContexts = ( enableSanitizeCssPlugin: process.env.NODE_ENV !== 'production', enableStylesCaching: true, enableVariablesCaching: true, - enableHardVariablesCaching: true, + enableHardVariablesCaching: false, }, telemetry: undefined, renderer: undefined, From 5444d76e59d95a450e1d89ddb4fff9c04968e5bf Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Mon, 24 Feb 2020 12:34:04 +0100 Subject: [PATCH 09/10] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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) From 7c35b398f1117fa5a7f234edaaf14b88a8ab199a Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Mon, 24 Feb 2020 12:47:22 +0100 Subject: [PATCH 10/10] fix types in tests --- packages/react-bindings/test/styles/resolveStyles-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-bindings/test/styles/resolveStyles-test.ts b/packages/react-bindings/test/styles/resolveStyles-test.ts index d6910de1da..d7a7ac95de 100644 --- a/packages/react-bindings/test/styles/resolveStyles-test.ts +++ b/packages/react-bindings/test/styles/resolveStyles-test.ts @@ -28,7 +28,7 @@ const defaultPerformanceOptions: StylesContextPerformance = { const resolveStylesOptions = (options?: { displayName?: ResolveStylesOptions['displayName'] - performance?: ResolveStylesOptions['performance'] + performance?: Partial props?: ResolveStylesOptions['props'] rtl?: ResolveStylesOptions['rtl'] }): ResolveStylesOptions => {