[vitest-pool-workers] Fix module fallback resolving bare specifiers to wrong subpath export#12615
Open
marshallswain wants to merge 8 commits intocloudflare:mainfrom
Conversation
🦋 Changeset detectedLatest commit: b7f2ee6 The changes in this PR will be included in the next version bump. 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 |
fa9f199 to
fee25ec
Compare
fee25ec to
8942d63
Compare
…o wrong subpath export When a dependency has both an npm dependency and a subpath export with the same name (e.g. dependency "some-lib" and subpath export "./some-lib"), the module fallback service incorrectly resolves the bare specifier to the subpath export file instead of the actual npm package. This is triggered by pnpm's symlinked node_modules structure, which causes workerd to join the bare specifier with the referrer directory, accidentally matching the subpath export file. The fix detects this collision by checking whether a bare specifier resolved to a file within the same package as the referrer, and if so, walks the node_modules tree to find and resolve the actual npm package.
8942d63 to
36da767
Compare
Contributor
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
Member
|
Important Note to self and to anyone who reviews this PR, I think that the bug is only present in pnpm, npm is not effected (and I assume neither other PMs are), see: https://github.com/marshallswain/vitest-pool-workers-subpath-export-bug/blob/76551ccfdc412f65ef90470515f5681fe04c66da/README.md?plain=1#L38-L40 |
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
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.
Reproduction here: https://github.com/marshallswain/vitest-pool-workers-subpath-export-bug
Fixes a bug where the module fallback service resolves bare specifiers to wrong subpath exports.
When a dependency has both an npm dependency and a subpath export with the same name (e.g., dependency
"some-lib"and subpath export"./some-lib"), the module fallback service could resolve the bare specifier to the subpath export file instead of the actual npm package. This is particularly triggered when using pnpm, whose symlinkednode_modulesstructure causes workerd to join the bare specifier with the referrer directory, andmaybeGetTargetFilePathfinds the subpath export file before any resolution logic runs.pnpm layout that triggers the bug
Root cause
workerd sends a module fallback request with the bare specifier
"some-lib"and the referrer path (e.g.,.../my-adapter/dist/index.js). The fallback handler joins these to produce a target path like.../my-adapter/dist/some-lib. ThemaybeGetTargetFilePath()function then tries file extensions and finds.../my-adapter/dist/some-lib.js— which is the subpath export file, not the npm package.Fix
Added a
maybeCorrectSubpathCollision()check that detects when a resolved path lands inside the same package as the referrer (indicating a likely subpath export collision). When detected, it walksnode_modulesto find the actual npm package, reads itspackage.json, and resolves the correct entry point through theexportsmap,module, ormainfields.The check is applied at both resolution paths:
maybeGetTargetFilePath()returns early (the primary bug path)viteResolve()returns (a secondary path where Vite's resolver could also match the subpath export)References
This fix aligns the module fallback behavior with Node's ESM resolution algorithm specification. Per the spec, when code inside
my-adaptercontains the bare specifierimport { createApp } from "some-lib", Node resolves it through two stages:PACKAGE_SELF_RESOLVE— checks if the specifier matches the containing package's ownnamefield. Sincepjson.nameis"my-adapter", not"some-lib", self-resolution returnsundefinedand does not consult theexportsmap at all.PACKAGE_RESOLVEstep 10 — walks up throughnode_modules/directories to locate a directory namedsome-lib, then reads that package'spackage.jsonand resolves through itsexports/main.The bug was that both
maybeGetTargetFilePath()and Vite's resolver were effectively skipping thenode_moduleswalk and matching the specifier"some-lib"against the referrer package's own subpath exports (i.e., the"./some-lib"entry inmy-adapter'sexportsmap). Per the spec, a package'sexportsmap is only consulted for a bare specifier whenpjson.name === packageName(self-resolve) or after the package is located via thenode_moduleswalk — neither of which should matchmy-adapter's exports when resolving the specifier"some-lib".Note on scope: The
resolveExportsEntry()helper in this fix is a pragmatic approximation of the spec'sPACKAGE_EXPORTS_RESOLVE, not a full implementation. It hardcodes condition preference asimport > default > requireand does not handle wildcard/pattern exports (e.g.,"./*": "./dist/*.js") or additional conditions like"node"or"worker". This is a reasonable trade-off for a fallback handler — the fix only needs to correctly locate the entry point of the actual npm package once the collision is detected, and the common cases (exportswith".",module,main) are covered.