-
-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: use "IsomorphicRequest" class to handle request body as buffer #267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a fantastic kick off of this idea! Thank you, @95th!
I've left a few comments and thoughts on the changes. I would like to know what you think about those. Let's discuss it, make whichever adjustments are necessary, and have a unified request handling!
The tests should be passing now! I've also added a few minor changes and left a roadmap in the pull request's description of what's remaining. You don't have to tackle everything that remains, just let me know what interests you and I can take care of the rest (like updating the docs). |
clearTimeout(timeoutTimer) | ||
resolve(JSON.parse(event.detail)) | ||
resolve(event.detail) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were previously parsing this, as event.detail
is a string. Do we not need to parse it into object anymore? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is automatically serialized and deserialized
@@ -2,7 +2,7 @@ import { FetchInterceptor } from '@mswjs/interceptors/lib/interceptors/fetch' | |||
|
|||
const interceptor = new FetchInterceptor() | |||
interceptor.on('request', (request) => { | |||
window.requestBody = request.body | |||
window.requestBody = request.text() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should await this, as request.text()
returns a Promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already return promise for it in the test:
function getRequestBody(page: Page): Promise<string> {
return page.evaluate(() => window.requestBody)
}
and documentation for evaluate
says:
If the function passed to the page.evaluate(pageFunction[, arg]) returns a [Promise], then page.evaluate(pageFunction[, arg]) would wait for the promise to resolve and return its value.
I'm also thinking of how the IIR will be propagated to the request listener. It'd be great to just reuse the I'm thinking of something like What do you think about this approach? |
I think we should have a export class MockedRequest extends IsomorphicRequest {
public cookies: Record<string, string> = {}
public redirect = 'manual'
public referrer = ''
public keepalive = false
public cache = 'default'
public mode = 'cors'
public referrerPolicy = 'no-referrer'
public integrity = ''
public destination = 'document'
public bodyUsed = false
public passthrough = passthrough
constructor(request: IsomorphicRequest) {
super(request)
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is by far the most enjoyable pull request in my open source journey. Thank you for collaborating with me on this!
Released: v0.17.0 🎉This has been released in v0.17.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Expose new Request API which allow access to raw buffer
Roadmap
bufferUtils
IsomorphicRequest
constructor on accepting another isomorphic request instance as the argument