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

Support React 18 #320

Merged
merged 11 commits into from Apr 7, 2022
Merged

Support React 18 #320

merged 11 commits into from Apr 7, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Apr 7, 2022

An exciting PR has been merged and released in 6.5.0-alpha.58 that adds storybook support for React 18. However, it currently breaks in the vite-builder test because of an import() of the new react-dom/client, which only exists in react 18. Due to vitejs/vite#6007, we have to do a little bit of trickery to stop vite from erroring out. The approach I'm taking here is:

  1. Rewriting the import of react-dom/client to a dummy file if it can't be require.resolve()ed. This fixes the problem in production builds.
  2. Additionally, adding react-dom/client to the list of optimizeDeps.exclude if the framework is react and the package can't be required. This is necessary to prevent vite from trying to pre-bundle, and throwing errors in dev.

This PR also updates the examples to the new 6.5.0-alpha.58 version of storybook, which includes the PR mentioned above, so that it can be tested in our react examples, and it adds a new react-18 example as well.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing from phone but first glance LGTM!

@IanVS
Copy link
Member Author

IanVS commented Apr 7, 2022

@shilman I need to get to bed, but do you have any idea what this error might be about?

https://github.com/storybookjs/builder-vite/runs/5862295224?check_suite_focus=true#step:6:240

Failed to load preset: {"type":"presets","name":"/home/runner/work/builder-vite/builder-vite/node_modules/@storybook/vue3/dist/cjs/server/framework-preset-vue3.js"} on level 2
➤ YN0000: ERR! Error: Cannot find module 'webpack/lib/rules/DescriptionDataMatcherRulePlugin'
➤ YN0000: ERR! Require stack:
➤ YN0000: ERR! - /home/runner/work/builder-vite/builder-vite/node_modules/@storybook/vue3/node_modules/vue-loader/dist/pluginWebpack5.js

It looks like it started in alpha.53, FWIW.

@shilman
Copy link
Member

shilman commented Apr 7, 2022

@IanVS

I think what's happening is as follows:

  • @storybook/vue3's preset imports VueLoaderPlugin from vue-loader
  • vue-loader does some kind of version detection to figure out whether to use webpack4 or webpack5
  • vue3 (or something else in the vue3 ecosystem) upgraded to use webpack5 recently

I'm fuzzy on exactly what changed to trigger the move to webpack5, but it wasn't anything in storybook AFAIK. we fixed it on our side with this PR storybookjs/storybook#17908

Therefore, it is trying to require something from webpack5 but maybe Storybook's webpack4 is hoisted (this will go away in 7.0, but for now we cope).

Possible workarounds:

  • yarn resolutions to force webpack5
  • add a webpack5 dependency to that project
  • figure out which vue dep is messing things up and force it to a lower version

I'm not sure what implications this will have to real-world vue3 user projects that are using vite. In 7.0 we will deal with it properly by simply not having any webpack deps for vue users. But in the meantime it's messy.

Vite is trying to prebundle @storybook/react even in vue, because it's in our list of optimizeDeps.include.
We'll be able to remove it soon in another PR, when we can revisit this as well.
@IanVS
Copy link
Member Author

IanVS commented Apr 7, 2022

OK thanks @shilman. I determined that the change to allow webpack 5 in storybookjs/storybook#17834 did indeed start allowing the installation of webpack 5 (previously we only had webpack 4 installed). This, in turn, broke my old version of vue-loader, which was still sitting at 16.2.0. A fix for webpack 5 was released in 16.4.1, so I've opened storybookjs/storybook#17912 to avoid this problem in other projects, and for now set "resolutions" in this project.

@IanVS
Copy link
Member Author

IanVS commented Apr 7, 2022

Alright, finally got CI green, and I've got some ✅ , so let's gooooo.

@IanVS IanVS merged commit 52b6fdf into main Apr 7, 2022
@IanVS IanVS deleted the react-18 branch April 7, 2022 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants