Skip to content

[vitest-pool-workers] Only probe extensions in module fallback for require(), not import#13074

Merged
penalosa merged 1 commit intomainfrom
penalosa/vitest-12022
Mar 27, 2026
Merged

[vitest-pool-workers] Only probe extensions in module fallback for require(), not import#13074
penalosa merged 1 commit intomainfrom
penalosa/vitest-12022

Conversation

@penalosa
Copy link
Copy Markdown
Contributor

@penalosa penalosa commented Mar 26, 2026 •
edited by devin-ai-integration bot
Loading

Fixes #12022.

The module fallback's maybeGetTargetFilePath unconditionally tried adding .js/.mjs/.cjs/.json extensions to extensionless specifiers for both require() and import. Per the Node.js spec, this extension-probing is specific to CommonJS require() — ESM import requires explicit extensions. Extensionless TypeScript imports continue to work correctly via Vite's resolver.

The extension loop is now gated behind an isRequire flag for user module paths. Internal lib paths (e.g. cloudflare:test-internal → test-internal.mjs) still always probe extensions since these are pool-owned files with known extensionless paths.

  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal module fallback behavior, no user-facing docs change
Open with Devin

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 26, 2026 •
edited
Loading

🦋 Changeset detected

Latest commit: 36c1179

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

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Mar 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2026 •
edited
Loading

✅ All changesets look good

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 26, 2026

All call sites are accounted for:

  1. Line 317: User modules — passes actual isRequire (correct).
  2. Line 340: Internal lib modules — hardcodes true to always probe extensions (correct, with comment explaining why).
  3. Line 217: Recursive call for directory resolution — passes through isRequire (correct).

The logic is sound, the test fixture properly reproduces the issue from #12022, and the fix correctly follows the Node.js spec distinction between CJS require() (extension probing) and ESM import (explicit extensions required). The changeset is well-written.

LGTM

github run

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 26, 2026 •
edited
Loading

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13074

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13074

miniflare

npm i https://pkg.pr.new/miniflare@13074

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13074

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13074

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13074

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13074

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13074

wrangler

npm i https://pkg.pr.new/wrangler@13074

commit: 36c1179

@penalosa penalosa force-pushed the penalosa/vitest-12022 branch from 0e9daed to 25b1ec6 Compare March 27, 2026 00:56
@penalosa penalosa force-pushed the penalosa/vitest-12022 branch from 25b1ec6 to 36c1179 Compare March 27, 2026 01:14
@penalosa penalosa marked this pull request as ready for review March 27, 2026 01:14
@penalosa penalosa requested a review from a team as a code owner March 27, 2026 01:14
@penalosa penalosa requested a review from edmundhung March 27, 2026 01:14
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented Mar 27, 2026 •
edited
Loading

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

devin-ai-integration[bot]
devin-ai-integration bot reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Mar 27, 2026
@penalosa penalosa merged commit 4618c05 into main Mar 27, 2026
50 of 55 checks passed
@penalosa penalosa deleted the penalosa/vitest-12022 branch March 27, 2026 13:22
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Mar 27, 2026
@workers-devprod workers-devprod mentioned this pull request Mar 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments

@edmundhung edmundhung edmundhung approved these changes

Assignees

No one assigned

Labels

None yet

Projects

workers-sdk
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[vitest-pool-workers] module fallback path resolution

3 participants

@penalosa @workers-devprod @edmundhung