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

fix: handle paths with special characters in injectQuery (fix #2585) #2614

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

GrygrFlzr
Copy link
Member

@GrygrFlzr GrygrFlzr commented Mar 20, 2021

Fixes regression introduced by #2435.

Details

  • Unlike the original code which uses url.parse, both new URL() and pathToFileURL will percent-encode path names.
  • new URL() will not percent-encode the % character, whereas pathToFileURL will percent-encode the % character.
  • In order to keep their behavior consistent, we manually replace % with %25 before passing it to new URL(), but we pass the raw path to pathToFileURL.
  • String.prototype.replaceAll is a relatively new API and only added to Node 15.0.0, so we're forced to use String.prototype.replace with a global regex modifier instead.
  • To prevent future regressions, tests were added for a number of edge cases, including part of the original issue solved by fix: Improve injectQuery path handling (fix #2422) #2435.

I'm super unhappy about the manual replace but I can't think of a better way to keep the behavior between new URL() and pathToFileURL consistent.

patak-dev
patak-dev previously approved these changes Mar 20, 2021
@patak-dev patak-dev added the p5-urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority) label Mar 20, 2021
@patak-dev patak-dev requested a review from antfu March 20, 2021 17:44
@patak-dev patak-dev merged commit ed321ba into vitejs:main Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p5-urgent Fix build-breaking bugs affecting most users, should be released ASAP (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants