-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
response.complete is false in end listener #443
Comments
Maybe I'm wrong. I'll reopen if needed |
const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const http = require('http')
const interceptor = new ClientRequestInterceptor({
name: 'my-interceptor',
})
interceptor.apply();
interceptor.on('request', ({request}) => {
request.respondWith(new Response('hey', { status: 200 }))
});
http.get('http://example.test', res => {
res.on('end', () => {
console.log(res.complete)
})
}) Do you spot my error here?
|
Afaik, you need to read the response body if you want for it to emit the |
thanks! const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const http = require('http')
const interceptor = new ClientRequestInterceptor({
name: 'my-interceptor',
})
interceptor.apply();
interceptor.on('request', ({request}) => {
request.respondWith(new Response('hey', { status: 200 }))
});
http.get('http://example.test', res => {
res.on('data', () => {})
res.on('end', () => {
console.log(res.complete) // prints false
})
})
|
I think you're right, |
Right now, when using Interceptors, the interceptors/src/interceptors/ClientRequest/NodeClientRequest.ts Lines 330 to 332 in 01aa28d
We do this cloning so then the received response can be read multiple times:
And while both use cases are working right now, it seems that something is off in regards to the |
In Node.js, it's the stream module that sets the Perhaps, our |
I think my initial implementation of
|
From a quick look, I think this is what nock does: https://github.com/nock/nock/blob/main/lib/playback_interceptor.js#L275-L291 |
Thanks for the reference! In Interceptors, we consume the same response body multiple times so we have to have some mechanism in place not to block further consumers if we read the response body internally (for example, in order to construct a |
hey @kettanaito & @mikicho, I am running into the same issue (i.e. Is there a workaround I can go with in the short term? Thanks in advance for the help! |
@saurabh-asurion I don't think there is a workaround, but I have a feeling we will solve it soon :) 🤞 |
@saurabh-asurion, thanks for reporting you're experiencing this issue as well. I need to think on what's the expected behavior here. The I'm working on the fix for this and would like to have it merged sometime on Monday. Thanks for patience. |
@kettanaito, really appreciate you explaining the issue. I spent quite some time looking into the interceptors codebase but wasn't able to figure out the root cause. Thank you for working on a fix! |
@saurabh-asurion, if you'd like to have more context, we never set interceptors/src/interceptors/ClientRequest/NodeClientRequest.ts Lines 330 to 332 in 01aa28d
Because it's not the The actual issue here is that we don't have to clone it that aggressively, to begin with. I think we should be fine with passing it through and observing the response's data (and don't meddling with the |
@kettanaito, after going down a (very informative) rabbit hole of IncomingMessage, stream-readable, and attempting to consume and clone the stream (not IncomingMessage) I was arriving at the same conclusion. I really appreciate the pointers from you (and @mikicho in this thread) as it's helping me in trying out different approaches and learning along the way. I doubt I will be able to solve it on my own but I will definitely try:) ps: this is the only library (that I have come across) that allows me to intercept the request/response without having to write separate interceptors for each HTTP client library 🙇 |
Released: v0.32.0 🎉This has been released in v0.32.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Removes the dependency `light-my-request` which was added in v2.2.0 to circumvent an internal issue of msw (mswjs/interceptors#443). This has since been fixed and this should therefore no longer be necessary.
We set the
this.response.complete
after theresponse.push(null)
, which emits theend
event.Therefore, I think the
response.complete
is always false in theend
event.IIUC the docs, it should be true:
https://nodejs.org/api/http.html#messagecomplete
I think we should switch these lines: https://github.com/mswjs/interceptors/blob/main/src/interceptors/ClientRequest/NodeClientRequest.ts#L489-L490
The text was updated successfully, but these errors were encountered: