-
Notifications
You must be signed in to change notification settings - Fork 6
fix(security): update storybook to 8 #400
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
✅ Deploy Preview for cfpb-design-system-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
billhimmelsbach
left a comment
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.
Added some comments here to make these storybook update changes a little easier to review.
| options: {} | ||
| }, | ||
| features: { | ||
| storyStoreV7: 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.
storyStoreV7 not supported by v8 of storybook, and is no longer needed.
| @@ -1,26 +1,30 @@ | |||
| import turbosnap from 'vite-plugin-turbosnap'; | |||
|
|
|||
| module.exports = { | |||
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.
Most of the changes in this file were done by the migration script.
| @@ -0,0 +1 @@ | |||
| !.storybook No newline at end of file | |||
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.
Added for the eslint storybook configuration.
| ] | ||
| } | ||
| }, | ||
| actions: { argTypesRegex: '^on[A-Z].*' }, |
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.
We didn't use the ability to implicitly add jest spies and its no longer supported in v8, and I ran our stories through the suggested script yarn dlx storybook migrate find-implicit-spies against the codebase to make sure.
| @@ -6,7 +6,7 @@ enableHardenedMode: true | |||
|
|
|||
| enableMirror: true | |||
|
|
|||
| enableOfflineMode: true | |||
| enableOfflineMode: false | |||
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 yarn feature caused me all kinds of problems when trying to get this working, so I don't think it's worth it. A real footgun here.
| @@ -1,4 +1,4 @@ | |||
| import { useArgs } from '@storybook/client-api'; | |||
| import { useArgs } from '@storybook/preview-api'; | |||
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.
@storybook/client-api has changed to @storybook/preview-api, so went through and changed these after the calls failed.
| @@ -1,6 +1,6 @@ | |||
| import { expect } from '@storybook/jest'; | |||
| import { expect } from '@storybook/test'; | |||
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.
@storybook/jest has been removed and replaced by @storybook/test, so went through and changed these after the calls failed.
| render: (arguments_) => <div style={{ minHeight: '200px' }}> | ||
| <Select {...arguments_} /> | ||
| </div> | ||
| render: arguments_ => ( |
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.
Linting.
| @@ -53,7 +53,7 @@ describe('<SelectMulti />', () => { | |||
| const label = 'MultiLabel'; | |||
| const maxSelections = 2; | |||
| const user = userEvent.setup(); | |||
| const onChange = jest.fn(); | |||
| const onChange = fn(); | |||
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.
Replacing these with the @storybook/test method of mocking out these functions.
|
We'll have to wait to review or merge this @ojbravo until this Netlify contract issue is renewed / resolved on April 15th. |
ojbravo
left a comment
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.
It's working!
Changes
How to test this PR
Screenshots
Closes: #339