-
-
Notifications
You must be signed in to change notification settings - Fork 59
fix: properly await async headers functions in treaty #230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The `headers` config option was not awaiting async functions, causing Promise objects to be passed through instead of resolved header values. - Make `processHeaders` async and await function results - Move config header processing inside async IIFE - Update type to allow `MaybePromise` return from header functions - Add test for async headers configuration
WalkthroughThe pull request converts header processing from synchronous to asynchronous operations. The Changes
Sequence DiagramsequenceDiagram
participant Client
participant createProxy
participant processHeaders as processHeaders<br/>(async)
participant onRequest
participant fetch
rect rgba(100, 150, 200, 0.2)
Note over Client,fetch: New Async Flow
Client->>createProxy: Request with async headers callback
createProxy->>processHeaders: await processHeaders()
activate processHeaders
processHeaders-->>createProxy: Promise<headers>
deactivate processHeaders
createProxy->>createProxy: await Promise resolution
createProxy->>onRequest: invoke with resolved headers
onRequest->>fetch: initiate fetch with merged headers
fetch-->>Client: Response
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/treaty2/index.ts (1)
91-98: Wrap thecase 'function'declaration in a block.The static analysis correctly identifies that
const vdeclared inside a switch case can be erroneously accessed from other cases. Wrap the case body in a block to restrict scope.case 'function': if (h instanceof Headers) return processHeaders(h, path, options, headers) - - const v = await h(path, options) - if (v) return processHeaders(v, path, options, headers) - return headers + { + const v = await h(path, options) + if (v) return processHeaders(v, path, options, headers) + return headers + }
🧹 Nitpick comments (2)
src/treaty2/index.ts (1)
397-406: DuplicateonRequestprocessing block.This block (lines 387-407) duplicates the earlier
onRequestprocessing (lines 292-312). Both iterate overonRequesthandlers and merge headers identically. Consider consolidating to avoid code duplication and potential inconsistencies if one block is updated but not the other.#!/bin/bash # Verify the duplicate onRequest blocks rg -n -A 15 'if \(onRequest\)' src/treaty2/index.tssrc/treaty2/types.ts (1)
246-246: DuplicateMaybePromisetype definition.
MaybePromiseis defined identically at line 59 (module scope) and line 246 (withinTreatynamespace). Consider removing the duplicate and using the module-level definition.- type MaybePromise<T> = T | Promise<T> + // Use module-level MaybePromise defined at line 59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/treaty2/index.ts(6 hunks)src/treaty2/types.ts(1 hunks)test/treaty2.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/treaty2/index.ts (1)
src/treaty2/types.ts (1)
Config(175-193)
test/treaty2.test.ts (2)
src/treaty2/index.ts (1)
treaty(517-543)example/a.ts (1)
app(4-19)
🪛 Biome (2.1.2)
src/treaty2/index.ts
[error] 96-96: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (5)
src/treaty2/index.ts (3)
67-72: LGTM! Correct async conversion ofprocessHeaders.The function signature is properly updated to be async and return a
Promise. This aligns with the PR objective to properly await async header functions.
260-278: LGTM! Proper async header resolution.The config headers and per-request headers are both correctly awaited before being merged into
fetchInit. The sequential processing ensures headers are fully resolved before the request is made.
302-311: LGTM! Consistent async handling inonRequesthandlers.Headers from
onRequestcallbacks are properly awaited and merged, maintaining consistency with the config header processing.test/treaty2.test.ts (1)
575-604: LGTM! Good test coverage for async headers.The test properly validates the async header configuration fix by:
- Using an async function with a simulated delay
- Conditionally returning headers based on path
- Verifying the resolved header value is correctly applied
This directly tests the bug fix described in the PR objectives.
src/treaty2/types.ts (1)
178-184: LGTM! Type correctly updated to support async headers.The
MaybePromisewrapper allows header functions to return either synchronous or promise-based results, aligning with the runtime implementation changes.
Summary
headersconfig functions were not being awaitedheadersbehavior withonRequestwhich already properly awaits async functionsChanges
processHeadersasync and await function resultsMaybePromisereturn from header functionsTest plan
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.