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

[v3.0] Use ASCII characters for hash placeholders #4631

Merged
merged 2 commits into from Sep 6, 2022

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Sep 3, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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

@github-actions
Copy link

github-actions bot commented Sep 3, 2022

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:
https://rollupjs.org/repl/?pr=4631

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #4631 (58c03c5) into release-3.0.0 (777e9aa) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@                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              
Impacted Files Coverage Δ
src/utils/base64.ts 100.00% <100.00%> (ø)
src/utils/hashPlaceholders.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@patak-dev
Copy link
Contributor

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 !~{\d+}~ could be enough in case it is better to keep 8 chars.

@lukastaegert
Copy link
Member Author

They should not hard-code the hash length anyway because it is now legitimate to write things like [hash:14] in your file name pattern for arbitrary hash length! But admittedly, we can also remove one character. I just want to have enough room in the default pattern for 1000 chunks so people will not reach the ceiling easily.

@patak-dev
Copy link
Contributor

patak-dev commented Sep 3, 2022

Did you consider using [a-z0-9] instead of \d for the chunk codes? That would bump the default pattern to 40k+ chunks using 8 chars for !~{...}~, and at least to me, I wouldn't mind chunks identified as ag4

@lukastaegert
Copy link
Member Author

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...

@lukastaegert lukastaegert merged commit 079ae7e into release-3.0.0 Sep 6, 2022
@lukastaegert lukastaegert deleted the ascii-hash-placeholders branch September 6, 2022 05:19
@lukastaegert lukastaegert added this to In progress in Release 3.0.0 via automation Sep 6, 2022
@lukastaegert lukastaegert moved this from In progress to Done in Release 3.0.0 Sep 6, 2022
lukastaegert added a commit that referenced this pull request Sep 6, 2022
* Use ASCII characters for hash placeholders

* Back to 8 character hashes with base-64 encoding
@lukastaegert lukastaegert mentioned this pull request Sep 6, 2022
9 tasks
@rollup-bot
Copy link
Collaborator

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 npm install rollup@3.0.0-6 or npm install rollup@beta. It will likely become part of a regular release later.

lukastaegert added a commit that referenced this pull request Sep 6, 2022
* Use ASCII characters for hash placeholders

* Back to 8 character hashes with base-64 encoding

3.0.0-6

Improve issue comment RegExp
lukastaegert added a commit that referenced this pull request Sep 22, 2022
* Use ASCII characters for hash placeholders

* Back to 8 character hashes with base-64 encoding

3.0.0-6

Improve issue comment RegExp
@rollup-bot
Copy link
Collaborator

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 npm install rollup@3.0.0-7 or npm install rollup@beta. It will likely become part of a regular release later.

@rollup-bot
Copy link
Collaborator

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 npm install rollup@3.0.0-8 or npm install rollup@beta. It will likely become part of a regular release later.

lukastaegert added a commit that referenced this pull request Oct 11, 2022
* Use ASCII characters for hash placeholders

* Back to 8 character hashes with base-64 encoding

3.0.0-6

Improve issue comment RegExp
@rollup-bot
Copy link
Collaborator

This PR has been released as part of rollup@3.0.0. You can test it via npm install rollup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 3.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants