chore: rework mergeComponentStyles to avoid additional merging (#15381)

* chore: rework mergeComponentStyles to avoid additional merging * fix styles merge * add changelog entry * remove a test * Update packages/fluentui/styles/src/mergeThemes.ts * Update packages/fluentui/styles/src/mergeThemes.ts * Update packages/fluentui/styles/src/mergeThemes.ts * Update packages/fluentui/styles/src/mergeThemes.ts * fix naming
microsoft · Jan 5, 2021 · d0d59e6 · d0d59e6
1 parent 6ecc063
commit d0d59e6
Show file tree

Hide file tree

Showing 6 changed files with 125 additions and 134 deletions.
diff --git a/packages/fluentui/CHANGELOG.md b/packages/fluentui/CHANGELOG.md
@@ -36,6 +36,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Fix `treeAsListBehavior` to support multi-select `Tree` @yuanboxue-amber ([#15147](https://github.com/microsoft/fluentui/pull/15147))
 - Fix `Tree` to have prop `onFocusParent` triggered on `ArrowLeft` for leaf node @yuanboxue-amber ([#15442](https://github.com/microsoft/fluentui/pull/15442))
 - Use `aria-checked` for multi-select tree instead of `aria-selected` @yuanboxue-amber ([#15142](https://github.com/microsoft/fluentui/pull/15142))
+- Rework `mergeComponentStyles()` to avoid additional merging @layershifter ([#15381](https://github.com/microsoft/fluentui/pull/15381))
 - Fix `Breadcrumb` to have the `div` wrapping all items obtain a11y attributes from `container` slot @yuanboxue-amber ([#15303](https://github.com/microsoft/fluentui/pull/15303))
 - Fix scrollbar color to have higher contrast ratio @yuanboxue-amber ([#15209](https://github.com/microsoft/fluentui/pull/15209))
 - Fix `Tree` to have un-selectable `treeItem` with `selectable` prop false @yuanboxue-amber ([#15170](https://github.com/microsoft/fluentui/pull/15170))

diff --git a/packages/fluentui/docs/src/examples/components/Toolbar/Performance/CustomToolbar.perf.tsx b/packages/fluentui/docs/src/examples/components/Toolbar/Performance/CustomToolbar.perf.tsx
@@ -499,8 +499,7 @@ const CustomToolbar: React.FunctionComponent<CustomToolbarProps> = props => {
 };
 
 const CustomToolbarPrototype = () => {
- let theme = {};
- theme = mergeThemes(teamsDarkTheme, darkThemeOverrides);
+ const theme = mergeThemes(teamsDarkTheme, darkThemeOverrides);
 
  return (
  <Provider theme={theme}>

diff --git a/packages/fluentui/react-bindings/src/styles/resolveStyles.ts b/packages/fluentui/react-bindings/src/styles/resolveStyles.ts
@@ -100,7 +100,9 @@ export const resolveStyles = (
  const styles = allDisplayNames.map(displayName => theme.componentStyles[displayName]).filter(Boolean);
 
  if (styles.length > 0) {
- mergedStyles = mergeComponentStyles(...styles);
+ mergedStyles = styles.reduce<ComponentSlotStylesPrepared>((acc, styles) => {
+ return mergeComponentStyles(acc, styles);
+ }, {});
  } else {
  mergedStyles = { root: () => ({}) };
  }
@@ -109,8 +111,10 @@ export const resolveStyles = (
  if (!noInlineStylesOverrides) {
  mergedStyles = mergeComponentStyles(
  mergedStyles,
- design && withDebugId({ root: design }, 'props.design'),
- styles && withDebugId({ root: styles } as ComponentSlotStylesInput, 'props.styles'),
+ mergeComponentStyles(
+ design && withDebugId({ root: design }, 'props.design'),
+ styles && withDebugId({ root: styles } as ComponentSlotStylesInput, 'props.styles'),
+ ),
  );
  }
 

diff --git a/packages/fluentui/styles/src/mergeThemes.ts b/packages/fluentui/styles/src/mergeThemes.ts
@@ -45,105 +45,122 @@ export const emptyTheme: ThemePrepared = {
  * Merges a single component's styles (keyed by component part) with another component's styles.
  */
 export const mergeComponentStyles__PROD = (
- ...sources: (ComponentSlotStylesInput | null | undefined)[]
+ stylesA: ComponentSlotStyle | null | undefined,
+ stylesB: ComponentSlotStyle | null | undefined,
 ): ComponentSlotStylesPrepared => {
- const initial: ComponentSlotStylesPrepared = {};
-
- return sources.reduce<ComponentSlotStylesPrepared>((partStylesPrepared, stylesByPart) => {
- // The of "[].forEach()" instead of "_.forEach" has zero sense, but somehow it solves a reported memory leak.
- // There is no 100% confidence that it actually fixes anything.
- if (_.isPlainObject(stylesByPart)) {
- Object.keys(stylesByPart).forEach(partName => {
- const partStyle = stylesByPart[partName];
-
- // Break references to avoid an infinite loop.
- // We are replacing functions with a new ones that calls the originals.
- const originalTarget = partStylesPrepared[partName];
- const originalSource = partStyle;
-
- // if there is no source, merging is a no-op, skip it
- if (
- typeof originalSource === 'undefined' ||
- originalSource === null ||
- (typeof originalSource === 'object' && Object.keys(originalSource).length === 0)
- ) {
- return;
- }
-
- // no target means source doesn't need to merge onto anything
- // just ensure source is callable (prepared format)
- if (typeof originalTarget === 'undefined') {
- partStylesPrepared[partName] = callable(originalSource);
- return;
- }
-
- // We have both target and source, replace with merge fn
- partStylesPrepared[partName] = styleParam => {
- // originalTarget is always prepared, fn is guaranteed
- return _.merge(originalTarget(styleParam), callable(originalSource)(styleParam));
- };
- });
- }
+ const result = {};
 
- return partStylesPrepared;
- }, initial);
-};
+ if (stylesA) {
+ Object.keys(stylesA).forEach(partName => {
+ const slotA = stylesA[partName];
+ const slotB = stylesB?.[partName];
 
-export const mergeComponentStyles__DEV = (
- ...sources: (ComponentSlotStylesInput | null | undefined)[]
-): ComponentSlotStylesPrepared => {
- if (!isDebugEnabled) {
- return mergeComponentStyles__PROD(...sources);
+ // if there is no source, merging is a no-op, skip it
+ if (typeof slotA === 'undefined' || slotA === null) {
+ return;
+ }
+
+ // no target means source doesn't need to merge onto anything
+ // just ensure source is callable (prepared format)
+ if (typeof slotB === 'undefined' || slotB === null) {
+ result[partName] = typeof slotA === 'function' ? slotA : () => slotA;
+ return;
+ }
+
+ if (slotA === slotB) {
+ result[partName] = typeof slotA === 'function' ? slotA : () => slotA;
+ }
+ });
  }
- const initial: ComponentSlotStylesPrepared = {};
 
- return sources.reduce<ComponentSlotStylesPrepared>((partStylesPrepared, stylesByPart) => {
- _.forEach(stylesByPart, (partStyle, partName) => {
- // Break references to avoid an infinite loop.
- // We are replacing functions with a new ones that calls the originals.
- const originalTarget = partStylesPrepared[partName];
- const originalSource = partStyle;
+ if (stylesB) {
+ Object.keys(stylesB).forEach(partName => {
+ const slotA = stylesA?.[partName];
+ const slotB = stylesB[partName];
 
  // if there is no source, merging is a no-op, skip it
- if (
- typeof originalSource === 'undefined' ||
- originalSource === null ||
- (typeof originalSource === 'object' && Object.keys(originalSource).length === 0)
- ) {
+ if (typeof slotB === 'undefined' || slotB === null) {
  return;
  }
 
  // no target means source doesn't need to merge onto anything
- // just ensure source is callable (prepared format) and has _debug
- if (typeof originalTarget === 'undefined') {
- partStylesPrepared[partName] = styleParam => {
- // originalTarget is always prepared, fn is guaranteed, _debug always exists
- const { _debug = undefined, ...styles } = callable(originalSource)(styleParam) || {};
-
- // new object required to prevent circular JSON structure error in <Debug />
- return {
- ...styles,
- _debug: _debug || [{ styles: { ...styles }, debugId: stylesByPart._debugId }],
- };
- };
+ // just ensure source is callable (prepared format)
+ if (typeof slotA === 'undefined' || slotA === null) {
+ result[partName] = typeof slotB === 'function' ? slotB : () => slotB;
+ return;
+ }
 
+ if (slotA === slotB) {
  return;
  }
 
  // We have both target and source, replace with merge fn
- partStylesPrepared[partName] = styleParam => {
- // originalTarget is always prepared, fn is guaranteed, _debug always exists
- const { _debug: targetDebug, ...targetStyles } = originalTarget(styleParam);
- const { _debug: sourceDebug = undefined, ...sourceStyles } = callable(originalSource)(styleParam) || {};
+ result[partName] = function mergedStyleFunction(styleParam) {
+ // originalTarget is always prepared, fn is guaranteed
+ return _.merge(
+ typeof slotA === 'function' ? slotA(styleParam) : slotA,
+ typeof slotB === 'function' ? slotB(styleParam) : slotB,
+ );
+ };
+ });
+ }
+
+ return result;
+};
+
+export const mergeComponentStyles__DEV = (
+ stylesA: ComponentSlotStylesInput | null | undefined,
+ stylesB: ComponentSlotStylesInput | null | undefined,
+): ComponentSlotStylesPrepared => {
+ if (!isDebugEnabled) {
+ return mergeComponentStyles__PROD(stylesA, stylesB);
+ }
+
+ const mergedKeys = [...(stylesA ? Object.keys(stylesA) : []), ...(stylesB ? Object.keys(stylesB) : [])];
+ const result = {};
+
+ mergedKeys.forEach(slotName => {
+ const slotA = styleParam => {
+ const { _debug = undefined, ...styles } = callable(stylesA?.[slotName])(styleParam) || {};
+
+ // new object required to prevent circular JSON structure error in <Debug />
+ return {
+ ...styles,
+ _debug: _debug || [{ styles: { ...styles }, debugId: stylesA._debugId }],
+ };
+ };
+
+ const slotB = styleParam => {
+ const { _debug = undefined, ...styles } = callable(stylesB?.[slotName])(styleParam) || {};
+
+ // new object required to prevent circular JSON structure error in <Debug />
+ return {
+ ...styles,
+ _debug: _debug || [{ styles: { ...styles }, debugId: stylesB._debugId }],
+ };
+ };
+
+ if (stylesA?.[slotName] && stylesB?.[slotName]) {
+ // We have both, replace with merge fn
+ result[slotName] = styleParam => {
+ // slot* are always prepared, fn is guaranteed, _debug always exists
+ const { _debug: debugA, ...resolvedStylesA } = slotA(styleParam);
+ const { _debug: debugB, ...resolvedStylesB } = slotB(styleParam);
+
+ const merged = _.merge(resolvedStylesA, resolvedStylesB);
+
+ merged._debug = debugA.concat(debugB || { styles: resolvedStylesB, debugId: resolvedStylesB._debugId });
 
- const merged = _.merge(targetStyles, sourceStyles);
- merged._debug = targetDebug.concat(sourceDebug || { styles: sourceStyles, debugId: stylesByPart._debugId });
  return merged;
  };
- });
+ } else if (stylesA?.[slotName]) {
+ result[slotName] = slotA;
+ } else if (stylesB?.[slotName]) {
+ result[slotName] = slotB;
+ }
+ });
 
- return partStylesPrepared;
- }, initial);
+ return result;
 };
 
 export const mergeComponentStyles: (

diff --git a/packages/fluentui/styles/test/mergeThemes/mergeComponentStyles-test.ts b/packages/fluentui/styles/test/mergeThemes/mergeComponentStyles-test.ts
@@ -64,16 +64,6 @@ describe('mergeComponentStyles', () => {
  expect(() => mergeComponentStyles(stylesWithUndefined, styles)).not.toThrow();
  });
 
- test('component parts without styles are not merged', () => {
- const target = { root: {} };
- const source = { icon: {} };
-
- const merged = mergeComponentStyles(target, source);
-
- expect(merged).not.toHaveProperty('root');
- expect(merged).not.toHaveProperty('icon');
- });
-
  test('component parts with style properties are merged', () => {
  const target = { root: { color: 'red' } };
  const source = { icon: { color: 'red' } };
@@ -165,6 +155,18 @@ describe('mergeComponentStyles', () => {
  const resolvedRoot = merged.root(styleParam);
  expect(resolvedRoot._debug).toBe(undefined);
  });
+
+ test('keeps references if possible', () => {
+ const styleRoot = jest.fn();
+
+ const firstMerge = mergeComponentStyles__PROD({ root: styleRoot }, { root: styleRoot });
+ const secondMerge = mergeComponentStyles__PROD(firstMerge, { root: styleRoot });
+
+ expect(secondMerge.root).toBe(styleRoot);
+
+ secondMerge.root(styleParam);
+ expect(styleRoot).toHaveBeenCalledTimes(1);
+ });
  });
 
  describe('dev version, debug disabled', () => {
@@ -225,47 +227,25 @@ describe('mergeComponentStyles', () => {
  });
 
  test('are flat for recursive merge', () => {
- const target = withDebugId(
- {
- root: {
- a: 'tA',
- },
- },
- 'target',
- );
- const source1 = withDebugId(
- {
- root: {
- a: 'tB',
- },
- },
- 'source1',
- );
- const source2 = withDebugId(
- {
- root: {
- a: 'tC',
- },
- },
- 'source2',
- );
+ const target = withDebugId({ root: { a: 'tA' } }, 'target');
+ const source = withDebugId({ root: { a: 'tB' } }, 'source');
 
- const merged1 = mergeComponentStyles__DEV(target, source1, source2);
+ const merged1 = mergeComponentStyles__DEV(target, source);
  const resolvedRoot1 = merged1.root(styleParam);
  expect(resolvedRoot1).toMatchObject({
- _debug: [{ debugId: 'target' }, { debugId: 'source1' }, { debugId: 'source2' }],
+ _debug: [{ debugId: 'target' }, { debugId: 'source' }],
  });
 
- const merged2 = mergeComponentStyles__DEV(mergeComponentStyles__DEV(target, source1), source2);
+ const merged2 = mergeComponentStyles__DEV(mergeComponentStyles__DEV(target, source), source);
  const resolvedRoot2 = merged2.root(styleParam);
  expect(resolvedRoot2).toMatchObject({
- _debug: [{ debugId: 'target' }, { debugId: 'source1' }, { debugId: 'source2' }],
+ _debug: [{ debugId: 'target' }, { debugId: 'source' }, { debugId: 'source' }],
  });
 
- const merged3 = mergeComponentStyles__DEV(target, mergeComponentStyles__DEV(source1, source2));
+ const merged3 = mergeComponentStyles__DEV(target, mergeComponentStyles__DEV(source, source));
  const resolvedRoot3 = merged3.root(styleParam);
  expect(resolvedRoot3).toMatchObject({
- _debug: [{ debugId: 'target' }, { debugId: 'source1' }, { debugId: 'source2' }],
+ _debug: [{ debugId: 'target' }, { debugId: 'source' }, { debugId: 'source' }],
  });
  });
  });

diff --git a/packages/fluentui/styles/test/mergeThemes/mergeThemes-test.ts b/packages/fluentui/styles/test/mergeThemes/mergeThemes-test.ts
@@ -185,16 +185,6 @@ describe('mergeThemes', () => {
  expect(merged.componentStyles).toHaveProperty('Icon');
  });
 
- test('component parts without styles are not merged', () => {
- const target = { componentStyles: { Button: { root: {} } } };
- const source = { componentStyles: { Button: { icon: {} } } };
-
- const merged = mergeThemes(target, source);
-
- expect(merged.componentStyles.Button).not.toHaveProperty('root');
- expect(merged.componentStyles.Button).not.toHaveProperty('icon');
- });
-
  test('component parts with style properties are merged', () => {
  const target = { componentStyles: { Button: { root: { color: 'red' } } } };
  const source = { componentStyles: { Icon: { root: { color: 'red' } } } };