Repository: uber-web/baseui (js)

Issue:
Prop overrides should accept a function
Currently, you can pass a function into `style` and `component` overrides but not props. For example:

<Component overrides={{  props: { 'data-test': 'foo' } }} />

works but

<Component overrides={{  props: props => ({ 'data-test': 'foo' }) }} />

doesn't.

This makes it inconsistent with `style` and `component` overrides and prevents some use-cases like adding BEM class names for different internal states of the component.

Pull request:
feat(overrides): prop overrides accept a function too
Resolves #1552 <!-- remove the (`) quotes to link the issues -->

#### Description
Make it possible to pass a function into props overrides. 
Rename `mergeStyleOverrides` to `mergeSpecialOverrides` since the same logic apply to `styles` and `props` overrides.
One important thing, since `mergeStyleOverrides` is only used inside `overrides.js` file I suggest that it is an internal function, so renaming it, is not breaking changes. (If it is breaking changes, we can define an alias for `mergeSpecialOverrides` as  `mergeStyleOverrides` and remove him when the new major version released)
Also, updated flow types, updated unit tests for `getOverrideProps` and  `getOverrides`.
<!-- Describe your changes below in as much detail as possible -->

#### Scope
- [X] Minor: New Feature
- [X] Major: Maybe Breaking Change


Patch:
diff --git a/src/helpers/overrides.js b/src/helpers/overrides.js
index f0277e26d..93534e213 100644
--- a/src/helpers/overrides.js
+++ b/src/helpers/overrides.js
@@ -9,15 +9,19 @@ import * as React from 'react';
 import {isValidElementType} from 'react-is';
 import deepMerge from '../utils/deep-merge.js';
 
-export type StyleOverrideFunctionT = ({}) => ?{};
-export type StyleOverrideObjectT = {};
+export type ConfigurationOverrideFunctionT = ({}) => ?{};
+export type ConfigurationOverrideObjectT = {};
 
-export type StyleOverrideT = StyleOverrideObjectT | StyleOverrideFunctionT;
+export type ConfigurationOverrideT =
+  | ConfigurationOverrideObjectT
+  | ConfigurationOverrideFunctionT;
+
+export type StyleOverrideT = ConfigurationOverrideT;
 
 export type OverrideObjectT<T> = {|
   component?: ?React.ComponentType<T & {children: React.Node}>,
-  props?: ?{},
-  style?: ?StyleOverrideT,
+  props?: ?ConfigurationOverrideT,
+  style?: ?ConfigurationOverrideT,
 |};
 
 export type OverrideT<T> =
@@ -55,8 +59,12 @@ export function getOverride(override: any): any {
  */
 export function getOverrideProps<T>(override: ?OverrideT<T>) {
   if (override && typeof override === 'object') {
+    const props =
+      typeof override.props === 'function'
+        ? override.props(override)
+        : override.props;
     return {
-      ...override.props,
+      ...props,
       $style: override.style,
     };
   }
@@ -128,26 +136,24 @@ export function mergeOverride<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);
+    merged.props = mergeConfigurationOverrides(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);
+    merged.style = mergeConfigurationOverrides(target.style, source.style);
   }
   return merged;
 }
 
 /**
- * Since style overrides can be an object *or* a function, we need to handle
+ * Since style or props 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 merges the result of each style override
  */
