fix(theme): merge partial with empty initial partial (#1452) (#1454)

elastic · Nov 2, 2021 · 2eadc71 · 2eadc71
1 parent 89ceafe
commit 2eadc71
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 24 deletions.
diff --git a/packages/charts/api/charts.api.md b/packages/charts/api/charts.api.md
@@ -1346,7 +1346,7 @@ export function mergeWithDefaultAnnotationLine(config?: Partial<LineAnnotationSt
 // @public (undocumented)
 export function mergeWithDefaultAnnotationRect(config?: Partial<RectAnnotationStyle>): RectAnnotationStyle;
 
-// @public
+// @public @deprecated
 export function mergeWithDefaultTheme(theme: PartialTheme, defaultTheme?: Theme, auxiliaryThemes?: PartialTheme[]): Theme;
 
 // @public (undocumented)

diff --git a/packages/charts/src/state/selectors/get_chart_theme.ts b/packages/charts/src/state/selectors/get_chart_theme.ts
@@ -6,8 +6,8 @@
  * Side Public License, v 1.
  */
 
+import { mergePartial } from '../../utils/common';
 import { LIGHT_THEME } from '../../utils/themes/light_theme';
-import { mergeWithDefaultTheme } from '../../utils/themes/merge_utils';
 import { PartialTheme, Theme } from '../../utils/themes/theme';
 import { createCustomCachedSelector } from '../create_selector';
 import { getSettingsSpecSelector } from './get_settings_specs';
@@ -19,12 +19,12 @@ export const getChartThemeSelector = createCustomCachedSelector(
 );
 
 function getTheme(baseTheme?: Theme, theme?: PartialTheme | PartialTheme[]): Theme {
-  const base = baseTheme || LIGHT_THEME;
+  const base = baseTheme ?? LIGHT_THEME;
 
   if (Array.isArray(theme)) {
     const [firstTheme, ...axillaryThemes] = theme;
-    return mergeWithDefaultTheme(firstTheme, base, axillaryThemes);
+    return mergePartial(base, firstTheme, {}, axillaryThemes);
   }
 
-  return theme ? mergeWithDefaultTheme(theme, base) : base;
+  return theme ? mergePartial(base, theme) : base;
 }
diff --git a/packages/charts/src/utils/common.test.ts b/packages/charts/src/utils/common.test.ts
@@ -105,28 +105,46 @@ describe('common utilities', () => {
       key6: 6,
     };
 
+    it('should allow undefined object', () => {
+      const result = getAllKeys(undefined, [object1]);
+
+      expect(result).toEqual(new Set(['key1', 'key2']));
+    });
+
+    it('should return allow undefined objects', () => {
+      const result = getAllKeys(undefined, undefined);
+
+      expect(result).toEqual(new Set([]));
+    });
+
+    it('should remove duplicates', () => {
+      const result = getAllKeys(object1, [object1]);
+
+      expect(result).toEqual(new Set(['key1', 'key2']));
+    });
+
     it('should return all keys from single object', () => {
       const result = getAllKeys(object1);
 
-      expect(result).toEqual(['key1', 'key2']);
+      expect(result).toEqual(new Set(['key1', 'key2']));
     });
 
     it('should return all keys from all objects x 2', () => {
       const result = getAllKeys(object1, [object2]);
 
-      expect(result).toEqual(['key1', 'key2', 'key3', 'key4']);
+      expect(result).toEqual(new Set(['key1', 'key2', 'key3', 'key4']));
     });
 
     it('should return all keys from single objects x 3', () => {
       const result = getAllKeys(object1, [object2, object3]);
 
-      expect(result).toEqual(['key1', 'key2', 'key3', 'key4', 'key5', 'key6']);
+      expect(result).toEqual(new Set(['key1', 'key2', 'key3', 'key4', 'key5', 'key6']));
     });
 
     it('should return all keys from only defined objects', () => {
       const result = getAllKeys(object1, [null, object2, {}, undefined]);
 
-      expect(result).toEqual(['key1', 'key2', 'key3', 'key4']);
+      expect(result).toEqual(new Set(['key1', 'key2', 'key3', 'key4']));
     });
   });
 
@@ -726,6 +744,58 @@ describe('common utilities', () => {
           },
         });
       });
