fix: treat only a whole file: URL as one when parsing a request - #670
Conversation
The test was unanchored, so any request carrying `file:` anywhere went to `fileURLToPath` and threw `ERR_INVALID_URL` - `/foo/file:bar.js`, and `/tmp/profile:1.js`, whose `profile:` contains it. It now reuses the anchored `isFileURL`, which also reads one character before any regexp runs on this hot path.
🦋 Changeset detectedLatest commit: b034a55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
=======================================
Coverage 98.14% 98.14%
=======================================
Files 49 49
Lines 10042 10046 +4
=======================================
+ Hits 9856 9860 +4
Misses 186 186
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A literal `file:///foo/bar.js` names no absolute path on Windows, so `fileURLToPath` threw there and three cases failed on the Windows runners. Deriving each input with `pathToFileURL` carries the drive letter, and a path with a space keeps the percent-encoding the test is for.
Merging this PR will improve performance by 10.63%
Performance Changes
Tip Curious why performance improved? Comment Comparing |
Node's URL parser normalizes `file:/abs/x.js` to three slashes and `import()` loads it, but the anchored test wanted two - so that form was kept as a literal path. It now needs one slash after the scheme, which still leaves `file:x.js` the relative-looking request it reads as rather than the `/x.js` Node makes of it.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## enhanced-resolve@5.25.1 ### Patch Changes - Read a `file:` URL the way Node does when parsing a request. (by [@alexander-akait](https://github.com/alexander-akait) in [#670](#670)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
parseIdentifiertested for a file URL with an unanchored/file:/i, so any request merely containingfile:was handed tofileURLToPathand threwERR_INVALID_URL:/foo/file:bar.js,./a/file:b.js, and/tmp/profile:1.js— whoseprofile:containsfile:— all crash resolution today. It now reuses the anchoredisFileURLadded in #668, so afile:inside a path segment stays part of the path and only a wholefile://URL is converted. That also leaves one definition of "is a file URL" in the codebase instead of two, and reads one character before any regexp runs on this hot path (the old test scanned the whole string).What kind of change does this PR introduce?
fix.
Did you add tests for your changes?
Yes —
test/identifier.test.jsgains afile:in a path segment group (the four inputs above, one with a query and fragment) and afile:URLs group pinning what must still convert, percent-encoding and an upper-case scheme included. The four path-segment cases throw without the change.Does this PR introduce a breaking change?
No — every request that resolved before still resolves; only the ones that threw now work. A single-slash
file:/abs/x.jsis no longer converted, but nothing produces that form (pathToFileURLandimport.meta.resolve()both emitfile:///…) and no test covered it.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Written with Claude Code. It found the unanchored test while working on #668, reproduced the crash, made the change and the tests, and ran the full
lintand test suites locally (1544/1544 green); every claim above was verified by running it.🤖 Generated with Claude Code
https://claude.ai/code/session_01TmnzpSwSysywhmNw9p3uT5
Generated by Claude Code