[vitest-pool-workers] Fix dynamic import() in entrypoint and DO handlers#13056
[vitest-pool-workers] Fix dynamic import() in entrypoint and DO handlers#13056
Conversation
🦋 Changeset detectedLatest commit: 1ab29ab 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 |
|
✅ All changesets look good |
|
Review posted successfully on PR #13056. Here's a summary of what I found:
The core fix itself — using |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
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: |
|
/bonk fix the formatting in this PR and commit the result |
There was a problem hiding this comment.
Good fix. The core approach is sound: onModuleRunner is a documented vitest VitestWorker hook (vitest v4.1.0 src/runtime/workers/types.ts), and patching transport.invoke() to route through runInRunnerObject() correctly ensures module resolution always happens in the runner DO's I/O context.
A few observations:
The transport is private readonly on ModuleRunner — the PR works around this by casting to a structural type at runtime (as { transport?: { invoke?: ... } }). This is fine pragmatically, but it's worth noting this is reaching into vite internals. If a future vite version changes the transport architecture (e.g., making it truly inaccessible or restructuring NormalizedModuleRunnerTransport), this patch would silently become a no-op due to the optional chaining guard. That's actually a reasonable degradation path, but it might be worth adding a __console.warn if the guard fails (i.e., runner.transport?.invoke is falsy), so the team gets early signal when/if vite changes break this.
Agreeing with other reviewers on two minor issues:
-
The comment on line 208-209 says "transport patch in entrypoints.ts" but the
onModuleRunnerhook is in this same file (index.ts).entrypoints.tshas theimportModule()wrapper which is a separate mechanism. The author already acknowledged this via the reply about the comment never being accurate, but the new text should still be corrected. -
runDurableObjectAlarmandrunInDurableObjectare imported but unused indynamic-import.test.ts. This will likely fail lint (@typescript-eslint/no-unused-vars).
Neither blocks merging.
|
Review posted on PR #13056 with approval. Here's a summary of my findings: The fix is correct and well-targeted. It uses vitest's Three comments posted:
|
Fixes #12924.
Fixes #5387.
Dynamic
import()insideexports.default.fetch(),SELF.fetch(), or Durable Object handlers would hang and fail with "Cannot perform I/O on behalf of a different Durable Object". The module runner's transport — which communicates over a WebSocket owned by the runner DO — was being invoked from a different DO context.The fix uses the
onModuleRunnerhook invitest/worker'sinit()to patch the module runner'stransport.invoke(), routing all calls throughrunInRunnerObject(). This ensures module resolution always happens in the runner DO's I/O context, regardless of where theimport()originates.Includes a regression fixture (
fixtures/vitest-pool-workers-examples/dynamic-import/) covering both the entrypoint path (#12924) and the DO path (#5387).