fix(ClientRequest): add rawHeaders symbol to response listener instance#704
fix(ClientRequest): add rawHeaders symbol to response listener instance#704
rawHeaders symbol to response listener instance#704Conversation
|
Hi, @mikicho. Thank you so much for opening this.
So, is it a timing issue? When |
src/utils/fetchUtils.ts
Outdated
|
|
||
| super(finalBody, { | ||
| // construct the response manually so it will use the Response proxy for raw headers | ||
| const response = Reflect.construct(Response, [finalBody, { |
There was a problem hiding this comment.
My only concern with this is that it messes up the prototype chain. We can no longer do things like x instanceof FetchResponse because the class constructor returns an arbitrary object (a plain Fetch API Response instance).
…ers-to-response-listener
|
@kettanaito WDYT? |
kettanaito
changed the title
rawHeaders symbol to response listener instance
| expect(response.status).toBe(200) | ||
| expect(response.statusText).toBe('OK') | ||
| expect(response.headers.get('x-response-type')).toBe('original') | ||
| expect(Reflect.get(response.headers, kRawHeaders)).toContainEqual(['x-response-type', 'original']) |
There was a problem hiding this comment.
The only thing I don't quite like about this change is this assertion. We shouldn't tap into internal symbols anywhere.
Is there any other way to check that the fix is working?
There was a problem hiding this comment.
I can't think of another way. This is what I plan to do in nock as well, to extract the rawHeaders from the response instance in the response listener.
Do you have another direction, even a general one?
|
@mikicho, this looks good! I have only one comment to share: #704 (comment). Would love for us to update the tests not to reference internal symbols. |
|
I believe this should be obsolete now that we have #719. Let's test-drive it in Nock and then close this pull request. |
|
@kettanaito, this is about the response raw headers, not the request's. |
|
Closing this one favor of the |
2 participants
@kettanaito I hope I didn't create an abomination 😅 let me know if this is the way and if there is a more elegant way to achieve this.
The problem is that
FetchResponsedid not use ourResponseproxy, resulting in norawHeaderssymbol being created in the instance. By utilizingReflect.construct, I ensured it went through the proxy as intended.TODO:
getOwnPropertySymbols?extends&Reflect.constructtrick if it's a valid option.