Fix how headers are handled in @hey-api/client-angular#3860
Fix how headers are handled in @hey-api/client-angular#3860StratusFearMe21 wants to merge 10 commits into
Conversation
|
|
🦋 Changeset detectedLatest commit: 4c5d311 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@StratusFearMe21 is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
size:S
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Consider regenerating the snapshots and the bundled example, adding a changeset, and addressing the inline comments before merging. The core fix is correct and welcome — it's the surrounding plumbing that's missing.
TL;DR — The PR correctly fixes the dropped-headers bug in @hey-api/client-angular by reordering auth-application before request construction and threading the immutable HttpHeaders instance back out of setAuthParams. The fix is sound; the gaps are stale generator snapshots, a missing changeset, and one residual aliasing edge case for auth.in === 'query'.
Key changes
- Split
requestOptionsinto options-building andfinalizeRequest— auth/validators now run between the two so the resultingHttpRequestsees post-auth headers. setAuthParamsreturns the newHttpHeadersinstance — caller reassignsopts.headerssoHttpHeaders.set/append(which return new instances rather than mutating) are no longer lost across the previous shallow-copy boundary.- Public
client.requestOptions(...)rewired — now callsfinalizeRequest(requestOptions(options)).reqto keep the same external contract.
Summary ｜ 2 files ｜ 1 commit ｜ base: main ← fix-angular-headers
Snapshots and example are stale
Before: generated snapshots and the committed
openapi-ts-angularexample still emit the oldrequestOptions(returning{ opts, req, url }) and the oldsetAuthParams(returningvoid).
After: every snapshot needs the newfinalizeRequestsplit and the newreturn options.headers;lines.
The Angular client bundle/client.ts and bundle/utils.ts are templated into generator output and captured as snapshots. The diff updates the source but not the 22 stale client/utils.gen.ts + client/client.gen.ts pairs under packages/openapi-ts-tests/main/test/__snapshots__/3.1.x/clients/@hey-api/client-angular/*/client/, nor the committed example at examples/openapi-ts-angular/src/client/client/. Run pnpm tu -- @hey-api/openapi-ts and pnpm examples:generate so the regeneration ships in this PR; otherwise the snapshot tests will diff in CI.
packages/openapi-ts-tests/main/test/__snapshots__/3.1.x/clients/@hey-api/client-angular/default/client/utils.gen.ts · examples/openapi-ts-angular/src/client/client/client.gen.ts
Missing changeset
Before:
.changeset/contains onlyREADME.md,changelog.js,config.json.
After: afixchangeset for the package owning the angular client bundle so the fix gets released.
This is a behavior-affecting bug fix to a published client. Per repo convention (Changesets) it needs a .changeset/*.md entry — e.g. precedent: PR #3814 for client interceptor changes — otherwise the fix won't ship in any version.
Pre-existing thematic bug worth folding in
Before:
opts.headers.delete('Content-Type')atclient.ts:85is a no-op becauseHttpHeaders.delete()returns a new instance instead of mutating.
After:opts.headers = opts.headers.delete('Content-Type');matches the pattern this PR establishes forset/appendand the existing pattern inmergeHeaders(utils.ts:294).
This line is unchanged by the diff, but it's the same immutability bug the PR fixes elsewhere, on the same theme ("how headers are handled"), and means empty-body requests still send the default Content-Type: application/json. Worth a one-line fix here while you're in the file.
Claude Opus ｜ 𝕏
| opts.headers = await setAuthParams({ | ||
| ...opts, | ||
| security: opts.security, | ||
| }); |
There was a problem hiding this comment.
Query-auth tokens still get dropped here. setAuthParams receives a shallow copy via { ...opts }; when opts.query is undefined, setAuthParams (utils.ts:200-203) does options.query = {} on the copy and writes the token into that throwaway object — the assignment never propagates back to outer opts, so finalizeRequest(opts) → buildUrl(opts) runs without the query-auth params. (When opts.query is already an object, the shared reference saves you.) Either return query alongside headers from setAuthParams, pre-initialize opts.query = opts.query ?? {} before this call, or stop shallow-copying and let setAuthParams mutate opts directly.
| return opts; | ||
| }; | ||
|
|
||
| const finalizeRequest = (opts: any) => { |
There was a problem hiding this comment.
opts: any weakens the type safety that the previous inline construction had — opts is the inferred return of requestOptions and the rest of the file (e.g. line 92's cast opts as Config & RequestOptions) implicitly relies on that. Consider (opts: ReturnType<typeof requestOptions>) so the cast is the only erasure.
| } | ||
|
|
||
| return; | ||
| return options.headers; |
There was a problem hiding this comment.
Pre-existing, but worth confirming intent: return options.headers inside the for loop means only the first security entry that produces a token is applied — subsequent entries (e.g. a second scheme, or a header + cookie combination) are silently dropped. Peer clients (client-fetch/utils.ts:126-154 and friends) iterate every entry. If this PR is the right place to align, drop the inner return and keep only the post-loop one.
There was a problem hiding this comment.
Yep, you're right. I don't think that should be there
|
@pullfrog, do I need a changeset even for a bugfix? |
|
@pullfrog, would this constitute a major, or a minor bump to the version? |
dosubot
Bot
added
size:M
StratusFearMe21
force-pushed
the
fix-angular-headers
branch
from
f5958fd to
6e48019
Compare
|
@pullfrog, how's the PR looking now? |
dosubot
Bot
added
size:L
|
Yeah, I'm stuck. I have no clue how to regenerate the snapshots to make the CI pass. |
1 participant
In the current implementation of the
@hey-api/client-angularclient, the HTTP headers that are set before a request is made don't make it to the finalHttpRequestobject.HttpRequestobject must be set after theoptshave been fully initialized with all theHttpHeaderssetoptswas being shallow cloned into thesetAuthParamsfunction, the finalHttpHeadersobject wasn't actually making it out of that function. I've modified the function so it returns the finalHttpHeadersout.