Skip to content

fix(shims): narrow runWith* return type when callback is async#864

Merged
james-elicx merged 2 commits intocloudflare:mainfrom
fengmk2:fix/lint-await-thenable-overloads
Apr 21, 2026
Merged

fix(shims): narrow runWith* return type when callback is async#864
james-elicx merged 2 commits intocloudflare:mainfrom
fengmk2:fix/lint-await-thenable-overloads

Conversation

@fengmk2
Copy link
Copy Markdown
Contributor

@fengmk2 fengmk2 commented Apr 21, 2026

Add Promise<T> overloads to runWithRequestContext, runWithHeadersContext, and runWithNavigationContext so async callbacks don't widen the return type to T | Promise<T>. This clears 4 typescript-eslint(await-thenable) warnings where Promise.all received the wider union.

Add Promise<T> overloads to runWithRequestContext, runWithHeadersContext,
and runWithNavigationContext so async callbacks don't widen the return
type to T | Promise<T>. This clears 4 typescript-eslint(await-thenable)
warnings where Promise.all received the wider union.
@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 21, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@864

commit: 6c206a8

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

The overloads are correct and the approach is clean — when the callback is () => Promise<T>, the narrower overload fires and the return type stays Promise<T> instead of widening to T | Promise<T>. This eliminates legitimate await-thenable warnings at call sites that pass async callbacks.

One note: there are 7 more exported runWith* functions in the shims directory with the exact same T | Promise<T> pattern that would benefit from the same treatment:

File Function
router-state.ts runWithRouterState
i18n-state.ts runWithI18nState
cache-runtime.ts runWithPrivateCache
cache.ts _runWithCacheState
head-state.ts runWithHeadState
navigation-state.ts runWithServerInsertedHTMLState
request-context.ts runWithExecutionContext

Plus the internal runWithUnifiedStateMutation helper in unified-request-context.ts, which all of the above delegate to when inside a unified scope.

The three functions fixed here are the ones that currently cause lint warnings, so this PR is fine to land as-is. But it might be worth adding the overloads to the remaining functions in the same PR for consistency — they'll eventually hit the same issue when someone passes an async callback to them.

export function runWithRequestContext<T>(
ctx: UnifiedRequestContext,
fn: () => T | Promise<T>,
): T | Promise<T>;
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.

The overload pair is correct. Worth noting that runWithUnifiedStateMutation (line 134) has the same T | Promise<T> signature and is called by runWithHeadersContext and runWithNavigationContext when inside a unified scope. Adding the same overload there would make the type narrowing work end-to-end through the delegation chain.

Not blocking — TypeScript is still happy because the implementation signature of runWithUnifiedStateMutation is compatible. But if someone later calls runWithUnifiedStateMutation directly with an async callback, they'd hit the same widening.

* useServerInsertedHTML callbacks on concurrent runtimes.
*/
export function runWithNavigationContext<T>(fn: () => Promise<T>): Promise<T>;
export function runWithNavigationContext<T>(fn: () => T | Promise<T>): T | Promise<T>;
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.

Same file has runWithServerInsertedHTMLState at line 82 with the identical T | Promise<T> pattern but no overload. Consider adding one there too for consistency.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 21, 2026

Review submitted. Here's a summary of my findings:

The PR is correct. The overloads properly narrow the return type when an async callback is passed — fn: () => Promise<T> now returns Promise<T> instead of T | Promise<T>, which clears the await-thenable lint warnings.

I left two inline comments:

  1. On unified-request-context.ts — suggesting the internal runWithUnifiedStateMutation helper (which the modified functions delegate to) should get the same overload for end-to-end type narrowing through the delegation chain.
  2. On navigation-state.ts — noting that runWithServerInsertedHTMLState in the same file has the identical pattern but wasn't updated.

And in the top-level review body, I listed 7 additional exported runWith* functions across the shims that have the same T | Promise<T> pattern and would benefit from the same treatment for consistency, even though they don't currently trigger lint warnings.

github run

…sync

Extend the Promise<T> overload pattern from PR cloudflare#864 to the other eight
exported runWith* helpers in the shims directory for consistency, so any
future call site passing an async callback gets the narrower Promise<T>
return type instead of the T | Promise<T> union.

Addresses bonk review on cloudflare#864.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

LGTM. Clean, correct, and complete.

The overloads are well-structured: the narrower fn: () => Promise<T> overload is listed before the wider fn: () => T | Promise<T> overload in every case, which is required for TypeScript's top-to-bottom overload resolution. The pattern is sound because AsyncLocalStorage.run<R>(store, callback: () => R): R naturally preserves the Promise<T> return type when the callback is async — the overloads just expose that narrowing to callers.

The second commit addresses all the remaining runWith* functions (including runWithUnifiedStateMutation and runWithServerInsertedHTMLState from the first review), so the entire shim surface is now consistent. No runtime changes, purely additive type signatures, CI is green.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 21, 2026

Review submitted. Approved the PR — the changes are correct, complete, and consistent across all runWith* functions in the shims directory.

github run

@james-elicx james-elicx enabled auto-merge (squash) April 21, 2026 09:56
@james-elicx james-elicx merged commit 88e57e1 into cloudflare:main Apr 21, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants