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

Upgrade storybook-builder to Storybook 8 #2674

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jpzwarte
Copy link
Contributor

What I did

  1. I upgraded the storybook dependencies from 7 to 8
  2. I did npm I and npm run build and there were no errors

What I havent' done (yet)

  1. Test it

Copy link

changeset-bot bot commented Mar 16, 2024

🦋 Changeset detected

Latest commit: bc1b090

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@bashmish
Copy link
Member

bashmish commented Mar 17, 2024

Hey, good you started this PR :D

I have worked on the test suite, but was unable to make it work in the build due to all kinds of reasons which I don;t get locally, the last one being that binaries like wds or http-server can't be found when NPM command is executed in the CI

if you wanna help with that, take a look here #2485
however I'm gonna work tonight on this and really hope to crack it down, so then we will be able to run a good test suite checking if storybook renders and main addons work, so I'll need to also write a few more tests

@bashmish
Copy link
Member

Just merged the tests PR #2485
Can you please check with latest changes from it?

@jpzwarte
Copy link
Contributor Author

Just merged the tests PR #2485 Can you please check with latest changes from it?

=> Failed to build the preview
Error [PLUGIN_ERROR]: Package subpath './dist/react-18' is not defined by "exports" in /Users/jzwartepoorte/Projects/web/node_modules/@storybook/react-dom-shim/package.json
    at exportsNotFound (node:internal/modules/esm/resolve:303:10)
    at packageExportsResolve (node:internal/modules/esm/resolve:650:9)
    at resolveExports (node:internal/modules/cjs/loader:591:36)
    at Module._findPath (node:internal/modules/cjs/loader:668:31)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1130:27)
    at Module._resolveFilename (/Users/jzwartepoorte/Projects/web/node_modules/esbuild-register/dist/node.js:4799:36)
    at Function.resolve (node:internal/modules/helpers:187:19)
    at getReactDomShimAlias (/Users/jzwartepoorte/Projects/web/packages/storybook-builder/dist/rollup-plugin-prebundle-modules.js:101:11)
    at Object.buildStart (/Users/jzwartepoorte/Projects/web/packages/storybook-builder/dist/rollup-plugin-prebundle-modules.js:51:50)

But I believe this is explainable: in storybook 8, you no longer have to add react and react-dom as dependencies to a project that's using storybook. See the "No more React requirement in non-React projects" section in https://storybook.js.org/blog/storybook-8/

@bashmish
Copy link
Member

@jpzwarte this requires a fix in the builder. Also looking at the blog post, I think @storybook/test requires changes too.
Because of those and also potential changes in other deps, all CANDIDATES in https://github.com/modernweb-dev/web/blob/%40web/storybook-builder%400.1.8/packages/storybook-builder/src/rollup-plugin-prebundle-modules.ts#L69 need to be checked and updated, not just new ones need to be added if the storybook breaks to render or build, but old ones need to be removed too.

Will you be willing to work on that?

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

2 participants