-
Notifications
You must be signed in to change notification settings - Fork 306
fix(perf): use swc for minify #1258
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
base: main
Are you sure you want to change the base?
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub ↗.
|
| @@ -1,2 +1 @@ | |||
| require('./sourcemap-register.js');(()=>{if(typeof __nccwpck_require__!=="undefined")__nccwpck_require__.ab=__dirname+"/";var e={};(()=>{var _=e;const o="qux";console.log?.("hello");_.foobar=o})();module.exports=e})(); | |||
| //# sourceMappingURL=index.js.map | |||
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 like a bug since the source map comment is missing
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.
Seems to be a limitation of swc as it is impossible to pass an object to the minify.sourceMap option.
Lines 485 to 489 in 36a7568
| sourceMap: map ? { | |
| content: map, | |
| filename, | |
| url: `${filename}.map` | |
| } : false |
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.
Should SWC write //# sourceMappingURL=index.js.map?
SWC has relevant code for sourceMap = inline but not for true. Does terser emit one?
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.
https://terser.org/docs/api-reference/#source-map-options so there was an option. I'll implement this option today or tomorrow and publish a new version of @swc/core
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.
swc-project/swc#10346 implements the option, and I'll publish a new version later today, after merging some PRs.
Sorry for the late response! The mention email was buried due to lots of other emails. (Mostly PR review requests)
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.
Sure. Lemme see if the tests passed here after bumping the swc to the latest version later today.
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.
https://github.com/swc-project/swc/actions/runs/14376924724 failed, so I'll fix it tomorrow and retry
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.
A new version is published! Can you try updating @swc/core?
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.
A new version is published! Can you try updating
@swc/core?
@kdy1 Sure! I will try it out later today.
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 think this is only worth doing if we also plan to replace swc for the Otherwise we include a massive dependency that we're not taking advantage of.
Also did you compare performance before/after? |
We can always do this gradually since #926 has been blocked and simply abandoned since (no one is investigating it anymore).
I use the integration test case ( Before: However integration test cases use many packages anyway, thus I assume the performance improvement is negligible on small projects with fewer dependencies. |
**Related issue:** - vercel/ncc#1258
| @@ -1 +1 @@ | |||
| {"version":3,"file":"index.js","names":["__webpack_require__","ab","__dirname","foobar","console","log","exports"],"sources":["../webpack/runtime/compat","../test/unit/minify-sourcemap-register/input.js"],"sourcesContent":["\nif (typeof __webpack_require__ !== 'undefined') __webpack_require__.ab = __dirname + \"/\";","const foobar = 'qux'\nconsole.log?.(\"hello\");\nexports.foobar = foobar\n"],"mappings":"MACA,UAAAA,sBAAA,YAAAA,oBAAAC,GAAAC,UAAA,I,uBCDA,MAAAC,EAAA,MACAC,QAAAC,MAAA,SACAC,EAAAH,Q","ignoreList":[]} | |||
| {"version":3,"file":"index.js","sources":["../webpack/runtime/compat",".././test/unit/minify-sourcemap-register/input.js"],"sourceRoot":"","sourcesContent":["\nif (typeof __webpack_require__ !== 'undefined') __webpack_require__.ab = __dirname + \"/\";","const foobar = 'qux'\nconsole.log?.(\"hello\");\nexports.foobar = foobar\n"],"names":[],"mappings":"MACA,GAAA,OAAA,sBAAA,YAAA,oBAAA,EAAA,CAAA,UAAA,2BCDA,MAAA,EAAA,MACA,QAAA,GAAA,GAAA,SACA,EAAA,MAAA,CAAA"} | |||
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.
@kdy1 Is it expected that names changed to an empty array?
Before:
["__webpack_require__","ab","__dirname","foobar","console","log","exports"]After:
[]That looks like a bug.
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 think it's swc-project/swc#6726
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's now fixed. Can you try updating @swc/cli?
Unlike #777 and #926 where swc is used to replace
ts-loader, in this PR swc is used to replaceterserin the modification phase.All CI passed: SukkaW#2