Skip to content

Fix module resolution for relative require() inside CJS deps when the project path contains spaces - #15156

Merged
dario-piotrowicz merged 4 commits into
mainfrom
dario/vitest-spaces-1
Aug 17, 2026
Merged

dario-piotrowicz merged 4 commits into
mainfrom
dario/vitest-spaces-1

Conversation

@dario-piotrowicz

Copy link
Copy Markdown
Member

Fixes #15048

When a project lives under a directory with a space in its name, externalized CommonJS dependencies that use relative require() calls (e.g. require("./lib/impl.js")) would fail with "No such module" because workerd percent-encodes the space as %20 in the module name, and subsequent relative imports inherit that encoding. The module fallback handler now marks paths containing spaces with the same sentinel prefix already used for non-ASCII paths, so the encoding can be deterministically reversed on the way back.


  • 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: bugfix

A picture of a cute animal (not mandatory, but encouraged)

@changeset-bot

changeset-bot Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c5b7fb2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/vitest-pool-workers Patch

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-actions

github-actions Bot commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

✅ All changesets look good

@ask-bonk

ask-bonk Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Dependencies aren't installed. I've done a thorough static analysis of the logic, which is sound. Let me verify my understanding of one potential concern by re-reading the decode function's safety claim regarding double-encoded %25.

The decode: decodeURIComponent(value.slice(prefix.length)). Input .../50%2520off/...? No wait — encode does % → %25 then space → %20. So 50%off → 50%25off, and decode reverses %25 → %. Correct. For a real %20 literal in the source path: build%20output → % becomes %25 → build%2520output, decode → build%20output. Correct round-trip.

The implementation is correct, well-documented, and thoroughly tested with regression tests covering the space case, space+literal-%, the redirect Location path, and the echoed-specifier decode path. The changeset is appropriately scoped as a patch to the correct package. No logic bugs, security issues, backward-compat violations, or incorrect API behavior found.

LGTM

github run

@pkg-pr-new

pkg-pr-new Bot commented Aug 12, 2026 •

Copy link
Copy Markdown
@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@15156

@cloudflare/build-output-utils

npm i https://pkg.pr.new/@cloudflare/build-output-utils@15156

@cloudflare/config

npm i https://pkg.pr.new/@cloudflare/config@15156

create-cloudflare

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

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@15156

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-functions

npm i https://pkg.pr.new/@cloudflare/pages-functions@15156

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@15156

@cloudflare/workers-editor-shared

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

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@15156

wrangler

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

commit: c5b7fb2

@dario-piotrowicz
dario-piotrowicz marked this pull request as ready for review August 13, 2026 10:39
@workers-devprod
workers-devprod requested review from a team and petebacondarwin and removed request for a team August 13, 2026 10:39
@workers-devprod

workers-devprod commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

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

Copy link
Copy Markdown
Contributor

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

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much nicer fix!

@workers-devprod workers-devprod left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codeowners reviews satisfied

@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Aug 14, 2026
@dario-piotrowicz
dario-piotrowicz marked this pull request as draft August 14, 2026 16:48
@dario-piotrowicz

Copy link
Copy Markdown
Member Author

Much nicer fix!

Thanks! 😄

Sorry however I converted the PR to draft as I think it wasn't completely fixing the issue, I'll double check whether these changes need some tweaking, sorry for not having done this earlier! 🙇

@dario-piotrowicz
dario-piotrowicz marked this pull request as ready for review August 17, 2026 11:05
@dario-piotrowicz

Copy link
Copy Markdown
Member Author

@petebacondarwin sorry I've fixed the implementation (the previous iteration didn't really work 😓), could you have another look? 🙏

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great! One nit.

Comment thread packages/vitest-pool-workers/src/pool/module-fallback.ts Outdated
devin-ai-integration[bot]

This comment was marked as resolved.

@dario-piotrowicz
dario-piotrowicz merged commit 3ddd3ce into main Aug 17, 2026
99 of 115 checks passed
@dario-piotrowicz
dario-piotrowicz deleted the dario/vitest-spaces-1 branch August 17, 2026 21:51
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[vitest-pool-workers] Paths with spaces still break on relative require() inside externalized CJS deps (the #14152 fix covers only file: rawSpecifiers)

3 participants