fix(parseIsomorphicRequest): bypassing cookies properly #1155
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b64f988:
|
| @@ -15,7 +16,8 @@ export function parseIsomorphicRequest( | |||
| url: request.url, | |||
| method: request.method, | |||
| body: parseBody(request.body, request.headers), | |||
| headers: request.headers, | |||
| credentials: request.credentials, | |||
| headers: new Headers(Array.from(request.headers.entries())), | |||
There was a problem hiding this comment.
I think request.headers is already an instance of the Headers polyfill. Could you share more info as to why we need to construct it again? Are some headers being omitted? If so, have you understand why?
There was a problem hiding this comment.
To be honest, I don't understand perfectly the reason for this but it's needed to keep the headers field. Even if we call parseIsomorphicRequest multiple times.
I'll check how to disappear the headers field when calling parseIsomorphicRequest multiple times.
There was a problem hiding this comment.
Oh. I understood the reason. That the reason why is the setRequestCookies will overwrite the headers field.
No. Apparently it should be confirmed a little more firmly. I'll consider it. (google translated)
There was a problem hiding this comment.
No worries!
The headers must not disappear—that's for certain. I just think that we don't need to deconstruct an existing Headers instance to construct it again 🤔 It'd be great to catch the issue you're experiencing into a unit test.
There was a problem hiding this comment.
I've considered it.
The parseIsomorphicRequest always attaches the all cookies
This fact is commented in https://github.com/mswjs/msw/pull/1155/files#diff-738c9fc8b1eee13315e077f1aed118cc90af2e9cfb3965436290ff67a6d64284R33 .
I think it's correct so I don't modify it.
The setRequestCookie consumes the cookie headers
So we should re-create the new headers field. (because it's overwritten).
Finally, I think current PR is correct.
There was a problem hiding this comment.
Hey, @hrsh7th. Thanks for opening this pull request.
I've left a few suggestions/clarifications. Let me know about those when you have time.
kettanaito
force-pushed
the
fix-credentials
branch
from
8c36d0b
to
c6a1e76
Compare
kettanaito
force-pushed
the
fix-credentials
branch
from
c6a1e76
to
b64f988
Compare
There was a problem hiding this comment.
Thanks for improving the library, @hrsh7th! Welcome to contributors 🎉
## [0.39.2](v0.39.1...v0.39.2) (2022-03-15) ### Bug Fixes * **parseIsomorphicRequest:** bypassing cookies properly ([#1155](#1155)) ([755bc9d](755bc9d)) * remove console.log from "setRequestCookies" ([6f7ed98](6f7ed98)) * set "credentials" to "same-origin" for "ClientRequest" ([#1159](#1159)) ([c3cd80a](c3cd80a)) * set minimal supported Node.js version to 14 ([#1160](#1160)) ([d7ab139](d7ab139))
|
🎉 This PR is included in version 0.39.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [0.39.2](v0.39.1...v0.39.2) (2022-03-15) ### Bug Fixes * **parseIsomorphicRequest:** bypassing cookies properly ([#1155](#1155)) ([755bc9d](755bc9d)) * remove console.log from "setRequestCookies" ([6f7ed98](6f7ed98)) * set "credentials" to "same-origin" for "ClientRequest" ([#1159](#1159)) ([c3cd80a](c3cd80a)) * set minimal supported Node.js version to 14 ([#1160](#1160)) ([d7ab139](d7ab139))
Fix #1154
This PR aims to bypass correctly APIs that require cookies.
I've created the msw-snapshot package for testing that will response from actual server or disk-cache.
I met this problem in the test with the API that requires the login session.