+
+      test('should traverse all partial keys', () => {
+        interface Test {
+          crosshair: {
+            line: {
+              dash?: number[];
+              strokeWidth: number;
+            };
+            band?: {
+              visible: boolean;
+            };
+          };
+        }
+        const customBase: Test = {
+          crosshair: {
+            line: {
+              strokeWidth: 1,
+            },
+          },
+        };
+        const partials: RecursivePartial<Test>[] = [
+          {
+            crosshair: {
+              line: {
+                dash: [4, 4],
+              },
+            },
+          },
+          {
+            crosshair: {
+              line: {
+                strokeWidth: 10,
+              },
+              band: {
+                visible: true,
+              },
+            },
+          },
+        ];
+        const newBase = mergePartial(customBase, {}, {}, partials);
+        expect(newBase).toEqual({
+          crosshair: {
+            line: {
+              strokeWidth: 10,
+              dash: [4, 4],
+            },
+            band: {
+              visible: true,
+            },
+          },
+        });
+      });
     });
 
     describe('MergeOptions', () => {

diff --git a/packages/charts/src/utils/common.ts b/packages/charts/src/utils/common.ts
@@ -255,17 +255,17 @@ export function getPartialValue<T>(base: T, partial?: RecursivePartial<T>, parti
  * @param objects
  * @internal
  */
-export function getAllKeys(object: any, objects: any[] = []): string[] {
-  const initialKeys = object instanceof Map ? [...object.keys()] : Object.keys(object);
-
-  return objects.reduce((keys: any[], obj) => {
-    if (obj && typeof obj === 'object') {
-      const newKeys = obj instanceof Map ? obj.keys() : Object.keys(obj);
-      keys.push(...newKeys);
-    }
+export function getAllKeys(object?: any, objects: any[] = []): Set<any> {
+  return new Set(
+    [object, ...objects].filter(Boolean).reduce((keys: any[], obj) => {
+      if (obj && typeof obj === 'object') {
+        const newKeys = obj ? (obj instanceof Map ? obj.keys() : Object.keys(obj)) : [];
+        keys.push(...newKeys);
+      }
 
-    return keys;
-  }, initialKeys);
+      return keys;
+    }, []),
+  );
 }
 
 /** @internal */
@@ -357,8 +357,9 @@ export function mergePartial<T>(
 
   if (hasPartialObjectToMerge(base, partial, additionalPartials)) {
     const mapCondition = !(baseClone instanceof Map) || options.mergeMaps;
-    if (partial !== undefined && (options.mergeOptionalPartialValues ?? true) && mapCondition) {
-      getAllKeys(partial, additionalPartials).forEach((key) => {
+    const partialKeys = getAllKeys(partial, additionalPartials);
+    if (partialKeys.size > 0 && (options.mergeOptionalPartialValues ?? true) && mapCondition) {
+      partialKeys.forEach((key) => {
         if (baseClone instanceof Map) {
           if (!baseClone.has(key)) {
             baseClone.set(
@@ -370,9 +371,9 @@ export function mergePartial<T>(
           }
         } else if (!(key in baseClone)) {
           baseClone[key] =
-            (partial as any)[key] !== undefined
+            (partial as any)?.[key] !== undefined
               ? (partial as any)[key]
-              : (additionalPartials.find((v: any) => v[key] !== undefined) || ({} as any))[key];
+              : (additionalPartials.find((v: any) => v?.[key] !== undefined) ?? ({} as any))[key];
         }
       });
     }
@@ -404,7 +405,7 @@ export function mergePartial<T>(
       return baseClone as any;
     }
 
-    return Object.keys(base).reduce((newBase, key) => {
+    return Object.keys(baseClone).reduce((newBase, key) => {
       const partialValue = partial && (partial as any)[key];
       const partialValues = additionalPartials.map((v) => (typeof v === 'object' ? (v as any)[key] : undefined));
       const baseValue = (base as any)[key];

diff --git a/packages/charts/src/utils/themes/merge_utils.ts b/packages/charts/src/utils/themes/merge_utils.ts
@@ -82,6 +82,7 @@ export function mergeWithDefaultAnnotationRect(config?: Partial<RectAnnotationSt
  * @param auxiliaryThemes - additional themes to be merged
  *
  * @public
+ * @deprecated - Please use `baseTheme` and `theme` on Settings instead
  */
 export function mergeWithDefaultTheme(
   theme: PartialTheme,