-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Rspack #22807
Rspack #22807
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
I am getting these times for
And these for
Live recompile is slower with |
Co-authored-by: Petar Petrov <[email protected]>
The difference in bundle size with RegExp |
@@ -168,10 +170,14 @@ const createWebpackConfig = ({ | |||
stats: { assets: true, chunks: true, modules: true }, | |||
transform: (stats) => JSON.stringify(filterStats(stats)), | |||
}), | |||
!latestBuild && | |||
new TransformAsyncModulesPlugin({ |
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 rspack
properly transpile TLA for the legacy build? I don't see anything about target support in their 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.
AFAICT, rspack
has the same problem that webpack
does. There are now a bunch of async functions in the legacy build that will cause errors on older browsers.
I'm not sure why this plugin was removed, but it should probably be added back.
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.
... or #23234 and then the transform isn't needed.
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 wasn't supported by rspack, I removed it and tested it with browser stack if it is necessary. Maybe I did a mistake here, will check again or your solution solves it 🙂
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 worries... you probably just didn't test anything that was old enough to not support async functions. It's moot now that #23234 is merged.
I wrote that plugin so I'll have to see if I can quickly make it compatible with rspack
for others.
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.
Webpackbar did a rspack update, maybe you can take this as inspiration:
@@ -102,7 +102,7 @@ const createRspackConfig = ({ | |||
chunks: !isProdBuild | |||
? // improve incremental build speed, but blows up bundle size | |||
new RegExp( | |||
`^(?!.*(${Object.keys(entry).join("|")}|work(?:er|let))$)` | |||
`^(?!(${Object.keys(entry).join("|")}|.*work(?:er|let))$)` |
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.
Why are we treating production and development builds differently? Looks like the regex is trying to accomplish the same thing for the modern build, but will give a different result for the legacy build? 😕
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.
We had troubles with the incremental build speed, so we found this solution but it fucks up all our chunks so for prod we still use the js lambda. Yeah you are right, maybe we should also disable this for legacyBuild
Breaking change
Proposed change
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: