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

fails to correctly minify undici #708

Closed
perrin4869 opened this issue Jun 5, 2024 · 9 comments
Closed

fails to correctly minify undici #708

perrin4869 opened this issue Jun 5, 2024 · 9 comments

Comments

@perrin4869
Copy link
Contributor

perrin4869 commented Jun 5, 2024

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
Copy link
Owner

tdewolff commented Jun 6, 2024

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
Copy link
Owner

tdewolff commented Jun 7, 2024

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

@perrin4869
Copy link
Contributor Author

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

@tdewolff
Copy link
Owner

tdewolff commented Jun 7, 2024

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
Copy link
Owner

tdewolff commented Jun 7, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

tdewolff commented Jun 7, 2024

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
Copy link
Contributor Author

Thanks! I'll run this on Monday!

@perrin4869
Copy link
Contributor Author

everything is fixed! thanks again!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants