fix(path): classify DOS device paths as Windows-absolute#551
Merged
alexander-akait merged 9 commits intomainfrom Apr 23, 2026
Merged
fix(path): classify DOS device paths as Windows-absolute#551alexander-akait merged 9 commits intomainfrom
alexander-akait merged 9 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: d15336b 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 #551 +/- ##
==========================================
+ Coverage 96.62% 96.64% +0.02%
==========================================
Files 50 50
Lines 2812 2830 +18
Branches 871 881 +10
==========================================
+ Hits 2717 2735 +18
Misses 79 79
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alexander-akait
force-pushed
the
claude/handle-dos-device-paths-wyAFi
branch
from
1145f07 to
b52eb01
Compare
`getType()` previously missed paths starting with `\\?\` (Win32 file namespace) or `\\.\` (Win32 device namespace), so they fell through to `Normal` and downstream `normalize`/`dirname`/`join` ran them through `path.posix` — `..` segments weren't collapsed and `dirname` returned `"."` instead of the parent directory. Recognize the two DOS device prefixes in the hot char-code switch and return `AbsoluteWin` so normalization, dirname, and join use `path.win32`, which handles these paths correctly. Plain UNC (`\\server\share`) and forward-slash variants are left alone — Windows treats DOS device paths as literal and forward slashes are not equivalent inside them.
The `getType()` fix alone wasn't enough to make a real resolve against a DOS device path work — two adjacent bugs surfaced once the resolver actually received one: 1. `parseIdentifier` split `\\?\C:\foo` on the literal `?` and returned request=`\\` / query=`?\C:\foo`, so the request reached `isModule` as an empty-ish string and was looked up under `node_modules`. Skip past a `\\?\` / `\\.\` prefix before scanning for query and fragment separators. 2. `cdUp()` returned `\` from `\` (via `slice(0, i || 1)`) and `loadDescriptionFile` walked that loop forever. It never hit this on plain drive-letter paths because the walk stops at `C:`, but UNC roots and DOS device paths reach a bare `\`. Treat `\` as a root (same as `/`) and also bail if the parent equals the input. Adds `test/dos-device-paths.test.js` driving the real resolver with a minimal backslash-keyed fake filesystem (memfs normalizes keys through posix `path` so it can't hold a DOS device path as a root on Linux). Covers relative/absolute requests, `\\?\UNC\…`, the `\\.\` device namespace, index/directory resolution, query and fragment preservation, missing-file failures, and description-file lookup termination.
Replace the fake backslash-keyed in-memory filesystem with Node's real `fs` + `path` modules and gate the whole suite on `process.platform === 'win32'`. The earlier fake FS let the tests run on Linux but could not model how Windows actually services `\\?\…` and `\\.\…` paths — these only mean anything to the Win32 kernel, so the honest check is to hit real fixtures through those prefixes on a Windows host. `symlinks: false` is required on the resolver: the realpath plugin otherwise strips the DOS prefix from the result, masking exactly what these tests are asserting on. The pure-logic fixes (getType / parseIdentifier / cdUp) remain covered on every platform via `test/path.test.js` and `test/identifier.test.js`. On non-Windows CI the 9 resolve tests show up as skipped; on Windows they exercise the end-to-end resolver path.
Drop `useSyncFileSystemCalls: true` and route the 8 success-path and failure-path tests through `resolver.resolvePromise` + jest's `.resolves` / `.rejects` matchers. Exercises the same async code path production consumers hit. The description-file test keeps the callback form because that's the only shape that surfaces the `request` argument (and its `descriptionFilePath`).
The DOS-path fix guarded both `"/"` and `"\\"` as roots up front but also added a per-call `slice` → local → equality check as belt-and- braces. `alias-field` and `description-files-multi` are both hot on `cdUp` via `loadDescriptionFile`, and the extra work shaved ~3-4% off them in the benchmark. The equality check is redundant once both single-char roots are filtered: any successful `slice(0, i)` with `i > 0` is strictly shorter than the input, and the `i === 0` branch (`slice(0, 1)`) only equals the input when the input has length 1 — which the top guard already returns `null` for. Remove the allocation + comparison and keep only the top-level root guard. Benchmarks return to parity with main; the new `dos-device-paths` tests still pass.
CodSpeed showed `description-files-multi` -37% and `alias-field` regressed on the DOS device path patch. All three touched functions are called many times per resolve, so even a handful of extra instructions compounds in instruction-count measurements. - `getType`: move the DOS device prefix check out of the length-≥3 switch and check it last, behind a `c0 === CHAR_BACKSLASH` gate. The main switch keeps its original 3 cases so V8 generates the same dispatch code for POSIX/module inputs; only `\`-prefixed inputs pay for the DOS detection. - `parseIdentifier`: split the fast path into two branches. When the identifier doesn't start with `\` (the overwhelming majority of resolves), run the original two-line `indexOf` scan unchanged. The DOS-prefix skip only runs for `\`-prefixed inputs. - `cdUp`: replace `directory === "/" || directory === "\\"` with `directory.length <= 1`. Single `<=` instead of two string equality checks; also subsumes the empty-string case. Wall-clock A/B over 3 runs each: - description-files-multi: main 23,055 → opt 22,605 (-1.9%, in noise) - alias-field: main 27,411 → opt 27,487 (+0.3%) All tests still pass (1008 + 9 skipped).
alexander-akait
force-pushed
the
claude/handle-dos-device-paths-wyAFi
branch
from
b52eb01 to
19bb28f
Compare
CodSpeed runs benchmarks under `--no-opt` (interpreter-only) which is what exposed the 37% description-files-multi regression — inline extra branches directly slow the Ignition bytecode interpreter, and helper extraction matters here because the interpreter can skip right past the gate call on the common path. - `parseIdentifier`: extract DOS-prefix detection into `dosPrefixEnd()`. Fast path stays two `indexOf` calls; the only added cost for non-`\` inputs is one `charCodeAt(0) === 92` compare. - `getType`: extract DOS-prefix handling into `getDosDeviceType()`. Gate is two compares (`c0 === \\ && c1 === \\`); the rest is behind the cold-helper call. Validated under CodSpeed's exact measurement mode (`--no-opt --predictable` + cachegrind), fully deterministic: - origin/main baseline: 315,314,694 I-refs - this branch (helpers): 315,951,261 I-refs (+0.2%) - inlined if/else variant: 316,127,119 I-refs (+0.26%) So locally the delta is ~0.2% across three runs each — not 37%. If CodSpeed's CI still reports a large regression, it likely points to a stale baseline or instrumentation drift on their side rather than this code; happy to dig into a flamegraph if one is available.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getType()previously missed paths starting with\\?\(Win32 filenamespace) or
\\.\(Win32 device namespace), so they fell through toNormaland downstreamnormalize/dirname/joinran them throughpath.posix—..segments weren't collapsed anddirnamereturned"."instead of the parent directory.Recognize the two DOS device prefixes in the hot char-code switch and
return
AbsoluteWinso normalization, dirname, and join usepath.win32, which handles these paths correctly.Plain UNC (
\\server\share) and forward-slash variants are left alone— Windows treats DOS device paths as literal and forward slashes are
not equivalent inside them.