Closed mikicho closed 2 months ago
Maybe I'm wrong. I'll reopen if needed
@kettanaito
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?
I didn't have time to find the root cause now, but I think the complete
property is always false because we clone the response: https://github.com/mswjs/interceptors/blob/main/src/interceptors/ClientRequest/NodeClientRequest.ts#L330-L332
I'll try to reproduce it with a test later
Do you spot my error here?
Afaik, you need to read the response body if you want for it to emit the end
event. Simply add the res.on('data', () => {})
listener and the end
will be emitted as you expect.
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
})
})
res.complete
prints false. Is this expected?
I think you're right, res.complete
should be true
by the time the end
event is emitted. Will open a fix.
Right now, when using Interceptors, the res.complete
property always remains false
. That's due to us cloning the original emitted response
instance right here:
We do this cloning so then the received response can be read multiple times:
Response
instance out of it.data
and end
events.And while both use cases are working right now, it seems that something is off in regards to the complete
and maybe even the end
event.
In Node.js, it's the stream module that sets the complete
property to true
when the message is complete:
Perhaps, our cloneIncomingMessage
doesn't propagate that [kOnMessageComplete]
symbol to the cloned messages.
I think my initial implementation of IncomingMessage
cloning was a bit too radical and mainly based on my lack of knowledge of streams in Node.js. I can't say it's much better now, but I think what we should do instead is to pipe the original response
through a PipeThrough()
stream that would allow us to react to incoming chunks, and then pipe those chunks to a Readable
that we'd pass to the createResponse()
.
I think if we pipe it to a
Readable
directly, it would still error if we try to read them twice but I can verify.
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 Response
representation out of IncomingMessage
). Nothing in the linked code would account for that, as it's more about converting the body to a stream and reading it than allowing it to read the same stream multiple times. I think that's due to the API difference between the libraries.
hey @kettanaito & @mikicho, I am running into the same issue (i.e. response.complete
is false
) even when using mswjs/interceptors
to just observe the response. I can confirm that I am not mocking the response in interceptor.('request', ({.....}))
Is there a workaround I can go with in the short term? Thanks in advance for the help!
@saurabh-asurion
Move to Nock 😉 (Just kidding, we are working hard to replace the Nock underlining interception engine with @mswjs/interceptors
)
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 response
event is emitted as soon as we received the response, which doesn't necessarily mean that we've received its entire body (for which complete
stands). But this is largely irrelevant for you as the consumer.
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 complete
to true
right now due to the way we clone the original response (IncomingMessage
).
Because it's not the IncomingMessage
itself that sets its complete
property. In fact, that property doesn't even belong to that class, it's a property from the generic stream class and it's being handled there by Node.js. But due to the way we're cloning the message, it never ends up inhering from that stream class (for one reason or the other).
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 super.emit()
call so it'd emit the actual response instance).
@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 🙇
This has been released in v0.32.0!
Make sure to always update to the latest version (npm i @mswjs/interceptors@latest
) to get the newest features and bug fixes.
Predictable release automation by @ossjs/release.
We set the
this.response.complete
after theresponse.push(null)
, which emits theend
event. Therefore, I think theresponse.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