Repository: uber-web/baseui
Pull request #262 (2018-09-17 18:57:36)

chore(overrides): better merging of overrides objects
This PR updates our `overrides` logic to have more intelligent merging capabilities.

#### Motivation

We'll use an example of ModalButton, which is just a `Button` with some default margin applied to it:

```js
const overrides = {
  BaseButton: {
    style: ({$theme}) => ({
      marginLeft: $theme.sizing.scale500,
    }),
  },
};

export default class ModalButton extends React.Component<ButtonPropsT> {
  static defaultProps = Button.defaultProps;

  render() {
    return (
      <Button
        {...this.props}
        overrides={mergeOverrides(overrides, this.props.overrides)}
      >
        {this.props.children}
      </Button>
    );
  }
}
```

With the existing logic on master, someone passing in a style override to ModalButton would _replace_ the margin styles here. The same goes for props (although we don't have any props in this example).

Ideally we can apply our intermediate overrides, and also accept overrides from the parent component that get _merged_ into our intermediate overrides. That's what this PR introduces, and it should make style overrides work much more predictably for components that reuse other base ui components.

baseui-probot-app-workflow[bot]: Found TODOs without GitHub issues:
https://github.com/uber-web/baseui/blob/c89bf8ed7938d0e53603c7388b2ae8dca7088277/src/helpers/overrides.js#L33
schnerd: > if this is a breaking change, should we add a description of what users are affected in what way, and how can they migrate?

Actually did a bit more digging and it looks like none of our components are using `mergeOverrides` with with styles/props (only components), and this PR doesn't change the component for semantics. So I take back my statement – this should probably just be a patch bump.
gergelyke: cool, thanks David 👍 

Patch:
diff --git a/src/helpers/overrides.js b/src/helpers/overrides.js
index d026c873e..04237cde2 100644
--- a/src/helpers/overrides.js
+++ b/src/helpers/overrides.js
@@ -6,11 +6,14 @@ LICENSE file in the root directory of this source tree.
 */
 // @flow
 import * as React from 'react';
+import deepMerge from '../utils/deep-merge';
+
+type StyleOverrideT = {} | (({}) => ?{});
 
 export type OverrideObjectT<T> = {|
   component?: ?React.ComponentType<T>,
   props?: ?{},
-  style?: ?{},
+  style?: ?StyleOverrideT,
 |};
 
 export type OverrideT<T> = OverrideObjectT<T> | React.ComponentType<T>;
@@ -19,12 +22,15 @@ export type OverridesT<T> = {
   [string]: OverrideT<T>,
 };
 
+/**
+ * Given an override argument, returns the component implementation override if it exists
+ */
 export function getOverride<T>(
   override: ?OverrideT<T>,
 ): ?React.ComponentType<T> {
   // Check if override is OverrideObjectT
   if (override && typeof override === 'object') {
-    // TODO remove this 'any' once this flow issue is fixed:
+    // Remove this 'any' once this flow issue is fixed:
     // https://github.com/facebook/flow/issues/6666
     // eslint-disable-next-line flowtype/no-weak-types
     return (override: any).component;
@@ -33,6 +39,10 @@ export function getOverride<T>(
   return override;
 }
 
+/**
+ * Given an override argument, returns the override props that should be passed
+ * to the component when rendering it.
+ */
 export function getOverrideProps<T>(override: ?OverrideT<T>) {
   if (override && typeof override === 'object') {
     return {
@@ -46,16 +56,22 @@ export function getOverrideProps<T>(override: ?OverrideT<T>) {
 }
 
 /**
- * Coerces an override value into an override object
+ * Coerces an override argument into an override object
  * (sometimes it is just an override component)
  */
-export function toObjectOverride<T>(override: OverrideT<T>): OverrideT<T> {
+export function toObjectOverride<T>(
+  override: OverrideT<T>,
+): OverrideObjectT<T> {
   if (typeof override === 'function') {
     return {
       component: (override: React.ComponentType<T>),
     };
   }
-  return override;
+  // Flow can't figure out that typeof 'function' above will
+  // catch React.StatelessFunctionalComponent
+  // (probably related to https://github.com/facebook/flow/issues/6666)
+  // eslint-disable-next-line flowtype/no-weak-types
+  return ((override || {}: any): OverrideObjectT<T>);
 }
 
 /**
@@ -74,19 +90,63 @@ export function getOverrideObject<T>(
 }
 
 /**
- * Merges two override objects – this is useful if you want to
- * inject your own overrides into a child component, but also
- * accept further overrides from your parent.
+ * Merges two overrides objects – this is useful if you want to inject your own
+ * overrides into a child component, but also accept further overrides from
+ * from upstream. See `mergeOverride` below.
  */
 export function mergeOverrides<T>(
   target?: OverridesT<T> = {},
   source?: OverridesT<T> = {},
 ): OverridesT<T> {
-  return Object.keys({...target, ...source}).reduce((acc, name) => {
-    acc[name] = {
-      ...toObjectOverride(target[name]),
-      ...toObjectOverride(source[name]),
-    };
+  const allIdentifiers = Object.keys({...target, ...source});
+  return allIdentifiers.reduce((acc, name) => {
+    acc[name] = mergeOverride(
+      toObjectOverride(target[name]),
+      toObjectOverride(source[name]),
+    );
     return acc;
   }, {});
 }
+
+/**
+ * Merges two override objects using the following behavior:
+ * - Component implementation from the source (parent) replaces target
+ * - Props and styles are both deep merged
+ */
+export function mergeOverride<T>(
+  target: OverrideObjectT<T>,
+  source: OverrideObjectT<T>,
+): OverrideObjectT<T> {
+  // Shallow merge should handle `component`
+  const merged = {...target, ...source};
+  // Props just use deep merge
+  if (target.props && source.props) {
+    merged.props = deepMerge({}, target.props, source.props);
+  }
+  // Style overrides need special merging since they may be functions
+  if (target.style && source.style) {
+    merged.style = mergeStyleOverrides(target.style, source.style);
+  }
+  return merged;
+}
+
+/**
+ * Since style overrides can be an object *or* a function, we need to handle
+ * the case that one of them is a function. We do this by returning a new
+ * function that deep emrges the result of each style override
+ */
+function mergeStyleOverrides(target: StyleOverrideT, source: StyleOverrideT) {
+  // Simple case of both objects
+  if (typeof target === 'object' && typeof source === 'object') {
+    return deepMerge({}, target, source);
+  }
+
+  // At least one is a function, return a new composite function
+  return (...args) => {
+    return deepMerge(
+      {},
+      typeof target === 'function' ? target(...args) : target,
+      typeof source === 'function' ? source(...args) : source,
+    );
+  };
+}
diff --git a/src/utils/deep-merge.js b/src/utils/deep-merge.js
new file mode 100644
index 000000000..2c250c3a3
--- /dev/null
+++ b/src/utils/deep-merge.js
@@ -0,0 +1,29 @@
+// @flow
+
+export default function deepMerge(
+  target?: ?{},
+  ...sources: Array<null | ?{}>
+): {} {
+  target = target || {};
+  const len = sources.length;
+  let obj;
+  let value;
+  for (let i = 0; i < len; i++) {
+    obj = sources[i] || {};
+    for (let key in obj) {
+      if (obj.hasOwnProperty(key)) {
+        value = obj[key];
+        if (isCloneable(value)) {
+          target[key] = deepMerge(target[key] || {}, value);
+        } else {
+          target[key] = value;
+        }
+      }
+    }
+  }
+  return target;
+}
+
+function isCloneable(obj: mixed) {
+  return Array.isArray(obj) || {}.toString.call(obj) == '[object Object]';
+}


Test patch:
diff --git a/src/helpers/__tests__/__snapshots__/overrides.test.js.snap b/src/helpers/__tests__/__snapshots__/overrides.test.js.snap
index 5adcd5cef..830dad7b6 100644
--- a/src/helpers/__tests__/__snapshots__/overrides.test.js.snap
+++ b/src/helpers/__tests__/__snapshots__/overrides.test.js.snap
@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`Helpers - Overrides - getOverrideProps: returns correct object when override has props and styles 1`] = `
+exports[`Helpers - Overrides getOverrideProps: returns correct object when override has props and styles 1`] = `
 Object {
   "$style": Object {
     "color": "blue",
@@ -9,6 +9,6 @@ Object {
 }
 `;
 
-exports[`Helpers - Overrides - getOverrideProps: returns empty object when no overrides 1`] = `Object {}`;
+exports[`Helpers - Overrides getOverrideProps: returns empty object when no overrides 1`] = `Object {}`;
 
-exports[`Helpers - Overrides - getOverrideProps: returns empty object when override is a component 1`] = `Object {}`;
+exports[`Helpers - Overrides getOverrideProps: returns empty object when override is a component 1`] = `Object {}`;
diff --git a/src/helpers/__tests__/overrides.test.js b/src/helpers/__tests__/overrides.test.js
index 5c0bfc400..041438b28 100644
--- a/src/helpers/__tests__/overrides.test.js
+++ b/src/helpers/__tests__/overrides.test.js
@@ -11,6 +11,7 @@ import {
   getOverrideProps,
   toObjectOverride,
   mergeOverrides,
+  mergeOverride,
   getOverrideObject,
 } from '../overrides';
 
@@ -19,116 +20,158 @@ function getMockComponent<T>(): React.ComponentType<T> {
   return mock;
 }
 
-test('Helpers - Overrides - getOverride', () => {
-  const CustomComponent = getMockComponent();
-  expect(getOverride(null)).toEqual(null);
-  expect(getOverride(CustomComponent)).toEqual(CustomComponent);
-  expect(getOverride({component: CustomComponent})).toEqual(CustomComponent);
-});
-
-test('Helpers - Overrides - getOverrideProps', () => {
-  const CustomComponent = getMockComponent();
-  const override = {
-    props: {propName: 'propsValue'},
-    style: {color: 'blue'},
-  };
-  expect(getOverrideProps(null)).toMatchSnapshot(
-    'returns empty object when no overrides',
-  );
-  expect(getOverrideProps(CustomComponent)).toMatchSnapshot(
-    'returns empty object when override is a component',
-  );
-  expect(getOverrideProps(override)).toMatchSnapshot(
-    'returns correct object when override has props and styles',
-  );
-});
+describe('Helpers - Overrides', () => {
+  test('getOverride', () => {
+    const CustomComponent = getMockComponent();
+    expect(getOverride(null)).toEqual(null);
+    expect(getOverride(CustomComponent)).toEqual(CustomComponent);
+    expect(getOverride({component: CustomComponent})).toEqual(CustomComponent);
+  });
 
-test('Helpers - Overrides - toObjectOverride', () => {
-  const CustomComponent = getMockComponent();
-  // $FlowFixMe - Calling toObjectOverride with no args
-  expect(toObjectOverride()).toBeUndefined();
-  // $FlowFixMe - Calling toObjectOverride with null
-  expect(toObjectOverride(null)).toBe(null);
-  expect(toObjectOverride(CustomComponent)).toEqual({
-    component: CustomComponent,
+  test('getOverrideProps', () => {
+    const CustomComponent = getMockComponent();
+    const override = {
+      props: {propName: 'propsValue'},
+      style: {color: 'blue'},
+    };
+    expect(getOverrideProps(null)).toMatchSnapshot(
+      'returns empty object when no overrides',
+    );
+    expect(getOverrideProps(CustomComponent)).toMatchSnapshot(
+      'returns empty object when override is a component',
+    );
+    expect(getOverrideProps(override)).toMatchSnapshot(
+      'returns correct object when override has props and styles',
+    );
   });
-  expect(
-    toObjectOverride({
-      component: (CustomComponent: React.ComponentType<*>),
+
+  test('toObjectOverride', () => {
+    const CustomComponent = getMockComponent();
+    // $FlowFixMe - Calling toObjectOverride with no args
+    expect(toObjectOverride()).toEqual({});
+    // $FlowFixMe - Calling toObjectOverride with null
+    expect(toObjectOverride(null)).toEqual({});
+    expect(toObjectOverride(CustomComponent)).toEqual({
+      component: CustomComponent,
+    });
+    expect(
+      toObjectOverride({
+        component: (CustomComponent: React.ComponentType<*>),
+        style: {width: '300px'},
+      }),
+    ).toEqual({
+      component: CustomComponent,
       style: {width: '300px'},
-    }),
-  ).toEqual({
-    component: CustomComponent,
-    style: {width: '300px'},
+    });
   });
-});
 
-test('Helpers - Overrides - mergeOverrides', () => {
-  const CustomRoot = getMockComponent();
-  const CustomFoo = getMockComponent();
-  const CustomBar = getMockComponent();
-
-  const overrides1 = {
-    Root: CustomRoot,
-  };
-
-  const overrides2 = {
-    Root: {
-      component: (CustomFoo: React.ComponentType<*>),
-    },
-    Bar: CustomBar,
-  };
-
-  expect(mergeOverrides(overrides1)).toEqual({
-    Root: {
-      component: CustomRoot,
-    },
-  });
+  test('mergeOverrides', () => {
+    const CustomRoot = getMockComponent();
+    const CustomFoo = getMockComponent();
+    const CustomBar = getMockComponent();
+
+    const overrides1 = {
+      Root: CustomRoot,
+    };
+
+    const overrides2 = {
+      Root: {
+        component: (CustomFoo: React.ComponentType<*>),
+      },
+      Bar: CustomBar,
+    };
+
+    expect(mergeOverrides(overrides1)).toEqual({
+      Root: {
+        component: CustomRoot,
+      },
+    });
 
-  expect(mergeOverrides(overrides1, overrides2)).toEqual({
-    Root: {
-      component: CustomFoo,
-    },
-    Bar: {
-      component: CustomBar,
-    },
+    expect(mergeOverrides(overrides1, overrides2)).toEqual({
+      Root: {
+        component: CustomFoo,
+      },
+      Bar: {
+        component: CustomBar,
+      },
+    });
   });
-});
 
-test('Helpers - Overrides - getOverrideObject', () => {
-  const DefaultComponent = getMockComponent();
-  const OverrideComponent = getMockComponent();
+  test('mergeOverride can merge props and style objects', () => {
+    expect(
+      mergeOverride(
+        {
+          props: {foo: true, bar: false},
+          style: {color: 'red', textTransform: 'uppercase'},
+        },
+        {
+          props: {foo: false},
+          style: {color: 'blue'},
+        },
+      ),
+    ).toEqual({
+      props: {foo: false, bar: false},
+      style: {color: 'blue', textTransform: 'uppercase'},
+    });
+  });
 
-  expect(getOverrideObject(null, DefaultComponent)).toEqual({
-    component: DefaultComponent,
-    props: {},
+  test('mergeOverride doesnt unnecessarily create new objects', () => {
+    const override1 = {props: {foo: true}};
+    const override2 = {style: {color: 'red'}};
+    const result = mergeOverride(override1, override2);
+    expect(result.props).toBe(override1.props);
+    expect(result.style).toBe(override2.style);
   });
 
-  expect(getOverrideObject(OverrideComponent, DefaultComponent)).toEqual({
-    component: OverrideComponent,
-    props: {},
+  test('mergeOverride can compose style functions', () => {
+    const override1 = {
+      style: () => ({color: 'red', textTransform: 'uppercase'}),
+    };
+    const override2 = {style: () => ({color: 'blue'})};
+    const result = mergeOverride(override1, override2);
+    expect(typeof result.style).toBe('function');
+    // $FlowFixMe style should be a function here
+    expect(result.style()).toEqual({
+      color: 'blue',
+      textTransform: 'uppercase',
+    });
   });
 
-  expect(
-    getOverrideObject(
-      {
-        component: OverrideComponent,
-        props: {
-          custom: 'prop',
+  test('getOverrideObject', () => {
+    const DefaultComponent = getMockComponent();
+    const OverrideComponent = getMockComponent();
+
+    expect(getOverrideObject(null, DefaultComponent)).toEqual({
+      component: DefaultComponent,
+      props: {},
+    });
+
+    expect(getOverrideObject(OverrideComponent, DefaultComponent)).toEqual({
+      component: OverrideComponent,
+      props: {},
+    });
+
+    expect(
+      getOverrideObject(
+        {
+          component: OverrideComponent,
+          props: {
+            custom: 'prop',
+          },
+          style: {
+            cursor: 'pointer',
+          },
         },
-        style: {
+        DefaultComponent,
+      ),
+    ).toEqual({
+      component: OverrideComponent,
+      props: {
+        custom: 'prop',
+        $style: {
           cursor: 'pointer',
         },
       },
-      DefaultComponent,
-    ),
-  ).toEqual({
-    component: OverrideComponent,
-    props: {
-      custom: 'prop',
-      $style: {
-        cursor: 'pointer',
-      },
-    },
+    });
   });
 });
diff --git a/src/utils/__tests__/deep-merge.test.js b/src/utils/__tests__/deep-merge.test.js
new file mode 100644
index 000000000..ee0ac81ec
--- /dev/null
+++ b/src/utils/__tests__/deep-merge.test.js
@@ -0,0 +1,20 @@
+// @flow
+import deepMerge from '../deep-merge';
+
+describe('deepMerge', () => {
+  test('performs deep merge on target', () => {
+    const target = {foo: {bar: {baz: true, quux: true}}};
+    const source = {foo: {bar: {baz: false}}};
+    const ret = deepMerge(target, source);
+    expect(ret).toBe(target);
+    expect(target).toEqual({foo: {bar: {baz: false, quux: true}}});
+  });
+
+  test('can handle three way merge', () => {
+    const target = {foo: 1, bar: 2};
+    const source1 = {foo: 2, baz: 3};
+    const source2 = {foo: 3, quux: 4};
+    const ret = deepMerge({}, target, source1, source2);
+    expect(ret).toEqual({foo: 3, bar: 2, baz: 3, quux: 4});
+  });
+});