cloudflare / cloudflare/workers-sdk

[vitest-pool-workers] Module fallback caches a per-file ESM/CommonJS decision under a per-directory key, so dual-format packages are classified by import order

Open
#15,095 1 comment 0 reactions 0 assignees View on GitHub
package:vitest
Dominant language
TypeScript
Stars
4.5k
Forks
1.5k
Avg merge
3d 8h
Merged PRs (30d)
186

Description

### What versions & operating system are you using?

`@cloudflare/vitest-pool-workers` 0.20.3 (reproduced against `main` @ 6dbd192f1), Node v24.14.1, macOS 15.

### Please provide a link to a minimal reproduction

Inline below — it runs inside this repo, against the package's own test suite, with no external fixtures.

### Describe the Bug

`isWithinTypeModuleContext()` in `packages/vitest-pool-workers/src/pool/module-fallback.ts` decides whether a `.js` file should be handed to `workerd` as an `esModule` or a `commonJsModule`. It answers **yes** in two cases: the nearest `package.json` has `"type": "module"`, **or** the file *is* that package's `"module"` entry point.

```ts
const dirPathTypeModuleCache = new Map();
function isWithinTypeModuleContext(filePath: string): boolean {
const parentPaths = getParentPaths(filePath);

for (const parentPath of parentPaths) {
const cache = dirPathTypeModuleCache.get(parentPath);
if (cache !== undefined) {
return cache; // ← file-dependent answer, returned for any file
}
}

for (const parentPath of parentPaths) {
try {
const pkgPath = posixPath.join(parentPath, "package.json");
const pkgJson = fs.readFileSync(pkgPath, "utf8");
const pkg = JSON.parse(pkgJson);
const maybeModulePath = pkg.module
? posixPath.join(parentPath, pkg.module)
: "";
const cache = pkg.type === "module" || maybeModulePath === filePath;
dirPathTypeModuleCache.set(parentPath, cache); // ← keyed by directory
return cache;
} ...
```

The second condition, `maybeModulePath === filePath`, is a property of **the file being resolved**. The `"type"` condition is a property of **the package**. Both are collapsed into one boolean and cached under a **directory** key, so the first `.js` file resolved out of a package permanently fixes the classification for every other file in it.

The concrete case is an ordinary dual-format package with no `"type"` field:

```json
{ "name": "dual-pkg", "main": "dist/index.cjs.js", "module": "dist/index.esm.js" }
```

Both entries are `.js` and live in the same directory. Resolve `dist/index.esm.js` first and `true` is cached for that directory, so `dist/index.cjs.js` is subsequently sent to `workerd` as an `esModule` even though its body is `module.exports = ...`. Resolve the CommonJS entry first and the ES module entry gets sent as a `commonJsModule` instead. Which one happens depends on import order across test files, so the classification a package gets is not stable.

#### Steps to reproduce

On `main`, append this to `packages/vitest-pool-workers/test/module-fallback.test.ts`:

```ts
describe("REPRO dual-format packages", () => {
let tmp: string;
beforeEach(() => {
tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "mf-repro-")));
});
afterEach(() => removeDirSync(tmp));

it("classifies the CommonJS entry after the ES module entry", async ({ expect }) => {
const pkgDir = path.join(tmp, "node_modules", "dual-pkg");
fs.mkdirSync(path.join(pkgDir, "dist"), { recursive: true });
fs.writeFileSync(
path.join(pkgDir, "package.json"),
JSON.stringify({
name: "dual-pkg",
main: "dist/index.cjs.js",
module: "dist/index.esm.js",
})
);
fs.writeFileSync(path.join(pkgDir, "dist", "index.esm.js"), "export const value = 1;");
fs.writeFileSync(path.join(pkgDir, "dist", "index.cjs.js"), "module.exports = { value: 1 };");

const referrer = toWorkerdSpecifier(path.join(tmp, "entry.js"));
const load = async (fileName: string) =>
await (
await handleModuleFallbackRequest(
fakeVite(),
moduleFallbackRequest({
method: "require",
specifier: toWorkerdSpecifier(path.join(pkgDir, "dist", fileName)),
referrer,
})
)
).json();

expect(await load("index.esm.js")).toHaveProperty("esModule");
expect(await load("index.cjs.js")).toHaveProperty("commonJsModule");
});
});
```

```sh
pnpm -F @cloudflare/vitest-pool-workers test test/module-fallback.test.ts
```

#### Actual

```
AssertionError: expected { …(2) } to have property "commonJsModule"
```

The second response is `{ name: ..., esModule: "module.exports = { value: 1 };" }` — the CommonJS build described to `workerd` as an ES module, purely because the sibling ES module build was resolved first.

Swapping the two `load()` calls makes the test pass and the *reverse* assertion fail, which is the tell that this is order-dependence rather than a wrong constant.

#### Expected

Each file is classified on its own merits: `dist/index.esm.js` → `esModule`, `dist/index.cjs.js` → `commonJsModule`, in either order.

Only `"type"` is safe to cache per directory. The `"module"` entry-point comparison has to run per file.

### Please provide any relevant error logs

No log — the fallback service returns 200 with the wrong module type, so the mistake only shows up as the module being evaluated with the wrong semantics inside `workerd`.

Contributor guide

Open the contributing guide

Research direction

Start in packages/vitest-pool-workers/src/pool/module-fallback.ts by reading isWithinTypeModuleContext(), then run the reproduction in packages/vitest-pool-workers/test/module-fallback.test.ts with pnpm -F @cloudflare/vitest-pool-workers test test/module-fallback.test.ts. Done means the dual-format package's ES module and CommonJS entries receive their correct classifications regardless of load order.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
backend, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.