fix(theming): harden getColor memoization key generation (#1966)

zendeskgarden · Oct 31, 2024 · 3dc00a8 · 3dc00a8
1 parent 1c6ac75
commit 3dc00a8
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 22 deletions.
diff --git a/docs/migration.md b/docs/migration.md
@@ -118,13 +118,16 @@ consider additional positioning prop support on a case-by-case basis.
 - Use this package if you were using `@zendeskgarden/react-dropdowns.next` in `v8`
   - The `v8` version of `@zendeskgarden/react-dropdowns` is no longer maintained and is
     renamed to `@zendeskgarden/react-dropdowns.legacy` in `v9`
+- `Combobox`
+  - `Option` no longer accepts an object type for the `value` prop. Use string
+    values for stable comparison.
+  - Removed `label` prop from `OptGroup`. Use `legend` instead.
 - `Menu`
   - value `auto` is no longer valid for the `fallbackPlacements` prop.
   - new `restoreFocus` prop (default: `true`) returns focus to trigger
     after menu interaction. When menu expansion is controlled to allow
     multiple item selection, set `restoreFocus={false}` and manage trigger
     focus manually on close.
-- Removed `label` prop from `OptGroup`. Use `legend` instead.
 
 #### @zendeskgarden/react-forms
 

diff --git a/packages/theming/src/utils/getColor.spec.ts b/packages/theming/src/utils/getColor.spec.ts
@@ -391,6 +391,12 @@ describe('getColor', () => {
       console.error = consoleError;
     });
 
+    it('does not throw a memoization key error when the theme is invalid', () => {
+      const test = () => getColor({ theme: {} as any, variable: 'background.default' });
+
+      expect(test).not.toThrow('Invalid value used as weak map key');
+    });
+
     it('throws an error if color arguments are missing', () => {
       expect(() => getColor({ theme: DEFAULT_THEME })).toThrow(Error);
     });

diff --git a/packages/theming/src/utils/getColor.ts b/packages/theming/src/utils/getColor.ts
@@ -281,6 +281,7 @@ CACHE.set(DEFAULT_THEME.colors, KEYS.colors);
 CACHE.set(DEFAULT_THEME.palette, KEYS.palette);
 CACHE.set(DEFAULT_THEME.opacity, KEYS.opacity);
 
+/* convert `getColor` parameters to a memoization key */
 const toKey = ({
   dark,
   hue,
@@ -291,25 +292,37 @@ const toKey = ({
   transparency,
   variable
 }: ColorParameters) => {
-  let themeColorsKey = CACHE.get(theme.colors);
+  let themeColorsKey;
 
-  if (themeColorsKey === undefined) {
-    themeColorsKey = ++KEYS.colors;
-    CACHE.set(theme.colors, themeColorsKey);
+  if (theme.colors) {
+    themeColorsKey = CACHE.get(theme.colors);
+
+    if (themeColorsKey === undefined) {
+      themeColorsKey = ++KEYS.colors;
+      CACHE.set(theme.colors, themeColorsKey);
+    }
   }
 
-  let themeOpacityKey = CACHE.get(theme.opacity);
+  let themeOpacityKey;
+
+  if (theme.opacity) {
+    themeOpacityKey = CACHE.get(theme.opacity);
 
-  if (themeOpacityKey === undefined) {
-    themeOpacityKey = ++KEYS.opacity;
-    CACHE.set(theme.opacity, themeOpacityKey);
+    if (themeOpacityKey === undefined) {
+      themeOpacityKey = ++KEYS.opacity;
+      CACHE.set(theme.opacity, themeOpacityKey);
+    }
   }
 
-  let themePaletteKey = CACHE.get(theme.palette);
+  let themePaletteKey;
+
+  if (theme.palette) {
+    themePaletteKey = CACHE.get(theme.palette);
 
-  if (themePaletteKey === undefined) {
-    themePaletteKey = ++KEYS.palette;
-    CACHE.set(theme.palette, themePaletteKey);
+    if (themePaletteKey === undefined) {
+      themePaletteKey = ++KEYS.palette;
+      CACHE.set(theme.palette, themePaletteKey);
+    }
   }
 
   let retVal = `{${themeColorsKey},${themePaletteKey},${themeOpacityKey}}`;

diff --git a/packages/theming/src/utils/getColorV8.spec.ts b/packages/theming/src/utils/getColorV8.spec.ts
@@ -167,6 +167,10 @@ describe('getColorV8', () => {
 
       expect(color).toBe(expected);
     });
+
+    it('handles an invalid theme', () => {
+      expect(() => getColorV8('test', 600, {} as any)).not.toThrow();
+    });
   });
 
   describe('by transparency', () => {

diff --git a/packages/theming/src/utils/getColorV8.ts b/packages/theming/src/utils/getColorV8.ts
@@ -42,25 +42,33 @@ const toKey = ({
   theme?: DefaultTheme;
   transparency?: number;
 }) => {
-  let retVal = `${hue}`;
+  let retVal = `${typeof hue === 'object' ? JSON.stringify(hue) : hue}`;
 
   if (shade !== undefined) {
     retVal += `,${shade}`;
   }
 
   if (theme !== undefined) {
-    let themeColorsKey = CACHE.get(theme.colors);
+    let themeColorsKey;
 
-    if (themeColorsKey === undefined) {
-      themeColorsKey = ++KEYS.colors;
-      CACHE.set(theme.colors, themeColorsKey);
+    if (theme.colors) {
+      themeColorsKey = CACHE.get(theme.colors);
+
+      if (themeColorsKey === undefined) {
+        themeColorsKey = ++KEYS.colors;
+        CACHE.set(theme.colors, themeColorsKey);
+      }
     }
 
-    let themePaletteKey = CACHE.get(theme.palette);
+    let themePaletteKey;
 
-    if (themePaletteKey === undefined) {
-      themePaletteKey = ++KEYS.palette;
-      CACHE.set(theme.palette, themePaletteKey);
+    if (theme.palette) {
+      themePaletteKey = CACHE.get(theme.palette);
+
+      if (themePaletteKey === undefined) {
+        themePaletteKey = ++KEYS.palette;
+        CACHE.set(theme.palette, themePaletteKey);
+      }
     }
 
     retVal += `,{${themeColorsKey},${themePaletteKey}}`;