search

tdewolff / minify

Go minifiers for web formats
https://go.tacodewolff.nl/minify
MIT License
3.72k stars 219 forks source link

fails to correctly minify undici #708

Closed perrin4869 closed 4 months ago

perrin4869 commented 4 months ago

I just ran into this in my last round of dependency updates, one of my project's dependencies pulled in undici and my tests started failing ^^;;

I managed to isolate the issue to undici, and I put together the file that is not correctly minified (main.out.js). I also put together the minified version produced by terser, which runs on node correctly. I don't really know where to start looking for the problem, since the bundled undici is quite big, so it's hard to isolate where the problem would be 😅

https://github.com/perrin4869/minify-undici-repro

Edit: I just confirmed that esbuild also manages to minify this correctly

tdewolff commented 4 months ago

Thanks a lot Julian for putting together a test case. I'm terribly swamped in work right now but will try to do my best to find out the culprit soon!

tdewolff commented 4 months ago

git bisect shows ff4ee8ba721967e311f507c01b41bec6555fe63c as the problem (28 May 2022), usage of template literals

perrin4869 commented 4 months ago

interesting, I wasn't aware this was a regression! Thank you so much for looking into it as always 😁

tdewolff commented 4 months ago

Specifically, it's not the fact that strings are converted to template literals, but that \n and \r are being converted to literal characters within template literals (eg. 0x5C6E => 0x0A). This ought to be fine, not sure how undici can tell the difference.

tdewolff commented 4 months ago

I've found it. See the end of https://tc39.es/ecma262/#sec-static-semantics-trv, it says that a a sequence of literal <CR><LF> becomes a 0x0A in the string, not 0x0D0A as I expected.

perrin4869 commented 4 months ago

that's interesting! this means we get to shave off a few more bytes off our scripts 😆 that's really impressive how you tracked this down!

tdewolff commented 4 months ago

Unfortunately, it's the reverse hehe. Now must keep the sequence \r as two characters :-( I've pushed out a fix and will be releasing a new version now.

perrin4869 commented 4 months ago

Thanks! I'll run this on Monday!

perrin4869 commented 3 months ago

everything is fixed! thanks again!!!!