-
Notifications
You must be signed in to change notification settings - Fork 10
Upgrade Storybook to v9 #252
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 95.64% 95.87% +0.23%
==========================================
Files 14 14
Lines 390 388 -2
Branches 66 64 -2
==========================================
- Hits 373 372 -1
+ Misses 17 16 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
extends: ['plugin:@typescript-eslint/recommended', 'plugin:storybook/recommended'], | ||
plugins: ['import'], | ||
rules: { | ||
'@typescript-eslint/no-explicit-any': 'off', | ||
}, | ||
settings: { | ||
'import/resolver': { | ||
node: { | ||
extensions: ['.js', '.ts', '.tsx', '.mjs', '.d.ts'], | ||
paths: ['node_modules/', 'node_modules/@types/'], | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this (minus the plugin:storybook/recommended
which is the new storybook ESLint config) is to account for the fact that we're not using @storybook/eslint-config-storybook
anymore.
Internally @storybook/eslint-config-storybook
was doing all this and much more.
Same goes for the other 3 eslintrc files as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal but is there some way to share eslint configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good point. I think we should, since we had to update this in 4 places and the same applies whenever we upgrade to ESLint v9. I'll see if there's a simple way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmeasday we can do this with a shared ESLint configuration, but adding that would bloat this PR (new workspace package for the shared config, publish it to NPM, etc). Are you OK if we wait to tackle the config-reuse for whenever we upgrade to ESLint v9 (which has a new config file format)?
Hm, the locally-run (non-built) archived storybook (run by running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to include on the PR description that this also updates support in our archive-storybook
package to also support 9+ (is that correct)?
LGTM, but we might want to test a canary against projects using both SB8 and 9 (and maybe <8.6 following this thread?)
extends: ['plugin:@typescript-eslint/recommended', 'plugin:storybook/recommended'], | ||
plugins: ['import'], | ||
rules: { | ||
'@typescript-eslint/no-explicit-any': 'off', | ||
}, | ||
settings: { | ||
'import/resolver': { | ||
node: { | ||
extensions: ['.js', '.ts', '.tsx', '.mjs', '.d.ts'], | ||
paths: ['node_modules/', 'node_modules/@types/'], | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal but is there some way to share eslint configs?
Yippee, figured out the issue -- We were specifically targeting node 18 in our CI workflow, but Storybook 9 requires node 20 or above. Updating our CI scripts to reflect that. |
@tevanoff this is ready for your review! I'm assuming we don't need to bump the package versions since Storybook is an internal dependency, but happy to get your take on that. |
I'm realizing that we'd at least want to bump the package some version -- even though this is an internal change, it is a change nonetheless. I'll bump the minor version. |
@tevanoff this is ready for your re-review -- I'm holding off on merging until I've tested more permutations of Storybook 8 and 9 together, but I've confirmed that a typical Cypress+Playwright codebase that upgrades to (indirectly) use Storybook 9 works fine with builds and storybooks. |
aec08dc
to
f39350a
Compare
// reference the entry file based on the package.json `bin` value, since it changed between SB 8.1 and 8.2 | ||
return resolve(dirname(require.resolve('storybook/package.json')), packageJson.bin.storybook); | ||
return resolve( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to try a similar approach to this:
https://github.com/storybookjs/test-runner/blob/next/src/test-storybook.ts#L128
So that the resolution will start from the node_modules of the e2e package instead. If a user had something like this:
/node_modules
|_/storybook
|_/chromatic-e2e
|__/node_modules
|___/storybook
it will prefer the one from chromatic-e2e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yannbf we decided to forego putting e2e on Storybook 9 for the time being. I'll close this PR to reflect that.
…ook was doing for us
… was from Cypress
…e need it at runtime
…f addon-essentials which is now part of storybook, but we don't need the docs)
This also includes them as dev dependencies in the root project so that we can run our own E2E tests.
c8b6eaf
to
9754aae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tevanoff and @winkerVSbecks thanks for the progress!
@tevanoff my comments are "notes for us", let me know if you'd like me to resolve them or leave them to you.
Closing this as we decided, in the short term, to forego upgrading to Storybook 9 for the time being. We will revisit this when either a) we have a better handle on the level of effort to build our own custom Storybook framework, or b) Storybook 10 comes out. |
Issue: #AP-6003
What Changed
Storybook (which is used internally by the e2e packages) upgraded from
8.6.0
to9.0.0
.I ran the
yarn dlx storybook@next upgrade
command and accepted all the suggested migrations.Main thing that changed (manually) was getting rid of
@storybook/eslint-config-storybook
and using the recommendedeslint-plugin-storybook
. This took some copy-pasta of configs from that removed package'seslintrc.js
in order to get the same configuration that we had before.Also changed Node.js version used in CI workflows -- Storybook 9 doesn't support Node 18, so it's upgraded to pull the latest (cached) LTS version of node so we don't have to keep updating the files every year or two.
How to test
Playwright
@chromatic-com/playwright
packagearchive-storybook
Cypress
@chromatic-com/cypress
package