-
-
Notifications
You must be signed in to change notification settings - Fork 53
feat: Support Storybook 10 #246
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
base: master
Are you sure you want to change the base?
Conversation
❌ Deploy Preview for storybook-addon-mock failed.
|
| @@ -1 +1 @@ | |||
| lts/jod | |||
| 22.19 | |||
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.
This is the new minimal Node version supported by SB10.
ac6b6a6 to
6d0d065
Compare
BREAKING CHANGE: No longer supports Storybook versions < 10
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "storybook-addon-mock", | |||
| "name": "storybook-addon-mock-root", | |||
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.
Having the same package name at the root of a yarn workspace and within one of the packages causes errors on more recent yarn versions.
| }, | ||
| }, | ||
| }, | ||
| import.meta.resolve('./local-preset.ts'), |
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.
New way of loading addons locally in SB10: you must import them. I've realigned local-preset on what we do on addon-kit.
| options: { | ||
| storySort: { | ||
| order: ['Docs', ['Introduction', 'Installation', 'User guide']], | ||
| order: ['Docs', ['Introduction', 'Installation', 'Advanced setup', 'User guide']], |
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.
Fixed up missing docs page in the sort order.
| "@storybook/addon-docs": "^9.0.12", | ||
| "@storybook/addon-links": "^9.0.12", | ||
| "@storybook/react-vite": "^9.0.12", | ||
| "@storybook/addon-docs": "next", |
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.
Recommended practice: point to the current development version in your addon workspace. Alternatively, you can set 10.0.0
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.
Historically, Storybook addons have had different build targets (node/browser), dts presence and externals based on whether files are consumed by the Storybook node server, manager UI, or preview environment.
Using Babel without dedicated build config for each environment prevents these optimisations from happening, and in some cases can cause addons to throw errors when your users build their own Storybook instance.
I've aligned your addon's build with our recommended tsup setup to make it easier for contributors to step in and update your addon, and to limit the risks associated with not distinguishing those three types of builds we consume.
| @@ -1,3 +1,6 @@ | |||
| # zx environment | |||
| scripts/prepublish-checks.js | |||
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.
Should not be linted, as it imports stuff like chalk that you don't have as a direct dependency and that's provided by zx
| "plugin:react/recommended", | ||
| "plugin:prettier/recommended" | ||
| ], | ||
| "root": true, |
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.
In recent ESLint versions, .eslintrc should not be used, and requires root: true for ESLint to avoid reading parent ESLint config sitting in e.g. your home folder.
I've left ESLint at version 8 to spare you having to switch to flat ESLint config immediately, and I've enabled this flag to avoid contributors accidentally merging their own eslintrc files when linting your code.
| "modules": true | ||
| } | ||
| }, | ||
| "settings": { |
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.
Avoids a runtime warning.
| import type { Config } from 'jest'; | ||
|
|
||
| export default { | ||
| extensionsToTreatAsEsm: ['.ts', '.jsx', '.tsx'], | ||
| testEnvironment: 'node', | ||
| transform: { | ||
| '^.+\\.(js|jsx|ts|tsx)$': 'ts-jest', | ||
| }, | ||
| } satisfies Config; |
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.
Basic config to ensure Jest can run ESM + TS files
| })); | ||
|
|
||
| export const ButtonToggle = ({ name, value, onChange, onBlur, onFocus }) => { | ||
| export const ButtonToggle = ({ name, value, onChange, onBlur, onFocus }: { |
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.
Jest really wanted your JSX to be written with TypeScript so I migrated those only (but not utils, I'd rather let you do that considering the amount of homemade classes you have).
| id={name} | ||
| type="checkbox" | ||
| onChange={(e) => onChange(e.target.checked)} | ||
| onChange={onChange ? (e) => onChange(e.target.checked) : undefined} |
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 don't always pass onChange
| margin-bottom: 0; | ||
| } | ||
| `; | ||
| const Content = styled.div<{ enabled: boolean }>(({ enabled }) => ({ |
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.
Ensure this is typed
| )} | ||
| </div> | ||
| </TabsState> | ||
| <Fieldset> |
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.
TabsState will be deprecated very soon, and permanently removed in Storybook 11. The way you used it would result in a WCAG violation.
I've replaced it with what I felt was semantically closer, though it does look different from what you had.
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.
| interface MockData { | ||
| url: string; | ||
| method: string; | ||
| status: string | number; | ||
| skip: boolean; | ||
| delay: number; | ||
| path: string; | ||
| searchParamKeys: string[]; | ||
| errors: string[], | ||
| originalRequest: unknown; | ||
| } |
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.
I've had to provide some basic typing here for useAddonState.
| if ( | ||
| module && | ||
| 'hot' in module && | ||
| module.hot && | ||
| typeof module.hot === 'object' && | ||
| 'decline' in module.hot && | ||
| module.hot.decline && | ||
| typeof module.hot.decline === 'function' | ||
| ) { | ||
| module.hot.decline(); | ||
| } |
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.
I'm not quite sure why this code exists. I've just added a few more type guards to make TS and ESLint happy.
The part below will be mandatory in Storybook 11 and is part of CSF Next type safety.
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.
I've moved preview and manager back to the top-level because this is where external contributors usually look for those files, and this is the scenario we describe in every addon migration guide.
I want to make it easier for folks like me to parse the addon's code and propose update PRs. By keeping file locations the same as in migration guides, you'll have more chances of getting contributors to offer PRs for you.
| : JSON.stringify(responseText); | ||
|
|
||
| return new Response(text, { | ||
| ok: ((status / 100) | 0) === 2, // 200-299 |
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.
This should be computed by the Response constructor itself.
| return new Response(text, { | ||
| ok: ((status / 100) | 0) === 2, // 200-299 | ||
| status: status, | ||
| statusText: statusTextMap[status.toString()], |
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.
Aligned with our own tsconfig on addon-kit
|
@Sidnioulz, would you mind splitting this PR into multiple small PRs? |
|
@nutboltu I don't think I'll have time for this in the coming weeks. I've been opening PRs on popular addons to get you folks started, but I now need to switch to other tasks. Feel free to cherry-pick anything you like and to commit it without preserving authorship information though. |


BREAKING CHANGE: No longer supports Storybook versions < 10
Closes #244