fix: don't treat bare specifiers as Vitest imports when dist is outside the root - #11159
amanprime07 wants to merge 1 commit into
Conversation
…de the root
getCachedVitestImport derives a "root relative" dist path with
normalizedDistDir.slice(root.length), which assumes the dist directory lives
inside the project root. In a monorepo Vitest is hoisted above the project
root, so the slice returns an unrelated suffix of the dist path ("st", "t")
that is then used as a prefix test against every import specifier.
Node builtins are the only ids that reach this function unresolved, so a bare
import such as "stream" or "tls" matches that suffix and is externalized to
<root>/stream, failing with ERR_MODULE_NOT_FOUND. Which builtins break depends
on the character length of the project root, so it varies per checkout path.
Only derive the relative dist path when the dist directory is actually inside
the root, comparing on a path boundary so a sibling directory whose name is a
prefix of the root (node_modules/vite vs node_modules/vitest) no longer
matches either. The root is normalized so the comparison also holds on
Windows, where distDir is built with node:path but config.root comes from
pathe.
66abc54 to
d1818ea
Compare
|
CI note: the failing The Vitest suite itself passes on Windows — That same test fails on I have deliberately not touched For reference, an earlier push of this branch did fail on Windows for a real reason of mine: the test derived its root from |
Description
In a monorepo, a bare Node builtin import such as
import { PassThrough } from "stream"can fail with:...while
import { PassThrough } from "node:stream"works, and the same file works in a sibling package.Root cause
getCachedVitestImportinpackages/vitest/src/runtime/moduleRunner/cachedResolver.tscomputes:This assumes the dist directory lives inside the project root (the comment below it refers to
/node_modules/.pnpm/vitest/dist). In a monorepo, Vitest is hoisted to the repository root whileconfig.rootis the package directory, sodistDiris not underroot. The slice then returns an arbitrary suffix of the dist path, which is subsequently used as a prefix test:Because
relativeRootis always a suffix of…/vitest/dist, the only values that collide with real specifiers are"t"and"st"— which are prefixes of the builtinstimers,tls,trace_events,tty,streamandstring_decoder.Node builtins are the only ids that reach this function unresolved (everything else is resolved to an absolute path first), so the impact is limited to bare builtin imports — but it is silent and confusing when it hits.
The affected builtins depend on the character length of the project root, so the same repository breaks differently depending on where it is checked out. Observed in one monorepo:
relativeRoot"st"stream,string_decoder"t"timers,tls,trace_events,ttynode:-prefixed specifiers never match a…/vitest/distsuffix, which is why they always work.Minimal reproduction
The directory name
asset-serviceis chosen so thatdistDir.slice(root.length) === "st"; with/tmp/monothe package path is 32 characters and the dist path is 34. Renaming the package directory to a different length makes the failure disappear.Reproduced on 3.2.0 → 3.2.7, 4.1.11 and 5.0.0. Not present in 2.0.5, 3.0.0 or 3.1.0 (the code was introduced in 3.2.0).
Fix
Only derive the relative dist path when the dist directory is genuinely inside the root; otherwise leave it empty so the branch is skipped. The intended
<root>/node_modules/.pnpm/vitest/distcase is unchanged.Two details worth noting:
root=<...>/node_modules/viteanddist=<...>/node_modules/vitest/distthe old slice produced"st/dist"rootis normalized before comparing, becausedistDiris built withnode:path(back-slashed on Windows) whileconfig.rootcomes frompatheTests
test/unit/test/cached-vitest-import.test.tscovers both directions:stream,stream/promises,tls,timers) are not externalized when dist is outside the rootvitestspecifiersReverting just the implementation change and re-running gives 3 failed / 2 passed: the three guards fail, while the two "still externalizes" tests keep passing — i.e. the change only removes the broken path and leaves the intended behaviour intact.
Verification
test/unit: 649 files / 7701 tests passed, no failurestest/workspaces: 20 files / 25 tests, and 1 file / 3 tests, passedtest/node-runner: 1 passed, 0 failedtest/e2e: run against bothmainand this branch. The set of failing files is identical. Each apparently-new failure was reproduced on unmodifiedmainindividually (group-order,list,no-unexpected-logging,vm-threads,snapshots/domain-aria-inline);listandvm-threadsare flaky locally (2/1/0 and pass/pass/fail across repeated runs) and thevm-threadsfailure isReferenceError: gc is not defined. These look environment-related on my machine rather than related to this change.tscerrors insrc/node/pools/browser.ts(Property 'send' does not exist on type 'CDPSession') are present onmainas well and are unrelated.Marked as a draft: I would appreciate a maintainer sanity-check on whether returning
''is preferable to leaving the valueundefined/skipping the cache entry, and whether you would like the test placed somewhere other thantest/unit.