core(build): add js minification to inline-fs plugin #13272
Conversation
brendankenny
requested review from
connorjclark
and removed request for
a team
|
|
||
| // TODO(bckenny): minify inlined javascript. | ||
| // Minify inlined javascript. | ||
| if (constructedPath.endsWith('.js')) { |
There was a problem hiding this comment.
One notable thing is that our current pipeline (and now this plugin) spends a full 2 seconds on my machine trying to re-minify axe.min.js, which ends up saving about 1.9KB (out of 416KB for just that file), much of which ends up being from stripping the license header out :)
We could skip minifying when constructedPath.endsWith('.min.js'), but not sure what balance is there.
There was a problem hiding this comment.
Shouldn't the license remain? https://github.com/terser/terser#:~:text=JSDoc-style%20comments-,that%20contain%20%22%40license%22,-%2C%20%22%40preserve%22%20or%20start
No strong preference here. Keeping the extra minification pass seems fine to me.
There was a problem hiding this comment.
oh yeah, it starts with /*! so it is actually preserved.
It's mostly annoying just how slow that single file is compared to everything else, but I'm fine keeping the status quo of not special casing it
| // Verify that minification was valid. End result should be something like: | ||
| // `const sum = eval('function add(a,b){return a+b}add(1,2);')sum;` | ||
| const evaledResult = eval(code); | ||
| expect(evaledResult).toEqual(3); |
There was a problem hiding this comment.
this is kind of tricky code but it should be robust to future changes to minification tool and options, and it didn't feel right to not have any unit test that the inlined string was still evaluatable javascript
3 participants
part of #13231
just uses our current settings (default terser minification targeting
ecma: 2019).