Skip to content

Remove await-thenable lint rule and fix invalid await usage#1353

Open
nickspaargaren wants to merge 4 commits intoh3js:mainfrom
nickspaargaren:remove-await-thenable-lint-rule
Open

Remove await-thenable lint rule and fix invalid await usage#1353
nickspaargaren wants to merge 4 commits intoh3js:mainfrom
nickspaargaren:remove-await-thenable-lint-rule

Conversation

@nickspaargaren
Copy link
Contributor

@nickspaargaren nickspaargaren commented Mar 20, 2026

This PR removes the typescript/await-thenable lint rule and updates a few call sites that were incorrectly using await,

Why

  • Avoids misleading async semantics and potential confusion around execution order.
  • Keeps the codebase aligned with stricter TypeScript linting by not suppressing await-thenable.

How to test

  • pnpm test
  • pnpm lint

Summary by CodeRabbit

  • Chores
    • Tightened TypeScript linting by enabling an additional rule.
    • Made proxy response callbacks support and await asynchronous handlers for more reliable response flow.
    • Adjusted test initialization sequencing for benchmark and DOM test suites to streamline setup.

@nickspaargaren nickspaargaren requested a review from pi0 as a code owner March 20, 2026 18:41
@coderabbitai
Copy link

coderabbitai bot commented Mar 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cd6c178d-bb0e-48da-b04d-f62e8759e1b5

📥 Commits

Reviewing files that changed from the base of the PR and between f8a26e8 and c85ed26.

📒 Files selected for processing (1)
  • src/utils/proxy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/utils/proxy.ts

📝 Walkthrough

Walkthrough

Removed a lint override from .oxlintrc.json, widened ProxyOptions.onResponse to allow async handlers and awaited its result in the proxy, and changed two tests to call registration/instance creation without awaiting them.

Changes

Cohort / File(s) Summary
Lint configuration
\.oxlintrc\.json
Removed the typescript/await-thenable: "off" override so the await-thenable rule is no longer disabled.
Proxy implementation
src/utils/proxy.ts
Widened ProxyOptions.onResponse type to return `void
Tests (setup control flow)
test/bench/bench.test.ts, test/happydom.test.ts
Changed calls from await createInstances() and await GlobalRegistrator.register(...) to non-awaited calls, so setup proceeds without waiting for those promises to resolve.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • pi0

Poem

🐇 I hopped through configs, tidy and bright,

Turned off a rule, then made async right.
Calls may now promise, I'll wait if they do,
Tests sprint ahead — that's the change crew! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: removing the await-thenable lint rule and fixing invalid await usage found in the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can enable review details to help with troubleshooting, context usage and more.

Enable the reviews.review_details setting to include review details such as the model used, the time taken for each step and more in the review comments.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 20, 2026

Open in StackBlitz

npm i https://pkg.pr.new/h3@1353

commit: 0e4190a

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/utils/proxy.ts (1)

21-21: ⚠️ Potential issue | 🔴 Critical

Breaking change: onResponse callback can be async but is no longer awaited.

The ProxyOptions.onResponse type signature => void doesn't prevent async functions—TypeScript allows async () => void. The existing test at test/proxy.test.ts:458-472 explicitly uses an async onResponse that returns a Promise:

onResponse(_event) {
  return new Promise((resolve) => {
    resolve(event.res.headers.set("x-custom", "hello"));
  });
}

With await removed at line 132, this Promise is fire-and-forget, causing the response to be returned before onResponse completes. This is inconsistent with H3Config.onResponse (src/types/h3.ts:40), which uses MaybePromise<void> and is awaited via Promise.resolve().then() in src/response.ts:29, and with the onResponse middleware (src/utils/middleware.ts:27), which awaits the callback.

To fix while preserving async support:

  1. Update the type to explicitly allow async callbacks: onResponse?: (event: H3Event, response: Response) => void | Promise<void>;
  2. Add await at line 132: await opts.onResponse(event, response);
Proposed fix
 export interface ProxyOptions {
   headers?: HeadersInit;
   forwardHeaders?: string[];
   filterHeaders?: string[];
   fetchOptions?: RequestInit & { duplex?: "half" | "full" };
   cookieDomainRewrite?: string | Record<string, string>;
   cookiePathRewrite?: string | Record<string, string>;
-  onResponse?: (event: H3Event, response: Response) => void;
+  onResponse?: (event: H3Event, response: Response) => void | Promise<void>;
 }
   if (opts.onResponse) {
-    opts.onResponse(event, response);
+    await opts.onResponse(event, response);
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/utils/proxy.ts` at line 21, ProxyOptions.onResponse currently allows
async callbacks but the implementation no longer awaits them, causing
fire-and-forget behavior; update the onResponse type signature to explicitly
allow promises (onResponse?: (event: H3Event, response: Response) => void |
Promise<void>) and restore awaiting the callback where it is invoked (await
opts.onResponse(event, response)); modify the type for ProxyOptions.onResponse
and change the invocation of opts.onResponse to be awaited so async handlers
complete before the response is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/utils/proxy.ts`:
- Line 21: ProxyOptions.onResponse currently allows async callbacks but the
implementation no longer awaits them, causing fire-and-forget behavior; update
the onResponse type signature to explicitly allow promises (onResponse?: (event:
H3Event, response: Response) => void | Promise<void>) and restore awaiting the
callback where it is invoked (await opts.onResponse(event, response)); modify
the type for ProxyOptions.onResponse and change the invocation of
opts.onResponse to be awaited so async handlers complete before the response is
returned.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5a0fd54c-ee04-4ddf-b199-3e11b90d10da

📥 Commits

Reviewing files that changed from the base of the PR and between 388b8f0 and f8a26e8.

📒 Files selected for processing (4)
  • .oxlintrc.json
  • src/utils/proxy.ts
  • test/bench/bench.test.ts
  • test/happydom.test.ts
💤 Files with no reviewable changes (1)
  • .oxlintrc.json

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