[vitest-pool-workers] Cache only per-directory facts when classifying .js modules - #15096
[vitest-pool-workers] Cache only per-directory facts when classifying .js modules#15096LeSingh1 wants to merge 2 commits into
.js modules#15096Conversation
… .js modules
isWithinTypeModuleContext() treats a .js file as ESM when its nearest
package.json has "type": "module" OR when the file is that package's
"module" entry point. The second condition depends on the file, but the
combined result was cached keyed on the directory, so the first .js file
resolved out of a package decided the classification for every other one.
For a dual-format package ("main" CJS, "module" ESM, no "type"), the
CommonJS build could be returned to workerd as an ES module — its
module.exports assignments then produce no exports — or the ESM build
returned as CommonJS, depending on which was imported first.
Cache "type" per directory and compare the "module" entry path per call.
🦋 Changeset detectedLatest commit: e6df4bd 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for (const parentPath of parentPaths) { | ||
| const cache = dirPathTypeModuleCache.get(parentPath); | ||
| if (cache !== undefined) { | ||
| return cache; | ||
| const cached = dirPathPackageCache.get(parentPath); | ||
| if (cached !== undefined) { | ||
| return cached.typeModule || cached.modulePath === filePath; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Nested packages can still be misclassified as ES modules based on which file was loaded first
A file's module format is decided from the first already-remembered ancestor folder (dirPathPackageCache.get(parentPath) at packages/vitest-pool-workers/src/pool/module-fallback.ts:103-106) instead of from the closest package description, so a file inside a nested package can be handed to the runtime with the wrong format depending on load order.
Impact: A dependency's file can be loaded as the wrong module format, producing confusing syntax/import errors in tests.
Early-return on any cached ancestor short-circuits the nearest `package.json` lookup
getParentPaths() returns parents nearest-first, but the first loop returns as soon as ANY ancestor is present in the cache, even if a nearer directory has an un-read package.json. Example: resolving /root/a.js caches /root (say type: "module"). Later resolving /root/node_modules/pkg/dist/x.js walks dist, pkg, node_modules, /root — none of the nearer dirs are cached, so /root's entry wins and pkg/package.json is never read, classifying the CommonJS dependency as ESM. The same applies to the new cached.modulePath === filePath comparison, which is then made against an unrelated package's module entry.
This hole predates the PR but is the same order-dependent misclassification class the PR sets out to fix, and it also makes the new test order-sensitive if a temp-root ancestor gets cached first. A fix would be to only consult the cache for the nearest directory that actually contains a package.json (e.g. also cache "no package.json here" negatives so the walk continues correctly).
Prompt for agents
In packages/vitest-pool-workers/src/pool/module-fallback.ts, isWithinTypeModuleContext() walks parent directories nearest-first and returns on the first cached DirPathPackageInfo it finds. Because only directories that actually contained a package.json are ever inserted into dirPathPackageCache, a cached far ancestor can short-circuit the walk before a nearer, not-yet-read package.json is discovered, causing a nested package's files to be classified using the wrong package's `type`/`module` fields (order-dependent). Consider recording negative results too (directories known to have no package.json) so the cached walk mirrors exactly the same nearest-package.json semantics as the uncached filesystem walk, or merge the two loops into a single walk that consults the cache per directory and falls back to reading package.json for that same directory before moving up.
Was this helpful? React with 👍 or 👎 to provide feedback.
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| const info: DirPathPackageInfo = { | ||
| typeModule: pkg.type === "module", | ||
| modulePath: pkg.module ? posixPath.join(parentPath, pkg.module) : "", | ||
| }; | ||
| dirPathPackageCache.set(parentPath, info); | ||
| return info.typeModule || info.modulePath === filePath; |
There was a problem hiding this comment.
🟡 Multi-file ES module builds of packages without a module type declaration are now treated as CommonJS
Files that sit next to a package's ES module entry point are no longer classified as ES modules (info.typeModule || info.modulePath === filePath at packages/vitest-pool-workers/src/pool/module-fallback.ts:119), so tests importing a package whose ES module build is split across several files fail with syntax errors.
Impact: Tests that depend on packages shipping a multi-file ES module build without a declared module type can break with parse errors that did not occur before.
Why the per-directory cache change removes ESM classification from sibling chunks
Previously, resolving dist/index.esm.js of a package whose package.json has "module": "dist/index.esm.js" and no "type" cached true for the package directory, so every other .js file resolved from that package (e.g. relative chunk imports like ./chunk-abc.js emitted by Rollup/esbuild ESM builds) was also returned to workerd as an esModule.
After this change the cache stores { typeModule: false, modulePath: "…/dist/index.esm.js" }, and the entry-point comparison is made per file. The entry itself is still ESM, but each sibling chunk now fails both checks and is returned as a commonJsModule, so workerd parses export/import statements as CommonJS and throws.
These chunk files are resolved directly on disk in maybeGetTargetFilePath() (packages/vitest-pool-workers/src/pool/module-fallback.ts:199-221), so Vite never re-classifies them; the classification at module-fallback.ts:556-558 is authoritative.
A possible mitigation is to keep treating files reached from an ESM entry point (or files inside the directory containing the module entry) as ESM, rather than only the exact entry-point path.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Oh.. good catch. This looks like a regression from the current solution 🤔
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3 participants
Fixes #15095.
isWithinTypeModuleContext()folds two different kinds of fact into one cached boolean:pkg.type === "module"— a property of the package, so safe to cache per directory.maybeModulePath === filePath— a property of the file being resolved, so not safe to cache per directory.Both were stored under a directory key, so the first
.jsfile resolved out of a package decided the classificationworkerdwas given for every other file in it. A dual-format package ("main": "dist/index.cjs.js","module": "dist/index.esm.js", no"type") therefore got its module type from import order: load the ES module build first and the CommonJS build is subsequently returned as anesModule; load the CommonJS build first and the ES module build is returned as acommonJsModule.This change caches only
{ typeModule, modulePath }— both package-level facts — and runs the entry-point comparison againstfilePathon every call. The lookup stays oneMaphit per parent directory, so no extrapackage.jsonreads are introduced.The added test resolves both entries of a dual-format package in one order and asserts each gets its own correct type. It fails on
mainwithexpected { …(2) } to have property "commonJsModule"and passes with the change. The 12 existing tests in the file are untouched and still pass, andpnpm -F @cloudflare/vitest-pool-workers check:typeis clean.Note
This is a contribution from an AI agent: Claude Code, Claude Opus 5.