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 8377 commits into
Conversation
|
|
🦋 Changeset detectedLatest commit: e009f30 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. |
…-dsl refactor: pydantic dsl
…chain refactor: rename expression to chain
refactor: valibot plugin
…urn-miswiring fix(client): SSE iterator's TReturn was the endpoint's HTTP error union
perf(shared/codegen-core): few small performance improvements
chore(deps): update eslint monorepo to v9.39.4
chore(deps): update vitest monorepo to v4.1.8
…-1.x chore(deps): update dependency @opencode-ai/sdk to v1.16.0
chore(deps): update dependency oxfmt to v0.53.0
chore(deps): update pnpm to v11.5.1
chore(deps): update dependency @types/node to v24.13.0
chore(deps): update dependency autoprefixer to v10.5.0
chore(deps): update dependency lint-staged to v17
…taValues Allow specifying type-safe meta values
fix(deps): update dependency commander to v15
…eaders-immutable fix: reassign `HttpHeaders` after delete in Angular client
| }, | ||
| ): Promise<void> { | ||
| for (const auth of options.security ?? []) { | ||
| security: Pick<Required<RequestOptions>, 'security'>['security'], |
There was a problem hiding this comment.
@StratusFearMe21 why did you need to move security into its own argument?
There was a problem hiding this comment.
I moved this to its own assignment because at the previous callsite opts was being shallow cloned into an object that was passed to the function, meaning any assignments to opts would go to the shallow-cloned object instead of the passed opts object
| expect(query).toEqual({}); | ||
| }); | ||
|
|
||
| it('sets first scheme only', async () => { |
There was a problem hiding this comment.
Removing that was pullfrog's suggestion. It tests if the function accepts the first security scheme that resolves to an AuthToken, instead of iterating through all the security schemes and handles all the schemes that resolve to an AuthToken
|
|
||
| export async function setAuthParams( | ||
| options: Pick<RequestOptions, 'auth' | 'query' | 'security'> & { | ||
| export const setAuthParams = async ( |
There was a problem hiding this comment.
@StratusFearMe21 why did you need to change the function syntax style?
There was a problem hiding this comment.
Oops, that was my mistake
There was a problem hiding this comment.
That function style is consistent with the rest of the file though
6 participants
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.