designTokensTable causes app crush

[yarinsa]

I am getting the following error:

SearchField.js:34 Uncaught TypeError: Cannot read properties of undefined (reading 'defaultText')
   at SearchField.js:34:32
   at handleInterpolation (index.js:3482:24)
   at serializeStyles3 (index.js:3597:15)
   at index.js:3805:24
   at index.js:2509:12
   at renderWithHooks (react-dom.development.js:14985:18)
   at updateForwardRef (react-dom.development.js:17044:20)
   at beginWork (react-dom.development.js:19098:16)

I traced back and it seems like its expecting me to have theme.color.defaultText (which is not even storybook theme interface).
Willing to help with the fix

[Sqrrl]

I'm having issues reproducing this. What Storybook version are you using?

[yarinsa]

6.5.12

[Sqrrl]

I ran into a similar issue recently. Seems to be related to storybookjs/storybook#7914

[YoungElPaso]

storybook-design-token/addon/src/components/TokenTable.tsx

Line 78 in 0691993

color: theme.color.defaultText,

Uses this theme property too, which seems to be the problem. This also does look like a valid Storybook theme property:

https://github.com/storybookjs/storybook/blob/42fae30325bb7635e38689e8cc9ece3740d49746/code/lib/theming/src/base.ts#L39

Maybe not having that set in a custom theme is the issue? Reading the docs on SB Theming (https://storybook.js.org/docs/react/configure/theming) it suggests that custom themes can use the default 'light' or 'dark' themes as a base, but neither of those necessarily include that property either and that when theming:

When setting a theme, set a complete theme object. The theme is replaced, not combined.

So, if this add on requires a value not provided in a custom theme, that could be source of the error.

I'll try to confirm this by adding that value to our theme and see what happens.

Update: nothing happens, this was a bit of a naive comment. Sorry. Definitely looks like having multiple versions of Emotion is the culprit IMO. I believe the use of MDX Doc Blocks adds a copy and blows away Theme from ThemeProvider, hence no property found.

If that's the case, I wonder if it's better if @storybook/theming is listed as a peer dependency? (It basically just wraps Emotion) - in which case it wouldn't be loaded a second time?

[YoungElPaso]

I tried to create a fork from Main but had trouble running build - got several errors. Unsure what the exact dev process is and can't find documentation on contributing for this addon.

Emotion maintainers recommend including it only as a peer dep to avoid the 'multiple copies of Emotion issue' - but in this case Emotion is bundled in @storybook/theming so I suppose that would need to be unbundled here as well?

Storybook docs aren't totally clear, but they seem to suggest that Storybook addons should include other addons as peer dependencies as well.

I'd be happy to try a PR but I'm not sure how to duplicate this weird edge case for testing.

I should note, broadly updating Storybook seems to fix this issue, but that might be incidental. I'll check the changelog for @storybook/theming to see if there's an insight there re how they import/bundle Emotion.

[Sqrrl]

I‘m busy right now making the addon compatible with SB 7. I‘ll try to fix the theming issues with the next release.

[Sqrrl]

I was able to reproduce the issue when using the Vite builder. It seems to work fine with Webpack. @yarinsa @YoungElPaso Which builder are you using?

[yarinsa]

@Sqrrl Vite

[YoungElPaso]

@Sqrrl webpack. But basically you were correct in your original referenced Storybook issue I think.

There's some Storybook addons that depend on @storybook/theming v6.15.5 and if you're not careful and use semver range (^) it's easy to upgrade to those, which at least my case, clashed (without error at npm install) with your addon's use of v6.15.13 (which wasn't upgraded).

So in my case, I had a package-lock.json file that included references to both versions, which conflict and is a well known issue with Emotion (the package that is wrapped by @storybook/theming) and therefore in the Storybook bundle I get the 'you have 2 copies of Emotion' warning- see screenshot below, and see this issue: emotion-js/emotion#2639).

Storybook itself uses the new version, which clobbers the version bundled in your addon. Thus no theme.color (or any theme) for your components.

I suspect people run into this issue when they have the Storybook dependencies listed in package.json with the ^ range and delete their lock file, and in doing some unrelated work run npm install which adds some other dependency but also updates those core storybook addons, thus provoking this clash.

For me, there's two possible solutions that both have worked so far, check these out @yarinsa :

1. Pin all storybook versions so they're not transitively updated when some other package is manually added to package.json and package-lock.json gets regenerated
2. Go back to previously working state, be careful to add new other packages only, making sure Storybook addons are not errantly updated, and run npm ls @storybook/theming to make sure only one version is present (the one that's compatible with this addon) (see screenshot for bad vs good npm ls results)

The second approach is probably the best as it will result in a cleaner diff and better history for the lockfile while allowing the use of semver ranges with the core Storybook addons.

That said, if you have time in the future, though this is basically an upstream issue, I wonder if making this addon only have @storybook/theme as a peer dep would work better to avoid this scenario? 🤷

---

Good npm ls @storybook/theming, you want all versions of that package to align, like this:

---

Bad npm ls @storybook/theming, note the versions don't match!

---

'Double' Emotion warning in console, with broken Token Docs:

---

My conclusion:

This is a pretty subtle bug and took a long time to figure out but basically AFAIK isn't an issue ultimately with this repo but with adhering more strictly to npm best practices and being careful about ranged dependencies. Since the fix requires no work here, thanks for your time and as far as I'm concerned @Sqrrl you could close this issue 👍