Storybook: Opt in to story store v7 #42486
Conversation
stokesman
added
[Type] Enhancement
|
Size Change: +7.09 kB (+1%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
storybook/main.js
Outdated
| '../packages/components/src/**/stories/*.@(js|tsx|mdx)', | ||
| '../packages/components/src/**/stories/index.@(js|tsx|mdx)', |
There was a problem hiding this comment.
Without this change the build seems to work fine but produces warnings about various files not having a default export. This change to the more specific pattern seemed the laziest way to avoid those warnings and I don't think it excludes anything. I'm open to any suggestions about doing this a different way. The two folders with files that produce warnings are:
There was a problem hiding this comment.
And also ToolsPanel.
It kind of seems that ideally this pattern should be handled in Storybook itself (cf. storybookjs/storybook#16598). But otherwise, it's probably either the index way, or moving sub-stories into a subfolder way. Someone may be confused when a story file doesn't get picked up as expected, but I don't see that happening very often. I think we can go with index for now and revisit if it causes issues.
There was a problem hiding this comment.
I think we can go with index for now and revisit if it causes issues.
Sounds like a good plan
There was a problem hiding this comment.
I checked for any directories named "stories" that are without index files and found five:
./packages/components/src/ui/context/stories,
./packages/components/src/date-time/stories,
./packages/components/src/duotone-picker/stories,
./packages/components/src/navigable-container/stories,
./packages/components/src/utils/hooks/stories'
So I think we'd need to add an index file or else they'd be overlooked by this change.
There was a problem hiding this comment.
The "no default export" errors are not appearing anymore for some reason 🤔 So I reverted the glob change for now.
If it reappears again, I think we could move to an approach where the glob is just for the root-level files in stories/, and then move the non-story files into a stories/utils/ subfolder or something.
There was a problem hiding this comment.
My bad, I forgot those errors were emitted at build-time, not at runtime (I was checking in devtools console) 🙈 I did end up moving the files to utils/ subfolders.
There was a problem hiding this comment.
Wow, the load time improvement is very perceivable! Thanks for taking the initiative on this one. It was on my mind as well, after seeing #42377 😆
It looks like we're going to need a bit more testing of intentional/unintentional globals, so we don't get unexpected behavior. I noticed that a global font-family is not being loaded anymore (e.g. see AnglePickerControl), which is likely due to #42414 not being loaded on initial load.
My inclination at this point is for font-family to be set as a global rule, instead of in each component. However, box-sizing resets will probably need to be added on a per-component basis (#42415).
For this PR though, I guess an intermediary step could be to move the global font-family, font-size, and box-sizing reset to a proper global style instead of the accidental leaky file.
storybook/main.js
Outdated
| '../packages/components/src/**/stories/*.@(js|tsx|mdx)', | ||
| '../packages/components/src/**/stories/index.@(js|tsx|mdx)', |
There was a problem hiding this comment.
And also ToolsPanel.
It kind of seems that ideally this pattern should be handled in Storybook itself (cf. storybookjs/storybook#16598). But otherwise, it's probably either the index way, or moving sub-stories into a subfolder way. Someone may be confused when a story file doesn't get picked up as expected, but I don't see that happening very often. I think we can go with index for now and revisit if it causes issues.
|
Thank you for working on this, @stokesman !
Since I believe the intention is for all components to use the same
Regarding
Agreed. We should look at the leaky styles and see which ones would potentially need to be replicated temporarily in this PR — |
My concern is that this would add unnecessary lines of code and CSS specificity, since |
Makes sense |
|
The global CSS issues will be handled in #42747. |
stokesman
force-pushed
the
update/story-store-v7
branch
from
5a6abae to
861c3a3
Compare
|
@stokesman I would really like this PR to land 😊 I don't think there are any blockers anymore, and it seems to be working ok after a rebase. Are you available to land it, or would you rather I take over? |
Me too! And I'd really like if you'd like to take over. I left #42486 (comment) about what I think is the only remaining todo here. Thanks for checking in and for the great work you did that removed the blockers for this! |
|
Gotcha, I'll take over then. Thank you! |
mirka
force-pushed
the
update/story-store-v7
branch
from
861c3a3 to
3911104
Compare
| "src/**/stories/**/*.js", // only exclude js files, tsx files should be checked | ||
| "src/**/test/**/*.js", // only exclude js files, ts{x} files should be checked |
|
Ok, should be good now. If anyone could do a quick smoke test and approve this PR, we can merge. tl;dr for those watching: Basically the load time should be perceivably faster compared to before. (That means LCP, not the build time unfortunately 😅) |
There was a problem hiding this comment.
Works well! (and loads fast!) 🚀
Thank you @stokesman and @mirka for working on this 🙏
What?
—https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#story-store-v7
Why?
How?
Making the required configuration changes. I'm not sure if I'm missing anything else though.
Testing Instructions