-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
storybook webpack builder to use swc instead of babel #22075
Conversation
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Pull request alert summary
📊 Modified Dependency Overview:
|
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 is fun! But I wonder if we can figure out how to split out the deps 🤔
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 like the idea, but I'm not a big fan of the dependency approach. Although I only have bad solutions:
- Current approach: include both babel and swc and make 100 % of Webpack users download more dependencies than needed.
- Make babel and swc both optional peer deps, forcing 100 % of Webpack users to install 2+ dependencies when including Storybook in their setup
- include babel by default, swc as peer, combining both problems above, but only for swc users
- opposite of 3.
For info:
@babel/core
+ babel-loader
: 1.3 MB
@swc/core
+ swc-loader
: 21kB
Given the data above, it would make sense to prefer option 4. However I don't think swc is ready to be a default (maybe?), so I'd personally go with option 3. option 1 is also not THAT bad given the small size of swc.
loader: require.resolve('swc-loader'), | ||
options: { | ||
...config, | ||
...options, |
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 way to merge the options will haunt us later. If options specify jsc
, they will completely override the default jsc
options from the config above. We need to merge them more intelligently. Or not support custom options at all.
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.
Unless we explicitly choose to not let users do that.
Similar to babel config, it should actually really come from swc's config file.
Or not support custom options at all.
Yes that exactly. I'm leaning towards that.
But considering 1 or the biggest use-cases of this feature will be "I don't know how to setup babel at all in my monorepo" + "storybook is so slow (due to babel being used)", having a tiny default setup seems sane.
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.
So I think the solution here is to remove the possibility for createSWCLoader
to take in any options at all.
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.
done
I COULD turn this into an addon, that users need to install and add in their I can't remove the babel-loader from the builder though, that would be a breaking change. |
I propose we go with this approach, (7.1.0) unless someone feels strongly we should turn this into an optionally installed addon the users needs to choose to install @storybookjs/core ? |
This was recently released FYI |
I think this is a good solution, given that SWC is so small I'm okay with adding it to core. |
@storybookjs/core Is anyone blocking this from moving forwards? |
@jonniebigodes Shall we have a meeting this week to discuss where to place documentation for this? |
@ndelangen I would say resolving this |
I've spoken to @jonniebigodes about the required changes to the documentation if this were to land. |
Some people don't want to use babel.. this adds an builder option for
builder-webpack
to switch it to useswc-loader