Support async response handler

[mikicho]

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('text'))
});
interceptor.on('response', async ({response}) => {
  console.log(1)
  console.log(await response.text())
});

http.get('http://example.test', res => {
  res.on('data', () => {})
  res.on('end', () => {
    console.log(2) 
  })
})

This prints:

1 2 text

I think I can open a PR and apply the same until logic as in the request event.

[kettanaito]

Can you share more on the expected behavior here (maybe some usage scenario)?

The response event is meant as a pure side-effect to react to a response that has already been received. I'm a bit confused about how the call order is important here, given you cannot affect the received response in any way.

[mikicho]

@kettanaito Nock has a cool recorder feature. To implement this feature with msw/interceptors we are using the response event, and we need to await and read the response body.

How the call order is important here

The call order is important because users (and tests) expect the recorded data to be available after the request ends. Do you have any concerns about this?

---

From Nock's perspective we can do one of two things, which I prefer to avoid:

1. For now, use the old implementation (I think the intercept and recording features are completely different implementations); a. We won't support fetch in recording. b. I'm happy about how well mswjs/interceptors covered this feature 😁 (which I was worried about)
2. Not guarantee the order, and update the tests. but this would be a regression.

[kettanaito]

Right now, both end on response and the response event are emitted in parallel. So, there's no guaranteed order of execution based on the response body (e.g. if it's a stream that takes time to finish). This is a bit related to the #400, and I think the current behavior is actually correct: the response event must indicate that the response has been received but not necessarily read (so the response stream is represented as ReadableStream on the Response instance you get in the listener; you can listen to it).

I'm not sure I agree to rework this behavior. If we block the event loop by awaiting all response listeners, we are making the response logic and the response listeners dependent (imagine the response listener throwing while the end hasn't been emitted on response yet). The response event must be a side-effect that runs in parallel to the response being received and read.

What, I think, you want to do is to make sure that Nock reads the response body before the test finishes. If that's the case, then the relationship here is not between the interceptor and Nock but between Nock and the process it runs in (effectively, whichever promise Nock returns that has to be awaited). You can achieve this relationship by awaiting the response listener call and the response body read:

// somewhere-in-nock.js
async function recorderFeature() {
  const responseBodyPromise = new DeferredPromise()

  interceptor.on('response', async ({ response }) => {
    responseBodyPromise.resolved(response.text())
  })

  const responseBody = await responseBodyPromise
}

This way the consumers of the recorderFeature will be guaranteed that the functionality won't resolve until the response body is read. Does this sound like what you want to achieve?

[mikicho]

Thanks for the info! I wasn't aware of this.

I tend to agree with you. My only concern is to make minimum changes in Nock during this migration in ordrer to minimize the risk and not create a gigantic PR for the reviewer. After that, I plan to do some performance improvements.

@gr2m WDYT?
Can we make nock.recorder.play async? I think the friction to the users is minimal and the behavior is more correct.

[gr2m]

@gr2m WDYT? Can we make nock.recorder.play async? I think the friction to the users is minimal and the behavior is more correct.

I agree. Also only a subset of users use the recorder, and it really should be moved out of the nock core package anyway

[mikicho]

Great! 🙏