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
[v3.0] Use ASCII characters for hash placeholders #4631
Conversation
4932980
to
9474ba5
Compare
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#ascii-hash-placeholders or load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## release-3.0.0 #4631 +/- ##
=================================================
- Coverage 98.97% 98.97% -0.01%
=================================================
Files 212 212
Lines 7482 7481 -1
Branches 2114 2114
=================================================
- Hits 7405 7404 -1
Misses 23 23
Partials 54 54
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thanks for digging into this. I agree that this will help with debugging. About the bump to 9 chars, looks like that will uncover some bugs in Vite as we have some regex expecting 8 chars. This is wrong on Vite's side though, and we will fix it, but I wonder if the bump to 9 chars could cause issues with other tools. Maybe it is something that would be good in the long run as the ecosystem shouldn't hardcode the hash length, and Rollup 3 is a good opportunity to force these fixes. FWIW, I think that |
They should not hard-code the hash length anyway because it is now legitimate to write things like |
Did you consider using |
Just laziness but you are right, there is probably no reason not to. I changed the algorithm to re-use a base64 conversion function we are already using elsewhere so by default, we now support 260k chunks... |
5458ef3
to
58c03c5
Compare
* Use ASCII characters for hash placeholders * Back to 8 character hashes with base-64 encoding
This PR has been released as part of rollup@3.0.0-6. Note that this is a pre-release, so to test it, you need to install Rollup via |
* Use ASCII characters for hash placeholders * Back to 8 character hashes with base-64 encoding 3.0.0-6 Improve issue comment RegExp
* Use ASCII characters for hash placeholders * Back to 8 character hashes with base-64 encoding 3.0.0-6 Improve issue comment RegExp
This PR has been released as part of rollup@3.0.0-7. Note that this is a pre-release, so to test it, you need to install Rollup via |
This PR has been released as part of rollup@3.0.0-8. Note that this is a pre-release, so to test it, you need to install Rollup via |
* Use ASCII characters for hash placeholders * Back to 8 character hashes with base-64 encoding 3.0.0-6 Improve issue comment RegExp
This PR has been released as part of rollup@3.0.0. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This changes the hash placeholder pattern to
_!~{\d+}~
where\d+
are some digits to identify the chunks. This should still be sufficiently random to reduce the risk of accidental matches (it is not valid JavaScript so it can only occur in strings and comments where at least it should do limited damage, and we only replace a pattern if the exact sequence of digits is matched).This should prevent issues we had from using generic unicode characters that were escaped by some tooling and also make debugging nicer.
cc @patak-dev @antfu