Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

theme-ui creates 2 class names in storybook when using sx prop #1530

Closed
webcore-it opened this issue Feb 27, 2021 · 9 comments
Closed

theme-ui creates 2 class names in storybook when using sx prop #1530

webcore-it opened this issue Feb 27, 2021 · 9 comments

Comments

@webcore-it
Copy link

webcore-it commented Feb 27, 2021

Describe the bug
I have an react app. Starting with v0.5.0, theme-ui is creating 2 classes when rendered in storybook for all components whenever the sx prop is used. (HTML tags with sx have only one class). One class contains the style of the component, the other class the style from the sx prop.

When using react-scripts to render the app, only one class is generated and all css properties are merged in that one class.

There is another issue with storybook: Not all sx/theme property values are resolved, so the css class(es) contains something like background-color: primary;. (That's is not the case when the app is rendered with react-scripts.)

To Reproduce
Steps to reproduce the behavior:

  1. Clone https://github.com/webcore-it/theme-ui-storybook
  2. run yarn
  3. run yarn dev to start storybook
  4. Inspect any of the elements to see the generated classes

To check react-scripts:

  1. run yarn start to start with react-scripts
  2. Inspect any of the elements to see the generated class

Expected behavior
Only one class is generated when using storybook and all theme styles are applied.

Screenshots
2 classes when using storybook:
Screenshot 2021-02-27 at 15 02 30

1 class when using react-scripts:
Screenshot 2021-02-27 at 15 03 10

Not resolved theme values:
Screenshot 2021-02-27 at 15 03 41

@hasparus
Copy link
Member

hasparus commented Mar 1, 2021

Hey @webcore-it! Thanks for the issue.

This is a common problem — unfortunately, it's not a bug in our code per se, it's more of a design issue.

Let me explain what's happening:

  • Storybook depends Emotion 10.
  • Theme UI newer than 0.5.0 depends Emotion 11.

We're passing the theme to one context, and read it from the other.
The fact that we "hide" Emotion from the user, at least until problems like these happen, means Theme UI is a leaky abstraction.

I see a few possibilities of what we can do about it

  • detecting two Emotion versions, and logging a nice error message describing that stuff like this will happen
  • or moving Emotion to peerDeps and washing our hands of it

Neither is a satisfactory solution.

cc @lachlanjc what do you think? I added a new label to mark issues about this problem. I'm afraid this is going to be at least 1/4 of all our bug reports, maybe even more.

@lachlanjc
Copy link
Member

Oof. What's the downside of moving Emotion to peerDeps & warn if folks install a version we're not compatible with?

@webcore-it
Copy link
Author

Thanks for your response. I looked at the Storybook issue board and there are a few tickets with requests to update the dependencies there to emotion 11.

As a workaround if found this comment in a merge request:
storybookjs/storybook#13300 (comment)

@bebraw Thanks, this got me most of the way there. I also had to do the same to managerWebpack as that contains aliases referencing the v10 packages and would give me runtime errors without it. This only became apparent to me when I cleared node_modules/.cache/storybook.

Also found v10 references in the default babel config which I needed to replace in order to get the CSS prop to work at runtime.

Finally I also needed to install @emotion/styled as a dev dependency to ensure I had v11 in node_modules and not the v10 that gets installed for Storybook.

In the end this setup is now working for me (Emotion 11, Storybook 6.1.18, Yarn Workspaces).

.storybook/main.js

// Location of root node_modules
const modulesDir = path.join(__dirname, '../../../../node_modules');

const updateEmotionAliases = (config) => ({
  ...config,
  resolve: {
    ...config.resolve,
    alias: {
      ...config.resolve.alias,
      '@emotion/core': path.join(modulesDir, '@emotion/react'),
      '@emotion/styled': path.join(modulesDir, '@emotion/styled'),
      '@emotion/styled-base': path.join(modulesDir, '@emotion/styled'),
      'emotion-theming': path.join(modulesDir, '@emotion/react'),
    },
  },
});

module.exports = {
  // ...
  managerWebpack: updateEmotionAliases,
  webpackFinal: updateEmotionAliases,
  babel: (config) => {
    const getEntryIndexByName = (type, name) => {
      return config[type].findIndex((entry) => {
        const entryName = Array.isArray(entry) ? entry[0] : entry;
        return entryName.includes(name);
      });
    };

    // Replace reference to v10 of the Babel plugin to v11.
    const emotionPluginIndex = getEntryIndexByName('plugins', 'babel-plugin-emotion');
    config.plugins[emotionPluginIndex] = require.resolve('@emotion/babel-plugin');

    // Storybook's Babel config is already configured to use the new JSX runtime.
    // We just need to point it to Emotion's version.
    // https://emotion.sh/docs/css-prop#babel-preset
    const presetReactIndex = getEntryIndexByName('presets', '@babel/preset-react');
    config.presets[presetReactIndex][1].importSource = '@emotion/react';

    return config;
  },
  // ...
};

This also worked in my example repo. Now there is one css class and the components are rendered as expected. (Here the only change to .storybook/main.js: webcore-it/theme-ui-storybook@62465d1)

I've applied this workaround to the actual codebase (yarn monorepo + multiple Gatsby apps) as well, and it worked on the first look.

Feel free to close this issue.

@hasparus
Copy link
Member

hasparus commented Mar 3, 2021

Thanks for the workaround @webcore-it!

Oof. What's the downside of moving Emotion to peerDeps & warn if folks install a version we're not compatible with?

More friction to install peer deps? I tried hard to think of this one, and I don't see any more.
Let's do it. Emotion-context related issues pop up every week or two, and this problem is super annoying, because you install a library, copy some sample code and bam it doesn't work in your project.

@hasparus hasparus closed this as completed Mar 3, 2021
@hasparus hasparus reopened this Mar 3, 2021
@hasparus
Copy link
Member

hasparus commented Mar 3, 2021

Let's keep this issue open and close it with a PR moving Emotion to peerDeps (and updating the docs).

ethanwu10 added a commit to redpwn/rctf that referenced this issue Mar 28, 2021
Storybook uses emotion v10 while theme-ui uses v11, causing conflicts
(see system-ui/theme-ui#1530). Work around this by aliasing emotion
packages to the v11 equivalents inside storybook's manager.
ethanwu10 added a commit to redpwn/rctf that referenced this issue Mar 28, 2021
Storybook uses emotion v10 while theme-ui uses v11, causing conflicts
(see system-ui/theme-ui#1530). Work around this by aliasing emotion
packages to the v11 equivalents inside storybook's manager.
@mryechkin
Copy link

@webcore-it thanks for the solution and the background info! Those exact steps helped solve the issue in our Storybook project (running version 6.2.9)

Was looking to try out theme-ui in our existing component library implementation, and couldn't even get it to run - but this solved it 🎉

@rafaelrinaldi
Copy link

rafaelrinaldi commented Oct 20, 2021

FYI I was able to fix that by using pnpm's overrides feature. This is what I have added to my package.json:

{
  "pnpm": {
    "overrides": {
      "@emotion/core": "npm:@emotion/react",
      "@emotion/styled": "npm:@emotion/styled",
      "@emotion/styled-base": "npm:@emotion/styled",
      "emotion-theming": "npm:@emotion/react"
    }
  }
}

Note that I must run Storybook via pnpm in order for this to work:

pnpm run storybook

@iChip
Copy link

iChip commented Nov 5, 2021

For the yarn users out there - my less configuration-related workaround was:

"resolutions": {
"@emotion/react": "^11.4.1"
},

@lachlanjc
Copy link
Member

Our switch to having Emotion as a peer dependency should resolve the majority of cases of this! Please flag if you're still running into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants