fix(node-fetch): respect set-cookies given in HeadersInit
#2079
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThe pull request addresses a bug in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PonyfillHeaders
Client->>PonyfillHeaders: set("set-cookie", value)
Note over PonyfillHeaders: Initialize _setCookies if undefined
PonyfillHeaders-->>PonyfillHeaders: Append cookie value to _setCookies
Client->>PonyfillHeaders: get("set-cookie")
PonyfillHeaders-->>Client: Return concatenated cookie string
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
Contributor
✅
|
Contributor
✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/node-fetch/tests/Headers.spec.ts (1)
64-73: Enhance test coverage for Set-Cookie handling.While the current test case is good, consider adding more test cases to cover:
- Empty arrays
- Single value arrays
- Null/undefined values
- Verification that the original array isn't modified
Here's an example of additional test cases:
describe('Set-Cookie', () => { it('handles values in the given map', () => { const init = { // Record<string, string[]> is not a HeadersInit actually 'set-cookie': ['a=b', 'c=d'], } as any; const headers = new PonyfillHeaders(init); expect(headers.get('Set-Cookie')).toBe('a=b,c=d'); }); + it('handles empty arrays', () => { + const init = { 'set-cookie': [] } as any; + const headers = new PonyfillHeaders(init); + expect(headers.get('Set-Cookie')).toBeNull(); + }); + it('handles single value arrays', () => { + const init = { 'set-cookie': ['a=b'] } as any; + const headers = new PonyfillHeaders(init); + expect(headers.get('Set-Cookie')).toBe('a=b'); + }); + it('preserves the original array', () => { + const cookieArray = ['a=b', 'c=d']; + const init = { 'set-cookie': cookieArray } as any; + const headers = new PonyfillHeaders(init); + headers.append('set-cookie', 'e=f'); + expect(cookieArray).toEqual(['a=b', 'c=d']); + }); });packages/node-fetch/src/Headers.ts (3)
121-129: Handle null values more gracefully in get method.The
toString()call could throw ifvalueis null. Consider adding a null check.get(name: string): string | null { const value = this._get(name); if (value == null) { return null; } - return value.toString(); + return value?.toString() ?? null; }
69-73: Reduce code duplication in set-cookie initialization.The pattern
this._setCookies ||= []; this._setCookies.push(value);is repeated in multiple places. Consider extracting it to a private method.+private ensureSetCookies(value: string) { + this._setCookies ||= []; + this._setCookies.push(value); +} // Then replace the repeated pattern with: -this._setCookies ||= []; -this._setCookies.push(value); +this.ensureSetCookies(value);Also applies to: 79-83, 92-96
131-136: Simplify the has method.The
hasmethod could be simplified by leveraging the_getmethod which already handles null checks.has(name: string): boolean { - if (name === 'set-cookie') { - return !!this._setCookies?.length; - } - return !!this._get(name); // we might need to check if header exists and not just check if it's not nullable + return this._get(name) !== null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/curvy-insects-cheer.md(1 hunks)packages/node-fetch/src/Headers.ts(12 hunks)packages/node-fetch/tests/Headers.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: unit / node 23
- GitHub Check: unit / deno
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: alpha / snapshot
- GitHub Check: unit / node 18
- GitHub Check: server (undici)
- GitHub Check: type check
- GitHub Check: prettier
- GitHub Check: server (ponyfill)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: lint
- GitHub Check: esm
- GitHub Check: server (native)
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.changeset/curvy-insects-cheer.md (1)
1-6: LGTM!The changeset correctly describes the bug fix and uses the appropriate patch version bump.
Contributor
✅
|
Contributor
✅
|
Contributor
✅
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/node-fetch/src/Headers.ts (1)
130-138:⚠️ Potential issueAdd null check before toString() call.
The
toString()call might throw ifvalueis null, despite the null check. Consider moving thetoString()call inside the null check.- if (value == null) { - return null; - } - - return value.toString(); + return value?.toString() ?? null;
🧹 Nitpick comments (2)
packages/node-fetch/src/Headers.ts (2)
165-192: Consider early return after Set-Cookie iteration.The code could be more readable with an early return after iterating through Set-Cookie headers.
- this._setCookies?.forEach(setCookie => { - callback(setCookie, 'set-cookie', this); - }); + if (this._setCookies?.length) { + this._setCookies.forEach(setCookie => { + callback(setCookie, 'set-cookie', this); + }); + if (!this._map && !this.headersInit) { + return; + } + }
269-274: Replace non-null assertion with default empty array.The non-null assertion operator could be replaced with a default empty array for better type safety.
- return this._setCookies!; + return this._setCookies ?? [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/node-fetch/src/Headers.ts(12 hunks)packages/node-fetch/tests/Headers.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: unit / node 18
- GitHub Check: alpha / snapshot
- GitHub Check: server (ponyfill)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: server (native)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: esm
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/node-fetch/tests/Headers.spec.ts (1)
64-79: LGTM! Comprehensive test coverage for Set-Cookie handling.The test suite effectively verifies both the concatenated string format via
getand the array format viagetSetCookie, ensuring proper handling of multiple Set-Cookie headers.packages/node-fetch/src/Headers.ts (4)
71-116: LGTM! Maintains lazy initialization pattern.The changes correctly handle Set-Cookie headers while preserving the performance optimization of lazy map initialization.
147-163: LGTM! Consistent handling of Set-Cookie headers.The
setanddeletemethods correctly handle Set-Cookie headers by managing the_setCookiesarray.
194-267: LGTM! Consistent iterator implementation.The iterator methods correctly handle Set-Cookie headers while maintaining the existing optimization patterns.
14-14:Details
❓ Verification inconclusive
Verify the impact of array filtering changes on other headers.
The array filtering logic in
_getmethod now joins all multiple header values with commas. While this works for Set-Cookie headers, verify that this behavior is appropriate for other header types that might have multiple values.Also applies to: 19-45
🏁 Script executed:
#!/bin/bash # Search for header usage in tests to verify if any other headers use multiple values rg -l "headers\.append\(" | xargs rg "append\([^)]+\)" -A 2Length of output: 1048
Review Header Array Filtering Behavior Impact
Based on the test output, joining multiple values with commas works as expected for standard headers (e.g., the test for “x-HEADER” returns "foo, bar"). It appears that the changes to make
_setCookiesoptional and update_get(including the filtering logic) correctly separate the handling of Set-Cookie headers from others.
- Confirm that all non-cookie headers are intended to merge multiple values into a comma-separated string.
- Ensure that headers requiring distinct treatment (like Set-Cookie) are not inadvertently joined as with other headers.
Please verify that these behaviors remain consistent with HTTP standards and that no unintended side effects occur for headers not covered by the current tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #2074
Closes #2076
Closes #2074
Completes WHATWG-63
Keeps the optimization logic
Summary by CodeRabbit