Repository navigation
[vitest-pool-workers] Fix require() resolving to ESM via "import" export condition - #13062
petebacondarwin wants to merge 3 commits into
Conversation
…ort condition When Vite 8's rolldown resolver handles a require() call, it can select the "import" export condition from a dual-format package instead of "require"/"default". This causes workerd to receive an ESM file where it expects CommonJS, producing SyntaxError. The module-fallback service now detects this case by checking whether the resolved path appears as the "import" condition value in the package's exports map, and redirects to the CJS alternative instead. Fixes #12984, #13037
🦋 Changeset detectedLatest commit: 17da622 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds a post-resolution check in the module-fallback service that detects when Vite 8's rolldown resolver picks an ESM "import" condition for a require() call, and redirects to the CJS "require"/"default" alternative.
findCjsEntryInConditionsdoes not handle nested conditions under"import"— When a package uses nested conditions like{ "import": { "types": "./dist/index.d.mts", "default": "./esm/index.mjs" }, "require": "./dist/index.cjs" }(common in TypeScript packages),importEntryis an object, thetypeof importEntry === "string"check fails, the recursion loop skips"import"/"require"/"default"keys, and the function silently returnsundefined. The workaround won't activate for these packages. This is a real limitation worth documenting, though I acknowledge the targeted packages (pg-protocol,pg-connection-string) use the simpler flat pattern, so this may not be blocking.
| requireEntry !== importEntry | ||
| ) { | ||
| return requireEntry; | ||
| } |
There was a problem hiding this comment.
When importEntry is an object (nested conditions under "import", e.g. { types: "...", default: "./esm/index.mjs" }), this whole block is skipped, and the recursion below also skips "import" at line 420. This means the workaround silently does nothing for packages that nest conditions under "import" — a pattern that is very common in TypeScript packages (e.g. zod, drizzle-orm, etc.).
This could be addressed by also checking when importEntry is an object whether any of its terminal string values match esmRelPath, and if so extracting the corresponding CJS entry from requireEntry. Something like:
| } | |
| return requireEntry; | |
| } | |
| // Handle nested conditions under "import" (e.g. { types: "...", default: "./esm/index.mjs" }) | |
| // Check if any terminal string in the import entry matches the resolved ESM path | |
| if ( | |
| typeof importEntry === "object" && | |
| importEntry !== null && | |
| typeof requireEntry === "string" | |
| ) { | |
| const importValues = Object.values( | |
| importEntry as Record<string, unknown> | |
| ); | |
| if (importValues.some((v) => v === esmRelPath)) { | |
| return requireEntry; | |
| } | |
| } |
I acknowledge the targeted packages (pg-protocol, pg-connection-string) use the flat pattern, so this may not be urgent. But given how common this pattern is in the npm ecosystem, it may be worth handling now to avoid future reports.
|
I'm Bonk, and I've done a quick review of your PR. This PR adds a post-resolution check in the module-fallback service that detects when Vite 8's rolldown resolver picks an ESM I flagged one issue:
|
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: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ort export conditions
|
Converting to draft while we see if there is a cleaner solution |
…ve-builtin limitation - Handle nested conditions under 'import' (TypeScript-style packages) - Handle sugar conditional exports (no '.' subpath key) - Pass kind:'require-call' to pluginContainer.resolveId for Vite 8+ (works for pre-bundled deps via vite:resolve-dev; does not fix the general case because vite:resolve-builtin uses a static isRequire) - Keep findCjsAlternative as the reliable fallback for the general case - Expand unit tests to cover all new cases - Update comments to accurately explain the Vite 8 architecture
|
Would be awesome if this could be fixed soon! |
|
can claude fable handle this one? too long time to wait |
|
We ran into exactly this issue ( We've been carrying this PR's fix as a pnpm patch (applied to the built Is this still planned to be merged? Happy to provide more detail on our setup if additional confirmation would help move it along. |
|
We dug into why this PR's The kind hint is currently inert: rolldown's Until that lands and rides a Vite release, the scan in this PR is the only working fix — we've been carrying it as a pnpm patch in production monorepo use since mid-June (re-cut cleanly against 0.18.7) with no regressions. |
|
I've pinged the rolldown team to see if we can get it fixed there. |
|
Closing in favour of #16335 |
Fixes #12984, #13037.
When Vite 8 switched to a rolldown-based resolver,
require()calls in the module-fallback service started resolving to the"import"export condition of dual-format packages (e.g.pg-protocol,pg-connection-string,mimetext) instead of"require"/"default". This caused workerd to receive an ESM file as a CommonJS module, producingSyntaxError: Cannot use import statement outside a module.Root Cause (Vite 8 architecture)
Vite 8's main resolve plugin is
vite:resolve-builtin— a native Rust builtin backed byviteResolvePluginfromrolldown/experimental. ItsisRequireflag is a static value set at construction time (undefinedin all normal configurations), so it always appends"import"to the conditions list regardless of whatkindis passed to theresolveIdhook.isRequirefromcustom["node-resolve"](a private internal convention we were relying on).custom["node-resolve"]is silently ignored. Thekind: "require-call"field is read byvite:resolve-dev(the optimizer-path plugin) and setsisRequiredynamically — but only for packages that have been pre-bundled by the dep optimizer. For non-pre-bundled packages,vite:resolve-devfalls through tovite:resolve-builtin, which ignoreskind. This appears to be a current limitation of the architecture rather than an intentional API, since the native plugin's interface (BindingViteResolvePluginResolveOptions) hasisRequireas a static field with no per-call override.Fix
Three complementary layers:
custom["node-resolve"].isRequire— kept for Vite ≤7 compatibility (rollup-based resolver reads this).kind: "require-call"— added for Vite 8+ as a forward-looking hint. Works today for pre-bundled deps viavite:resolve-dev. If Vite fixesvite:resolve-builtinto readkindper-call, this will start working for the general case automatically.findCjsAlternative()— the reliable fallback for the general case in Vite 8. AfterpluginContainer.resolveId()returns, if the resolved path appears as an"import"condition value in the package'sexportsmap, the"require"/"default"CJS alternative is returned instead. This is an explicit post-resolution fixup rather than a resolver-level fix, but it is correct and necessary given the current Vite 8 architecture.The exports-map walk handles:
{ ".": { "import": "...", "require": "..." } }){ "import": "...", "require": "..." })"import"conditions (TypeScript-style:{ "import": { "types": "...", "default": "..." }, "require": "..." })Verified against both issues
pg/pg-protocol/pg-connection-string): no longer throwsSyntaxError; reachesECONNREFUSED(expected — no DB running)mimetextviaagents): same class of failure, same fix appliesWhy the module-fallback approach is the right one
A reviewer might ask: "Can't we just do what
vite-plugin-cloudflaredoes and avoid the module-fallback service altogether?"vite-plugin-cloudflareuses the Vite Module Runner pattern: all deps pass through Vite's pre-bundler withnoExternal: true, CJS packages are interop-wrapped into ESM at that stage, and workerd executes Vite-transformed code strings via anUnsafeEvalbinding (runInlinedModule). TheunsafeModuleFallbackServiceis only used there for binary module types (Wasm, Data, Text) — never for JavaScript.vitest-pool-workersintentionally takes the opposite approach, and for good reason: the entire point of this package is to run tests in the actual workerd runtime with production-equivalent module semantics.The Module Runner (
runInlinedModule) wraps each module's code in anasync functionbody, not a real ES module. This changes semantics in ways that matter for test correctness:import.meta, live bindings, TDZ, module caching, and circular dependency handling all behave differently from how workerd's native ESM loader would handle them. A test suite that passes underrunInlinedModulemay not accurately reflect what happens in production.By contrast,
vitest-pool-workers' module-fallback approach lets workerd use its own native ESM loader for all user code. Files are served lazily from disk one at a time, with only minimal shimming (CJS→ESM name-export wrapping viacjs-module-lexer). workerd then loads them exactly as it would in production.Pre-bundling everything upfront isn't a practical alternative either: test files are discovered dynamically at runtime (you can't know at startup which files will be run), Vitest's own internals are CJS-heavy and would require substantial interop work, and lazy per-file loading preserves module identity (two packages that both
require('lodash')share a single copy, as in production).So: the
findCjsAlternativeworkaround added in this PR is the correct narrow fix for the Vite 8 regression. The right place to fix the symptom is inviteResolve(), precisely becausemodule-fallback.tsis the component responsible for bridging Vite's resolver into workerd's native module system — and that bridge needs to correctly honourrequire()vsimportsemantics across Vite versions.