-export function mergeStyleOverrides(
-  target: StyleOverrideT,
-  source: StyleOverrideT,
-): StyleOverrideT {
+export function mergeConfigurationOverrides(
+  target: ConfigurationOverrideT,
+  source: ConfigurationOverrideT,
+): ConfigurationOverrideT {
   // Simple case of both objects
   if (typeof target === 'object' && typeof source === 'object') {
     return deepMerge({}, target, source);


Test patch:
diff --git a/src/helpers/__tests__/__snapshots__/overrides.test.js.snap b/src/helpers/__tests__/__snapshots__/overrides.test.js.snap
index 830dad7b6..d0a5b928d 100644
--- a/src/helpers/__tests__/__snapshots__/overrides.test.js.snap
+++ b/src/helpers/__tests__/__snapshots__/overrides.test.js.snap
@@ -9,6 +9,29 @@ Object {
 }
 `;
 
+exports[`Helpers - Overrides getOverrideProps: returns correct object when props is a function and styles in override is not present 1`] = `
+Object {
+  "$style": undefined,
+  "propName": "propValue",
+}
+`;
+
+exports[`Helpers - Overrides getOverrideProps: returns correct object when props is a function and styles in override is present 1`] = `
+Object {
+  "$style": Object {
+    "color": "red",
+  },
+  "className": "someCSSClass",
+  "propName": "propValue",
+}
+`;
+
+exports[`Helpers - Overrides getOverrideProps: returns correct object when props is a function which return null 1`] = `
+Object {
+  "$style": undefined,
+}
+`;
+
 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 {}`;
diff --git a/src/helpers/__tests__/overrides.test.js b/src/helpers/__tests__/overrides.test.js
index 8b8fb716b..85a4d5c45 100644
--- a/src/helpers/__tests__/overrides.test.js
+++ b/src/helpers/__tests__/overrides.test.js
@@ -12,7 +12,7 @@ import {
   toObjectOverride,
   mergeOverrides,
   mergeOverride,
-  mergeStyleOverrides,
+  mergeConfigurationOverrides,
   getOverrides,
 } from '../overrides.js';
 
@@ -35,6 +35,15 @@ describe('Helpers - Overrides', () => {
       props: {propName: 'propsValue'},
       style: {color: 'blue'},
     };
+    const propsAsFunction = override => {
+      const props: {propName: string, className?: string} = {
+        propName: 'propValue',
+      };
+      if (override.style) {
+        props.className = 'someCSSClass';
+      }
+      return props;
+    };
     expect(getOverrideProps(null)).toMatchSnapshot(
       'returns empty object when no overrides',
     );
@@ -44,6 +53,24 @@ describe('Helpers - Overrides', () => {
     expect(getOverrideProps(override)).toMatchSnapshot(
       'returns correct object when override has props and styles',
     );
+    expect(getOverrideProps({props: propsAsFunction})).toMatchSnapshot(
+      'returns correct object when props is a function and styles in override is not present',
+    );
+    expect(
+      getOverrideProps({
+        props: propsAsFunction,
+        style: {color: 'red'},
+      }),
+    ).toMatchSnapshot(
+      'returns correct object when props is a function and styles in override is present',
+    );
+    expect(
+      getOverrideProps({
+        props: () => null,
+      }),
+    ).toMatchSnapshot(
+      'returns correct object when props is a function which return null',
+    );
   });
 
   test('toObjectOverride', () => {
@@ -138,20 +165,46 @@ describe('Helpers - Overrides', () => {
     });
   });
 
-  test('mergeStyleOverrides', () => {
-    const overrideObject1 = {color: 'red', textTransform: 'uppercase'};
-    const overrideObject2 = {color: 'blue'};
+  test('mergeOverride can compose props functions', () => {
+    const override1 = {
+      props: () => ({prop1: 'val1', prop2: 'val2'}),
+    };
+    const override2 = {props: () => ({prop1: 'newValue'})};
+    const result = mergeOverride(override1, override2);
+    expect(typeof result.props).toBe('function');
+    // $FlowFixMe props should be a function here
+    expect(result.props()).toEqual({
+      prop1: 'newValue',
+      prop2: 'val2',
+    });
+  });
+
+  test('mergeConfigurationOverrides', () => {
+    const overrideObject1 = {key1: 'value1', key2: 'value2'};
+    const overrideObject2 = {key1: 'overrideValue'};
     const overrideFunction1 = () => overrideObject1;
     const overrideFunction2 = () => overrideObject2;
     const expectedResult = {
-      color: 'blue',
-      textTransform: 'uppercase',
+      key1: 'overrideValue',
+      key2: 'value2',
     };
 
-    const result1 = mergeStyleOverrides(overrideObject1, overrideObject2);
-    const result2 = mergeStyleOverrides(overrideObject1, overrideFunction2);
-    const result3 = mergeStyleOverrides(overrideFunction1, overrideObject2);
-    const result4 = mergeStyleOverrides(overrideFunction1, overrideFunction2);
+    const result1 = mergeConfigurationOverrides(
+      overrideObject1,
+      overrideObject2,
+    );
+    const result2 = mergeConfigurationOverrides(
+      overrideObject1,
+      overrideFunction2,
+    );
+    const result3 = mergeConfigurationOverrides(
+      overrideFunction1,
+      overrideObject2,
+    );
+    const result4 = mergeConfigurationOverrides(
+      overrideFunction1,
+      overrideFunction2,
+    );
 
     expect(typeof result1).toBe('object');
     expect(result1).toEqual(expectedResult);
@@ -172,6 +225,7 @@ describe('Helpers - Overrides', () => {
   test('getOverrides', () => {
     const DefaultComponent = getMockComponent();
     const OverrideComponent = getMockComponent();
+    const propsAsFunction = props => ({key1: 'value1', key2: 'value2'});
 
     expect(getOverrides(null, DefaultComponent)).toEqual([
       DefaultComponent,
@@ -205,5 +259,27 @@ describe('Helpers - Overrides', () => {
         },
       },
     ]);
+
+    expect(
+      getOverrides(
+        {
+          component: OverrideComponent,
+          props: propsAsFunction,
+          style: {
+            cursor: 'pointer',
+          },
+        },
+        DefaultComponent,
+      ),
+    ).toEqual([
+      OverrideComponent,
+      {
+        key1: 'value1',
+        key2: 'value2',
+        $style: {
+          cursor: 'pointer',
+        },
+      },
+    ]);
   });
 });