[core] Refactor getWorld interface to be asynchronous#942
Conversation
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
🦋 Changeset detectedLatest commit: 2653faf The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (73 failed)mongodb (7 failed):
redis (7 failed):
turso (59 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
workflow with 1 step💻 Local Development
▲ Production (Vercel)
workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
This reverts commit 15193e8.
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Review: [core] Refactor getWorld interface to be asynchronous
This is a well-executed refactor that enables dynamic imports for custom world implementations. The async conversion is thorough across the codebase, the promise caching in getWorld/getWorldHandlers correctly prevents race conditions, and the new Function trick for hiding dynamic imports from bundlers is pragmatic. A few items below.
| } | ||
|
|
||
| /** | ||
| * This hides the dynamic import behind a function to prevent the bundler from |
There was a problem hiding this comment.
Using new Function('specifier', 'return import(specifier)') is a well-known trick to hide dynamic imports from bundlers, but it has a security implication: it uses eval-like behavior that may be flagged by CSP policies (unsafe-eval). Worth adding a comment explaining why this is necessary and that it runs server-side only (not in browser contexts where CSP would matter).
There was a problem hiding this comment.
Good call — there's already a comment above explaining why the new Function indirection is needed (added in an earlier commit per Nathan's review). Since this only runs server-side in Node.js workers, CSP isn't a concern here.
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Good use of promise caching with WorldCachePromise to prevent concurrent createWorld() calls. However, there's a subtle issue: if createWorld() rejects (e.g., the custom world module fails to load), the rejected promise is cached in StubbedWorldCachePromise permanently. Subsequent calls to getWorldHandlers() will await the same rejected promise and fail forever without retrying. Consider clearing the promise cache on rejection:
if (!globalSymbols[StubbedWorldCachePromise]) {
globalSymbols[StubbedWorldCachePromise] = createWorld().catch((err) => {
globalSymbols[StubbedWorldCachePromise] = undefined;
throw err;
});
}Same applies to getWorld() below.
There was a problem hiding this comment.
Great catch — fixed in b16728c. Both getWorld() and getWorldHandlers() now clear the cached promise on rejection so subsequent calls can retry:
globalSymbols[WorldCachePromise] = createWorld().catch((err) => {
globalSymbols[WorldCachePromise] = undefined;
throw err;
});| */ | ||
| runId: string; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Storing worldPromise instead of world is a clean approach. One consideration: every property getter (status, workflowName, createdAt, etc.) now chains .then() on the promise, meaning each access re-awaits the resolved promise. This is harmless (resolved promises resolve immediately on .then()) but creates an extra microtask per access. If performance of these getters matters in hot paths, you could cache the resolved world. But this is a very minor nit - the current approach is clean and correct.
There was a problem hiding this comment.
Agreed — the extra microtask from .then() on a resolved promise is negligible. These getters aren't in hot paths (they're called once per client-side getRun() access), so the simplicity win is worth it.
| const DEFAULT_STEP_MAX_RETRIES = 3; | ||
|
|
||
| const stepHandler = getWorldHandlers().createQueueHandler( | ||
| '__wkf_step_', |
There was a problem hiding this comment.
The refactor from getWorldHandlers().createQueueHandler(...) to (worldHandlers: WorldHandlers) => worldHandlers.createQueueHandler(...) with deferred resolution at withHealthCheck(async (req) => stepHandler(await getWorldHandlers())(req)) means getWorldHandlers() is called on every request. Since getWorldHandlers now caches via the promise + resolved value, the overhead is minimal (just checking the cache), but it's worth confirming this doesn't create a new handler instance per request. Looking at the flow: stepHandler(worldHandlers) returns the result of createQueueHandler(...) - so yes, a new handler is created per request. Consider memoizing at the handler level:
let cachedHandler: ReturnType<typeof worldHandlers.createQueueHandler> | null = null;
const getHandler = async (req: Request) => {
if (!cachedHandler) cachedHandler = stepHandler(await getWorldHandlers());
return cachedHandler(req);
};This avoids recreating the queue handler on every request.
There was a problem hiding this comment.
Addressed — the resolved handler is now cached per-process, see the reply to Nathan's comment on the same issue.
| if (!reader) { | ||
| const world = getWorld(); | ||
| const world = await getWorld(); | ||
| const stream = await world.readFromStream(name, startIndex); |
There was a problem hiding this comment.
Moving await getWorld() inside pull means the world is resolved lazily on first read. This is a nice improvement - it means stream construction doesn't block on world resolution.
| // Already a file:// URL | ||
| if (specifier.startsWith('file://')) { | ||
| return specifier; | ||
| } |
There was a problem hiding this comment.
The resolveModulePath function handles several cases well (file://, absolute, relative, package specifiers). One edge case: if require.resolve(specifier) fails (package not installed), the catch block falls through to returning the raw specifier, which will then fail at dynamicImport. The error message from dynamicImport may be confusing (e.g., "Cannot find module './some-package'"). Consider re-throwing the require.resolve error with a more helpful message, or at least logging a warning.
There was a problem hiding this comment.
Fair point — in practice the raw specifier gets passed to dynamicImport() which produces a reasonably clear ERR_MODULE_NOT_FOUND with the specifier in the message. Wrapping it further might actually obscure the native error. I think the current behavior is acceptable but happy to revisit if users report confusing errors.
# Conflicts: # docs/content/docs/api-reference/workflow-api/get-world.mdx # packages/core/src/runtime.ts # packages/core/src/runtime/helpers.ts # packages/core/src/runtime/resume-hook.ts # packages/core/src/runtime/start.ts # packages/core/src/runtime/step-handler.ts # packages/core/src/runtime/world.ts # packages/core/src/serialization.ts # packages/world-postgres/HOW_IT_WORKS.md # workbench/astro/src/pages/api/test-health-check.ts # workbench/example/api/test-health-check.ts # workbench/express/src/index.ts # workbench/fastify/src/index.ts # workbench/hono/src/index.ts # workbench/nest/src/app.controller.ts # workbench/nextjs-turbopack/app/api/test-health-check/route.ts # workbench/nextjs-webpack/app/api/test-health-check/route.ts # workbench/nitro-v2/server/api/test-health-check.post.ts # workbench/nitro-v3/routes/api/test-health-check.post.ts # workbench/nuxt/server/api/test-health-check.post.ts # workbench/sveltekit/src/routes/api/test-health-check/+server.ts # workbench/vite/routes/api/test-health-check.post.ts
- Remove duplicate WORKFLOW_LOCAL_BASE_URL in env.ts - Add await to createWorld() call in CLI setup (now async) - Update step-handler tests for async getWorldHandlers pattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
The core design is sound — the promise-deduplication caching pattern in getWorld/getWorldHandlers is correct, the new Function('specifier', 'return import(specifier)') trick to hide dynamic imports from bundlers is the right approach, and all callers in the codebase have been updated. The Run class correctly stores worldPromise and resolves it once before the poll loop. The documentation and workbench updates are thorough.
However, there are two blocking issues and some non-blocking concerns.
Blocking
-
createQueueHandleris called on every request — In the Vercel world,createQueueHandlerallocates a newQueueClientand callsclient.handleCallback()per invocation. Previously this happened once at module init time and the handler was reused for the process lifetime. Now it happens on every incoming request. This is a measurable regression on the hottest path in the system (every workflow and step invocation). -
Changeset should mark this as a breaking change —
getWorld()changing from() => Worldto() => Promise<World>is a breaking API change. All external consumers must addawait. The changeset should include**BREAKING CHANGE**in its description per AGENTS.md policy.
Non-blocking
- Unrelated formatting changes (double→single quotes in
ai-agent-detection.ts,proxy.ts; object formatting inhooks-table.tsx,use-resource-data.test.ts) add noise to the diff. Consider splitting these out. setWorld(undefined)while agetWorldHandlerspromise is in-flight could cause the old promise to overwrite the cleared cache. Only relevant in tests, but worth a comment.
| if (result.timeoutSeconds !== undefined) { | ||
| return { timeoutSeconds: result.timeoutSeconds }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Blocking: handler(await getWorldHandlers()) calls worldHandlers.createQueueHandler(prefix, callback) on every incoming request. In the Vercel world, createQueueHandler instantiates a new QueueClient and calls client.handleCallback(...) per invocation — this is not cheap.
Previously, createQueueHandler was called once at module init time and the resulting (req: Request) => Promise<Response> was reused for the process lifetime. This is a measurable regression on the hottest path in the system.
Fix: cache the resolved handler after the first call:
let cachedHandler: ((req: Request) => Promise<Response>) | undefined;
return withHealthCheck(async (req) => {
if (!cachedHandler) {
cachedHandler = handler(await getWorldHandlers());
}
return cachedHandler(req);
});This preserves lazy initialization (world resolved on first request) while amortizing createQueueHandler across the process lifetime, matching the old behavior.
There was a problem hiding this comment.
Fixed in b16728c. The resolved handler is now cached after the first call, matching the old per-process-lifetime behavior:
let cachedHandler: ((req: Request) => Promise<Response>) | undefined;
return withHealthCheck(async (req) => {
if (!cachedHandler) {
cachedHandler = handler(await getWorldHandlers());
}
return cachedHandler(req);
});| "@workflow/core": patch | ||
| --- | ||
|
|
||
| Make `getWorld` asynchronous so it can use dynamic imports |
There was a problem hiding this comment.
Blocking: getWorld() changing from () => World to () => Promise<World> is a breaking API change — all external consumers must now await getWorld(). Per AGENTS.md: "Ensure that any breaking changes are marked as BREAKING CHANGE."
Suggested:
**BREAKING CHANGE**: Make `getWorld` and `createWorld` asynchronous to support ESM dynamic imports for custom world modules.
(AGENTS.md also says "all changes should be marked as patch" which is already the case — so patch + BREAKING CHANGE annotation is the correct combination.)
There was a problem hiding this comment.
Updated the changeset to include the BREAKING CHANGE annotation and added @workflow/cli since its setup.ts also needed updating:
**BREAKING CHANGE**: Make `getWorld` and `createWorld` asynchronous to support ESM dynamic imports for custom world modules. All callers must now `await getWorld()`.
| error: errorMessage, | ||
| stack: normalizedStack, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Blocking (same issue as runtime.ts): stepHandler(await getWorldHandlers())(req) calls createQueueHandler on every step invocation request. Same fix needed — cache the resolved handler.
let cachedStepHandler: ((req: Request) => Promise<Response>) | undefined;
export const stepEntrypoint: (req: Request) => Promise<Response> =
withHealthCheck(async (req) => {
if (!cachedStepHandler) {
cachedStepHandler = stepHandler(await getWorldHandlers());
}
return cachedStepHandler(req);
});There was a problem hiding this comment.
Fixed in the same commit — same pattern applied:
let cachedStepHandler: ((req: Request) => Promise<Response>) | undefined;
export const stepEntrypoint = withHealthCheck(async (req) => {
if (!cachedStepHandler) {
cachedStepHandler = stepHandler(await getWorldHandlers());
}
return cachedStepHandler(req);
});| } | ||
|
|
||
| /** | ||
| * Create a new world instance based on environment variables. |
There was a problem hiding this comment.
Non-blocking: Nice pattern — new Function('specifier', 'return import(specifier)') hides the dynamic import from bundlers so they don't try to statically resolve custom world modules. Worth a comment explaining why this indirection exists, since it looks suspicious at first glance.
Also, minor: resolveModulePath converts relative paths via pathToFileURL(resolve(targetWorld)). The resolve() call uses process.cwd() as the base — is that always correct? If the workflow config is in a subdirectory, relative paths would resolve against the wrong base. This is likely an existing behavior though, not introduced by this PR.
There was a problem hiding this comment.
The new Function comment was added in an earlier commit. Re: process.cwd() as the resolve base — yes, this matches the existing behavior from the sync require() version which used createRequire(join(process.cwd(), 'index.js')). It resolves relative to wherever the process is running, which is the project root for both Vercel functions and local dev.
- Cache resolved queue handlers to avoid per-request createQueueHandler overhead - Clear rejected promise cache in getWorld/getWorldHandlers so failures can be retried - Add BREAKING CHANGE annotation to changeset Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| --- | ||
| "@workflow/core": patch | ||
| "@workflow/cli": patch | ||
| --- | ||
|
|
||
| **BREAKING CHANGE**: Make `getWorld` and `createWorld` asynchronous to support ESM dynamic imports for custom world modules. All callers must now `await getWorld()`. |
There was a problem hiding this comment.
| --- | |
| "@workflow/core": patch | |
| "@workflow/cli": patch | |
| --- | |
| **BREAKING CHANGE**: Make `getWorld` and `createWorld` asynchronous to support ESM dynamic imports for custom world modules. All callers must now `await getWorld()`. | |
| --- | |
| "@workflow/core": minor | |
| "@workflow/cli": minor | |
| --- | |
| **BREAKING CHANGE**: Make `getWorld` and `createWorld` asynchronous to support ESM dynamic imports for custom world modules. All callers must now `await getWorld()`. |
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
All blocking issues from my previous review have been addressed in b16728cd:
-
createQueueHandlerper-request regression — Fixed. Bothruntime.tsandstep-handler.tsnow cache the resolved handler after the first call, matching the old behavior wherecreateQueueHandlerwas called once at module init. The world is still resolved lazily on first request, but theQueueClientallocation is amortized across the process lifetime. -
Changeset missing BREAKING CHANGE — Fixed. Now reads
**BREAKING CHANGE**: Make getWorld and createWorld asynchronous.... Also correctly added@workflow/cli: patchsincecli/src/base.tswas updated. -
Bonus: Promise rejection handling —
getWorld/getWorldHandlersnow clear the cached promise on rejection via.catch((err) => { cache = undefined; throw err; }). This means ifcreateWorld()fails (e.g., ESM import error), subsequent calls retry instead of permanently caching the failure. Good improvement.
LGTM.
`new Function('specifier', 'return import(specifier)')` throws
"A dynamic import callback was not specified" in CJS contexts like
the vitest e2e test runner. Try require() first — it works for
CJS-compatible packages (e.g. @workflow/world-postgres) and in test
runners. Fall back to dynamic import() for ESM-only modules.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Waiting with merging until new release cycle |
Integrate main's resilient start (runInput), replay timeout guard, preloaded events, expanded StartOptions exports, and streamFlushIntervalMs into the branch's async getWorld() pattern. Cache flush interval from resolved world promise for synchronous access in scheduleFlush. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
getWorld() is now async, but the resilient start test was missing the await, causing it to spread a Promise instead of a World instance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The webhook handler (resumeWebhook) needs to read hook metadata to determine the respondWith behavior. However, the webhook Lambda may not have the deployment encryption key available, causing metadata hydration to fail with "Encrypted stream data encountered but no encryption key is available". Fix by passing undefined instead of the encryption key when serializing hook metadata in the suspension handler. Hook metadata is small (just respondWith config) and doesn't need encryption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The package version was reset from 4.2.x-beta to 4.0.0 for the v5
beta release. The encryption format capability check used 4.2.0-beta.64
as the minimum version, causing getRunCapabilities("4.0.0") to report
encryption as unsupported. This made resumeHook strip the encryption
key, while the step handler still encrypted data — causing "Encrypted
stream data encountered but no encryption key is available" errors in
the webhook handler.
Fix by lowering the minVersion to 4.0.0 to cover the reset range.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…anges
The PR branches had version 4.0.0 (intermediate changeset reset) instead
of 5.0.0-beta.0 (the actual published version). This caused
getRunCapabilities("4.0.0") to report encryption as unsupported, breaking
the webhook respondWith flow on Vercel Prod deployments.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Re-reviewing after 9 new commits since my last approval (b16728cd). Most are mechanical (merge commits, version sync to 5.0.0-beta.0, hook encryption change + revert, e2e test await fix). The one substantive change is 81a548b7 which reverses the module resolution strategy: require() is now tried before dynamicImport().
Status
| Commit | Change | Assessment |
|---|---|---|
81a548b7 |
Try require() before dynamicImport() for custom world modules |
Blocking — see below |
4300ff04 |
await getWorld() in resilient start e2e test |
Correct |
a5b0e82d + e129d929 |
Hook encryption change + revert | Net zero — no concern |
8345fcfe + 8a5acb5c + 9220639e |
Capabilities/version sync | Mechanical — correct |
dec7335d + 1c944854 |
Merge commits | N/A |
Blocking: require() before dynamicImport() can silently load wrong export
The commit message says this fixes CJS test runners where new Function-based import() is unavailable. That's a legitimate concern. However, trying require() first on the raw targetWorld specifier introduces a subtle issue for dual-package (CJS+ESM) custom world modules.
When a package has both CJS and ESM entry points (common with "exports" in package.json), require() resolves the CJS entry and import() resolves the ESM entry. These can export different things. The whole point of making getWorld async was to support ESM — but now require() will silently succeed and load the CJS entry for dual packages, even though the user may have intended the ESM entry.
Additionally, the bare require(targetWorld) call bypasses resolveModulePath() entirely — so relative paths and file:// URLs will behave differently between the require() path and the dynamicImport() path.
Suggested fix: Try dynamicImport() first (the primary path since this PR's purpose is ESM support), fall back to require() for environments where dynamic import is unavailable:
let mod: any;
try {
const resolvedPath = resolveModulePath(targetWorld);
mod = await dynamicImport(resolvedPath);
} catch {
mod = require(targetWorld);
}This preserves ESM-first semantics while still supporting CJS test runners as a fallback.
| } | ||
|
|
||
| const mod = require(targetWorld); | ||
| // Try require() first — works for CJS-compatible packages and in test |
There was a problem hiding this comment.
Blocking: require() before dynamicImport() has two issues:
-
Dual-package hazard: For packages with both CJS and ESM entries,
require()silently loads the CJS entry. The user may have intended the ESM entry (especially since this PR's purpose is ESM support).require()succeeding doesn't mean it loaded the right export. -
Path handling inconsistency:
require(targetWorld)is called with the raw specifier, whiledynamicImport()goes throughresolveModulePath()which handles relative paths, absolute paths, andfile://URLs. A relative path like./my-world.mjswould be resolved byrequire()relative to the CJS resolution algorithm (thecreateRequirebase), but bydynamicImport()relative toprocess.cwd()viaresolveModulePath. These can differ.
Suggested: flip the order — try dynamicImport() first, fall back to require():
let mod: any;
try {
const resolvedPath = resolveModulePath(targetWorld);
mod = await dynamicImport(resolvedPath);
} catch {
mod = require(targetWorld);
}This preserves ESM-first semantics (the reason this PR exists) while still supporting CJS test runners where new Function-based import() fails.
There was a problem hiding this comment.
Good call — flipped the order in 3fa3b6a. dynamicImport() is now tried first (through resolveModulePath()), with require() as the fallback for CJS test runners. This preserves ESM-first semantics and avoids the dual-package hazard you described.
Addresses review feedback — try ESM-first since this PR's purpose is ESM support, fall back to require() for CJS test runners where new Function-based import() is unavailable. This avoids the dual-package hazard where require() silently loads the CJS entry for packages that ship both CJS and ESM. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Review: Make getWorld interface asynchronous
This is a well-executed refactor that makes getWorld() and createWorld() async to support ESM dynamic imports for custom world modules (fixes #812, #825). The approach is sound — the promise-based caching prevents race conditions, the rejected promise clearing enables retry, and the handler restructuring correctly defers createQueueHandler until the world is resolved.
What looks good
-
Race condition prevention:
getWorld()andgetWorldHandlers()store the promise immediately inglobalSymbols[WorldCachePromise]before awaiting, so concurrent callers share the same in-flight promise. This is the correct pattern. -
Rejected promise clearing:
.catch((err) => { globalSymbols[WorldCachePromise] = undefined; throw err; })ensures transient failures (e.g., network errors during module loading) don't permanently poison the cache. Subsequent calls get a fresh attempt. -
Handler restructuring: Both
workflowEntrypointandstepEntrypointare correctly changed from eagerly creating the queue handler at module scope (const handler = getWorldHandlers().createQueueHandler(...)) to a factory pattern (const handler = (worldHandlers: WorldHandlers) => worldHandlers.createQueueHandler(...)). ThewithHealthCheckwrapper resolves the world and passes the handlers. This means the async world resolution happens once per cold start, not per request. -
Module resolution order: ESM-first with
dynamicImport()falling back torequire()is correct for this PR's purpose. TheresolveModulePathhelper correctly handles file URLs, absolute paths, relative paths, and package specifiers. -
new Functionfor dynamic import: Hidingimport()behindnew Function('specifier', 'return import(specifier)')prevents bundlers from trying to resolve the custom world module at build time. This is a well-known pattern. -
setWorld()clears promise caches: BothWorldCachePromiseandStubbedWorldCachePromiseare cleared whensetWorld()is called, preventing stale promises from overriding the explicitly set world instance. -
Breaking change annotation: The changeset correctly includes
**BREAKING CHANGE**since all callers must nowawait getWorld(). -
Docs updated: All code samples in the API reference docs are updated to use
await getWorld().
Observations (non-blocking)
-
Cached queue handlers: After the
getWorldHandlers()promise resolves, the resolved world is cached inglobalSymbols[StubbedWorldCache]. On subsequent calls, the sync path returns immediately without awaiting. Similarly,getWorld()caches inglobalSymbols[WorldCache]. The promise is only used for the initial resolution. This means per-request overhead is just a globalThis symbol lookup — negligible. -
createRequirepath: Changed fromjoin(process.cwd(), 'index.js')topathToFileURL(process.cwd() + '/package.json').href. This is a correctness improvement —pathToFileURLhandles special characters and Windows paths correctly. -
Large diff size (1456 additions / 1261 deletions): Most of this is re-indentation from wrapping the handler bodies inside the factory function. The actual logic changes are confined to
world.ts(~70 lines of new code) and the handler wiring inruntime.ts/step-handler.ts.
Resolve conflicts preserving the async getWorldHandlers interface while incorporating main's specVersion health check parameter and features.encryption context field. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
The blocking issue from my last review is resolved in 3fa3b6aa:
require() before dynamicImport() → flipped to dynamicImport() first — Exact fix I suggested. dynamicImport() (ESM-first) is now tried first via resolveModulePath(), with require() as fallback for CJS test runners. This preserves ESM-first semantics (the purpose of this PR) while still supporting environments where new Function-based import() is unavailable.
The merge artifact fix (1c663f6f) is clean — removes a duplicate MAX_QUEUE_DELIVERIES import from merge conflict resolution and adds the features: { encryption: !!encryptionKey } field that was introduced on main after the branch diverged.
LGTM.
Resolve conflicts: async getWorld() with main's world.streams API (streams docs, e2e, run.getTailIndex, serialization stream helpers). Made-with: Cursor
- Clarify getWorld API card, get-world page, and world overview/streams - Add getWorld() to docs-typecheck globals - SKILL: await getWorld(), world.streams API, bump to 1.7 Made-with: Cursor
ghost
mentioned this pull request
4 participants
This is based on community PR #836 which highlighted the need for an asynchronous interface so that we can use ESM dynamic imports.
Fixes #812
Fixes #825