fix(ClientRequest): improve rawHeaders recording #602
Conversation
mikicho
changed the title
|
Some tests are failing 🤦 Forgot to test on latest v18/v20. |
|
There's a breaking change in Headers behavior between the minor versions of v18 of Node. The latest v18 works as expected. I will suggest people to update to the latest if they encounter any issues. |
| expect(getRawFetchHeaders(response.headers)).toEqual([['X-My-Header', '1']]) | ||
| }) | ||
|
|
||
| it('stops recording once the patches are restored', () => { |
There was a problem hiding this comment.
I'm wondering if we need the test I wrote if we have this test
There was a problem hiding this comment.
Unit vs integration. Let's keep yours. It tests that ClientRequest interceptors applies/restores the headers patching. The unit tests don't test that.
|
Fixed the compatibility with the latest v18/v20. The tests are finally green again. @mikicho, could you please give these changes another look once you have a moment? Thank you. |
kettanaito
changed the title
| const request = Reflect.construct(target, args, newTarget) | ||
|
|
||
| if (typeof args[1] === 'object' && args[1].headers != null) { | ||
| defineRawHeaders(request.headers, inferRawHeaders(args[1].headers)) |
There was a problem hiding this comment.
Latest Node.js v18 and v20 does not copy Headers symbols (previous versions did!). We need to manually copy the kRawHeaders symbol (or set it by creating a new Headers instance) when constructing Request/Response instances.
|
@kettanaito LGTM |
|
Forgot to add unit tests for |
Released: v0.33.1 🎉This has been released in v0.33.1! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
@kettanaito The problem was that we didn't unset the
kRestorePatchessymbol ondisposeI think my solution may not be "private" enough because I set it to be
configurable. Alternatively, we can conditionally return undefined by toggling a variable, but it's smelly and error-prone.What is the best solution here?