-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
url: handle "unsafe" characters properly in pathToFileURL
#54545
url: handle "unsafe" characters properly in pathToFileURL
#54545
Conversation
Review requested:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54545 +/- ##
=======================================
Coverage 88.03% 88.03%
=======================================
Files 652 652
Lines 183761 183817 +56
Branches 35863 35873 +10
=======================================
+ Hits 161765 161816 +51
- Misses 15229 15241 +12
+ Partials 6767 6760 -7
|
This comment was marked as outdated.
This comment was marked as outdated.
How does it have an impact on |
There was a typo in the current benchmark causing only |
This comment was marked as outdated.
This comment was marked as outdated.
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1618/
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1619/
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't land this PR with this much impact on almost all Node.js operations.
V8 performance lesson of the day: multiple regexes are better than a single one apparently 🤷♂️ |
This comment was marked as duplicate.
This comment was marked as duplicate.
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1620/
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1621/
|
This comment was marked as duplicate.
This comment was marked as duplicate.
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1622/
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1623/
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1624/
|
As @RedYetiDev originally mentioned in #54515, both |
4aa41b2
to
94ce4a2
Compare
Heads up that this would affect nodejs/loaders#198 because the hooks are exposing URLs, even though for CJS, they are always paths, so there needs to be one extra |
Actually it seems node/lib/internal/modules/esm/resolve.js Lines 266 to 267 in 00c0644
node/lib/internal/modules/esm/resolve.js Line 794 in 00c0644
It's invoked 16 times when loading npm CLI, even though the npm CLI is mostly CommonJS (but it does load several package.json and uses dynamic import to load chalk) |
I think as Yagiz mentioned, we should probably move the implementation to C++ (we need to call the URL constructor anyway, so given we have to cross into native land anyway, it would only makes sense to use C++ rather than JS regexes to do the string manipulation). I don't know if I'm the right person to work on this, but I can certainly try – I first need to fix the current implementation, as it looks like it's failing on Windows. |
There are relevant test failures on windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RLGTM
Landed in 7ae193d |
Co-authored-by: EarlyRiser42 <[email protected]> PR-URL: nodejs#54545 Fixes: nodejs#54515 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Co-authored-by: EarlyRiser42 <[email protected]> PR-URL: #54545 Fixes: #54515 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Co-authored-by: EarlyRiser42 <[email protected]> PR-URL: nodejs#54545 Fixes: nodejs#54515 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Co-authored-by: EarlyRiser42 <[email protected]> PR-URL: nodejs#54545 Fixes: nodejs#54515 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: #54515
Given the number of characters to cover, I leaning towards using one regex to deal with them all rather than adding more special cases. Let's check what the benchmark says.
Originally posted by @RedYetiDev in #54515 (comment)
I took the tests from #54516, so I added @EarlyRiser42 as co-author.