-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Addon-docs: Replace source-loader with csf-plugin #19680
Conversation
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 looks good!
I'm not worried about the breaking change since it's fairly easy for users to migrate.
code/lib/csf-tools/src/enrichCsf.ts
Outdated
const addParameter = template(` | ||
%%key%%.parameters = { storySource: { source: %%source%% }, ...%%key%%.parameters }; | ||
`)({ | ||
key: t.identifier(key), | ||
source: t.stringLiteral(source), | ||
}) as t.Statement; |
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.
Without knowing too much about what goes on behind the scenes, on the surface this looks like something that could be handled with a simple template literal instead of requiring babel/template?
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.
No, this needs to be babel AST nodes. The other option is to do it without @babel/template
is to construct the AST by hand, which is more verbose and not very human-readable. I was thinking of doing it as an optimization, and will create a telescoping PR to see if we like that better.
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.
See changes in #19684
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 I see those changes. I don't know how much faster it is, but the raw AST is definitely less readable that template
.
It might look okay now, but coming back to it in a year to do some modifications sounds like a nightmare.
Co-authored-by: Tom Coleman <[email protected]>
… into shilman/csf-plugin
@@ -62,7 +66,7 @@ export async function webpack( | |||
babelOptions, | |||
mdxBabelOptions, | |||
configureJSX = true, | |||
sourceLoaderOptions = { injectStoryParameters: 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.
Would it make sense to still accept this property with the purpose of warning users/throwing an error and directing them to the migration docs?
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.
Great idea! 💯
So that it runs on code that still contains comments
…description CSF-tools: Turn story comments into docs descriptions
], | ||
"scripts": { | ||
"check": "../../../scripts/node_modules/.bin/tsc --noEmit", | ||
"prep": "node ../../../scripts/prepare.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.
@shilman was there a reason not to use tsup for bundling this?
@@ -32,6 +33,10 @@ const config: StorybookConfig = { | |||
core: { | |||
disableTelemetry: true, | |||
}, | |||
viteFinal: (vite) => ({ | |||
...vite, | |||
plugins: [...(vite.plugins || []), csfPlugin({})], |
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.
@shilman coming back to this.
Why are we explicitly adding this to our UI SB instead of adding it per default through builder-vite
?
Issue: N/A
csf-plugin
is asource-loader
replacement that is dramatically simpler & easier to maintain.It also supports both Webpack & Vite, providing a proof of concept for how we might do cross-builder addons
in the future.
In addition to the maintenance implications,
addon-storysource
andaddon-docs
both usedsource-loader
, but in conflicting ways. So to use both at the same time, users needed to jump through configuration hoops.What I did
csf-tools
to provide basic static source extractioncsf-plugin
that provides that extraction for both Weback & Viteui
storybook to test ViteWhat I need
@joshwooding @IanVS let's figure out the best place to integrate it into Vite. Should
addon-docs
get aviteFinal
?How to test
Then navigate to the Controls stories & view source on the DocsPage