-
Notifications
You must be signed in to change notification settings - Fork 218
chore(storybook): minimal upgrade migration #5309
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
|
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
b361a04
to
6004f3f
Compare
icon: html` | ||
<sp-icon-edit hidden slot="icon"></sp-icon-edit> | ||
`, | ||
icon: `<sp-icon-edit hidden slot="icon"></sp-icon-edit>`, |
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 version is much stricter about safe template rendering. It's considered unsafe to call a template result via function inside another template result. This is the way to accomplish that same result.
}, | ||
]; | ||
|
||
export const tags = ['autodocs']; |
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.
can NOT wait for this to go live <3
@@ -277,32 +278,3 @@ export const configuredVisualRegressionPlugin = () => | |||
); | |||
}, | |||
}); | |||
|
|||
export function watchSWC() { |
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.
Do you know what this was doing or why its safe to remove this now?
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.
Yes this was forcing rebuild of the assets when updates were made to the source files. Storybook handles this automatically and doesn't need us to manually kick this off any more. We can validate this by making changes to an existing asset and seeing it live refresh on the page. I'll add a test case for that when I start writing up validation steps.
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.
worked for me!
"@storybook/addon-designs": "^8.0.0", | ||
"@storybook/addon-essentials": "^7.5.0", | ||
"@storybook/addon-links": "^7.5.0", | ||
"@storybook/web-components": "^7.5.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.
looks like this package was removed but this is how we set our custom manifest (the part you commented out in preview.js). Was this intentional or is it not supported in storybook 8?
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.
Does look like it comes with v8 support: https://www.npmjs.com/package/@storybook/web-components
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 may say that it's supported but there's some serious issues in that package. When I add the dependency, it adds this to our install logs (yes, it does the same if we bump all packages to 8.6.12 too):

When we add it in anyway despite the logged error, it blocks styles from loading. When I commented out the CEM loading, the arg types still loaded in the stories and the styles rendered. We can debug loading the CEM another way but I'm not sure this package is going to be it.
d596606
to
b4fd473
Compare
d5bc7ea
to
1b26c82
Compare
1b26c82
to
0ae7e45
Compare
0ae7e45
to
73d269b
Compare
import { merge } from 'webpack-merge'; | ||
|
||
/** @type { import('@storybook/web-components-webpack5').StorybookConfig } */ | ||
export default { |
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.
A personal nit: how do you feel about including the disableWhatsNewNotifications: true
setting in the core
settings (like we do in Spectrum CSS)? Also: what if we set disableTelemetry: 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.
I'm definitely in support of that - would it work if I added this as a "fast follow" once this merges?
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.
Yeah, absolutely!
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 pulled down this branch and ran it successfully on my local environment. Everything looked good, functioned as expected, and was speedy!
I love that this enables the AutoDocs stuff!
I dropped one nit comment here that is just a personal preference, but let me know what you think.
Otherwise, I think this should be good to go.
0788b9b
to
f10e882
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.
LGTM - would love a fast follow to investigate the custom element manifest or automating the args table over the current implementation which is completely manual.
f10e882
to
f51747d
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.
As always, thanks for making our developer experience so much better!
@@ -277,32 +278,3 @@ export const configuredVisualRegressionPlugin = () => | |||
); | |||
}, | |||
}); | |||
|
|||
export function watchSWC() { |
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.
worked for me!
Description
This update migrations the SWC Storybook instance off of the
@web/storybook-builder
and@web/storybook-framework-web-components
and onto the latest breaking change (v8) release of@storybook/*
andstorybook
packages.This update does not:
Had to remove the
maintained node versions
entry to the browsers list config because it was causing the Storybook build to fail:Related issue(s)
Motivation and context
Blog post
How has this been tested?
yarn start
: expect no regressions to local StorybookScreenshots
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.