refactor: make ExecutionContext ALS the single source of truth#431
refactor: make ExecutionContext ALS the single source of truth#431james-elicx merged 2 commits intomainfrom
Conversation
Remove ctx as an explicit parameter from internal functions so that all downstream code consistently retrieves the Workers ExecutionContext via getRequestExecutionContext() from the ALS. Changes: - isr-cache.ts: drop ctx? param from triggerBackgroundRegeneration(); remove duplicate ExecutionContext interface (use ALS directly) - app-rsc-entry.ts: drop ctx 4th param from generated _handleRequest; replace ctx.waitUntil() calls with _getRequestExecutionContext()?.waitUntil() - kv-cache-handler.ts: remove duplicate ExecutionContext interface; import ExecutionContextLike from request-context instead - isr-cache.test.ts: update ctx.waitUntil test to supply ctx via runWithExecutionContext() (ALS) instead of as a direct argument - Update entry-templates.test.ts snapshots
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Clean refactoring. The change is straightforward and correct — all ctx parameter threading was already redundant since runWithExecutionContext(ctx, _run) wraps the entire request at the top of handler() (line 1440 in app-rsc-entry.ts), so getRequestExecutionContext() is always populated by the time any waitUntil call runs.
Verified:
-
ALS is always populated when needed. The outer
handler(request, ctx)calls_runWithExecutionContext(ctx, _run)before any code path that reaches_getRequestExecutionContext()?.waitUntil(...). The?.optional chaining correctly handles Node.js dev wherectxis undefined. -
Dev/prod parity is fine. Checked
pages-server-entry.tsanddev-server.ts— both already use the ALS pattern (_getRequestExecutionContext()in the inlinedtriggerBackgroundRegeneration). Thedev-server.tsimportstriggerBackgroundRegenerationfromisr-cache.ts, so it picks up this change automatically.prod-server.tsdoesn't usewaitUntildirectly. -
KVCacheHandlerretains the constructor fallback. Thethis.ctxfield still exists as a fallback in_deleteInBackgroundand_put(getRequestExecutionContext() ?? this.ctx), which is the right call — it preserves backward compatibility for anyone constructingKVCacheHandlerwith an explicitctxoption outside the ALS scope. -
Test coverage is adequate. The ALS test wraps
triggerBackgroundRegenerationinrunWithExecutionContextand assertswaitUntilis called. The "no ctx" test verifies the graceful no-op path. -
Snapshot changes are mechanical — just the parameter removal and
waitUntilpattern change, repeated across 6 test fixture snapshots. Verified they match the source template changes.
|
Review submitted. I approved PR #431 with a detailed review covering:
All 25 CI checks pass. |
Summary
Files changed