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

storybook webpack builder to use swc instead of babel #22075

Merged
merged 16 commits into from
May 23, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Apr 13, 2023

Some people don't want to use babel.. this adds an builder option for builder-webpack to switch it to use swc-loader

@socket-security
Copy link

socket-security bot commented Apr 13, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
[email protected] None +0 kdy1

Copy link
Member

@tmeasday tmeasday left a 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 🤔 :hurtrealbad:

Copy link
Contributor

@JReinhold JReinhold left a 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:

  1. Current approach: include both babel and swc and make 100 % of Webpack users download more dependencies than needed.
  2. Make babel and swc both optional peer deps, forcing 100 % of Webpack users to install 2+ dependencies when including Storybook in their setup
  3. include babel by default, swc as peer, combining both problems above, but only for swc users
  4. 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,
Copy link
Contributor

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.

Copy link
Member Author

@ndelangen ndelangen Apr 13, 2023

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.

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@ndelangen
Copy link
Member Author

I COULD turn this into an addon, that users need to install and add in their main.ts.
The addon would remove the babel-loader and adds the swc loader.

I can't remove the babel-loader from the builder though, that would be a breaking change.

@ndelangen
Copy link
Member Author

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 ?

@yannbf
Copy link
Member

yannbf commented Apr 21, 2023

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
https://github.com/radist2s/storybook-addon-swc

@JReinhold
Copy link
Contributor

I think this is a good solution, given that SWC is so small I'm okay with adding it to core.

@ndelangen
Copy link
Member Author

@storybookjs/core Is anyone blocking this from moving forwards?

@ndelangen
Copy link
Member Author

@jonniebigodes Shall we have a meeting this week to discuss where to place documentation for this?

@ndelangen ndelangen added the ci:daily Run the CI jobs that normally run in the daily job. label May 17, 2023
@JReinhold
Copy link
Contributor

JReinhold commented May 18, 2023

Is anyone blocking this from moving forwards?

@ndelangen I would say resolving this

#22075 (comment)

@ndelangen ndelangen requested review from JReinhold and tmeasday May 19, 2023 07:11
@ndelangen
Copy link
Member Author

I've spoken to @jonniebigodes about the required changes to the documentation if this were to land.

@ndelangen ndelangen merged commit 0010915 into next May 23, 2023
@ndelangen ndelangen deleted the norbert/other-secret-project branch May 23, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder-webpack5 ci:daily Run the CI jobs that normally run in the daily job. maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants