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

feat(storybook): upgrade to v7 #8342

Merged
merged 11 commits into from
May 24, 2023
Merged

feat(storybook): upgrade to v7 #8342

merged 11 commits into from
May 24, 2023

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented May 17, 2023

I got Storybook v7 to start with these changes. To test:

  • clone this PR; yarn install, yarn build, etc
  • in a Redwood app, add storybook to the web side (this won't be necessary when this PR is merged and @redwoodjs/testing is published with these changes):
    cd web
    yarn add -D storybook
    
  • in the Redwood app, yarn rwfw project:sync, yarn rw sb (make sure you have stories)
image

Working notes:

The following aren't necessary for this PR, but are just points to explore for the future:

  • look into using ESM in storybook/main.js https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#esm-format-in-mainjs
  • so far yarn rw storybook on v7 seems slower
  • for some reason Storybook's main config file, main.js, is named differently in user projects (it's named storybook.config.js). we should consider renaming it
  • testing provides @storybook/addon-docs, but we don't use it. maybe we just consider it important enough to include by default but I'd like to know for sure
  • audit webpackFinal
  • look into making a framework package
  • swap playwright tests for storybook test runner?

@jtoar jtoar added the release:breaking This PR is a breaking change label May 17, 2023
builder: 'webpack5',
framework: {
name: '@storybook/react-webpack5',
options: {},
Copy link
Contributor Author

@jtoar jtoar May 17, 2023

Choose a reason for hiding this comment

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

This empty object is actually necessary:

image

@replay-io
Copy link

replay-io bot commented May 17, 2023

@jtoar jtoar force-pushed the ds-storybook/v7 branch from b689d1c to 87f58f4 Compare May 19, 2023 23:30
Comment on lines -32 to -41
// Storybook's UI uses a separate Webpack configuration
managerWebpack: (sbConfig) => {
const userManagerPath = fs.existsSync(rwjsPaths.web.storybookManagerConfig)
? rwjsPaths.web.storybookManagerConfig
: './manager.example.js'
sbConfig.resolve.alias['~__REDWOOD__USER_STORYBOOK_MANAGER_CONFIG'] =
userManagerPath

return sbConfig
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ability to configure the manager is gone and is the most obvious breaking change

Comment on lines +38 to +41
// Only set staticDirs when running Storybook process; will fail if set for SB --build
...(process.env.NODE_ENV !== 'production' && {
staticDirs: [path.join(redwoodProjectPaths.web.base, 'public')],
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this up because it gets lost under webpackFinal

@@ -27,6 +27,7 @@
"postcss": "^8.4.23",
"postcss-loader": "^7.3.0",
"prettier-plugin-tailwindcss": "^0.2.8",
"storybook": "7.0.12",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need this here to get tests to run correctly; we can remove it after a canary gets published

@jtoar jtoar marked this pull request as ready for review May 20, 2023 09:14
Comment on lines +6 to +10
timeout: 90_000 * 3,
expect: {
timeout: 10 * 1000,
},
workers: 1, // do not run things in parallel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could experiment with these settings more, but so far I had to increase the timeout and borrow the strategy used in the Vite PR to get the smoke tests to pass

Copy link
Contributor Author

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Got the go ahead from @thedavidprice; merging so we can work on the next steps

@jtoar jtoar merged commit 5f03da5 into main May 24, 2023
@jtoar jtoar deleted the ds-storybook/v7 branch May 24, 2023 01:05
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone May 24, 2023
@thedavidprice
Copy link
Contributor

Great to see this one in 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants