Skip to content
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

[XHR] Response interceptors executes after application receives the response #400

Closed
JAspeling opened this issue Aug 8, 2023 · 6 comments · Fixed by #515
Closed

[XHR] Response interceptors executes after application receives the response #400

JAspeling opened this issue Aug 8, 2023 · 6 comments · Fixed by #515
Labels
enhancement New feature or request scope:xhr

Comments

@JAspeling
Copy link

JAspeling commented Aug 8, 2023

Issue Title

XHR Response interceptors get executed after the application receives the response.

Description

I expect to see the response interceptor functions execute before the application receives the response. This is not the case with XMLHttpRequestInterceptor. It is, however, the case with FetchInterceptor

Context

I am working on a solution where we need to intercept all HTTP requests fired by any client (fetch, Angular, Axios, etc). There's no telling which underlying HTTP method is used to make the request, so it should cater to all.

We came across this @mswjs/interceptors package that looked promising as it allows us to intercept all requests, but in the POC, we encountered some strange behaviour.

The expected flow would be:

HTTP request initiated

  - Request interceptor 1
  - Request interceptor 2
  - Request interceptor ...n

  - Response interceptor 1
  - Response interceptor 2
  - Response interceptor ...n

HTTP Response received by the application

This should be the same for all HTTP requests.

Current Behavior

I've noticed that XHR requests it does not follow the above pattern. Instead, they do the following:

HTTP request initiated

  - Request interceptor 1
  - Request interceptor 2
  - Request interceptor ...n

HTTP Response received by the application

  - Response interceptor 1
  - Response interceptor 2
  - Response interceptor ...n

Expected Behavior

I expect the interceptor functions to be executed in the same order, whichever HTTP method is used.

Request interceptor functions should be executed before the request reaches the API, and Response interceptor functions should be executed before the response reaches the application.

Screenshots and Additional Information (if applicable)

image

Steps to Reproduce (if applicable)

Please see the reproduction repo where the below behaviour can be seen:
Github Repo: https://github.com/JAspeling/mswjs-interceptors-angular/blob/main/src/main.ts
Stackblitz: https://stackblitz-starters-dgrmdd.stackblitz.io
Stackblitz editor: https://stackblitz.com/edit/stackblitz-starters-dgrmdd?file=src%2Fmain.ts

  • Use the XMLHttpRequestInterceptor as a BatchInterceptor.
  • Add request interceptors
  • Add response interceptors
  • Make an XMLHttpRequest call to an API endpoint:
xhrRequest(): void {
    console.log('============== XHR [APPLICATION REQUEST INITIATED] ==============');
    const xhr = new XMLHttpRequest();

    xhr.onreadystatechange = function () {
      if (xhr.readyState === 4 && xhr.status === 200) {
        var data = JSON.parse(xhr.responseText);
        console.log('============== XHR [APPLICATION RESPONSE RECIEVED] ==============');
      }
    };

    xhr.open('GET', this.api, true);
    xhr.send();
  }
  • See the order of execution in the console logs.

Environment

Angular CLI: 16.1.6
Node: 18.17.0
Package Manager: npm 9.6.7
OS: win32 x64

@angular-devkit/architect       0.1601.6
@angular-devkit/build-angular   16.1.6
@angular-devkit/core            16.1.6
@angular-devkit/schematics      16.1.6
@angular/cli                    16.1.6
@schematics/angular             16.1.6
rxjs                            7.8.1
typescript                      5.1.6
zone.js                         0.13.1

Please feel free to reach out if anything is unclear or if there is a misunderstanding around using the @mswjs/interceptors package.

@JAspeling JAspeling changed the title [XHR] Response interceptors executes after application receives the response. [XHR] Response interceptors executes after application receives the response Aug 8, 2023
@JAspeling
Copy link
Author

@kettanaito Any idea why I'm getting this behaviour?

@kettanaito
Copy link
Member

kettanaito commented Aug 14, 2023

Hey, @JAspeling. Thanks for a great issue report. Let me provide some additional context and let's see whether we're on the same page.

Intentions

I can confirm that the intention of this library is to emit the request event when the request is sent, and the response event once the response is received. This stands true for all currently implemented interceptors with the exception for the ClientRequestInterceptor which I will mention below.

Regarding the FetchInterceptor, it emits the response event after your application receives a response. What may be confusing here is that the .then() callback this library adds will execute before your own .then() callbacks.

return pureFetch(request).then((response) => {
const responseClone = response.clone()
this.logger.info('original fetch performed', responseClone)
this.emitter.emit('response', {
response: responseClone,
isMockedResponse: false,
request: interactiveRequest,
requestId,
})
return response
})

But that's not what's happening. These two callbacks will execute one after another, but both happen once the response has been received (the request promise has been resolved). It is the Promise-like nature of handling fetch responses that may be confusing here.

It's important to note that response received isn't equivalent to response read. This stands true for fetch and for ClientRequest also, where the response event is emitted as soon as the response is received (the response event of ClientRequest) but the actual response body is streamed into the Fetch API response instance you can access in the response event listener:

this.emitter.emit('response', {
response: createResponse(message),
isMockedResponse: false,
request: capturedRequest,
requestId,
})

Technically speaking, we should emit the request event earlier than we do now, and then pipe the request body chunks to the ReadableStream of the Fetch API request instance representing the in-progress request. I think that would contribute to a more predictable behavior, but it will also change the intention because the request event would be emitted when the request is constructed.

Regarding XMLHttpRequest

We emit the response event within the load event of the XMLHttpRequest, which indicates a successful fetch. You see the difference in time between the response event being emitted and your own onreadystatechange callback because state change triggers before the load event does. Here's an excerpt from the XMLHttpRequest specification describing this order:

  1. Fire an event named readystatechange at xhr.
  2. Fire a progress event named load at xhr with transmitted and length.
  3. Fire a progress event named loadend at xhr with transmitted and length.

Because of this order, your callback will always execute before the response event gets emitted by this library.

I would love to move the response event to readyStateChange once it received response headers, which would translate to the first byte of the response. The challenge here is that, to my best knowledge, there is no method in XHR to read incoming response body chunks. That reading seems to be done internally and all the consumer can do is monitor the bytes read in the progress event (but not the bytes themselves). This makes it problematic to represent the response body as a ReadableStream the response event consumers would be able to read as the response body comes in.

Summary

This is mainly for myself, but let's write down the current state of things:

  • ClientRequestInterceptor
    • request event is emitted once the request body is sent (in the .send() method).
    • response event is emitted as soon as the response is received (once the response is emitted by ClientRequest). The response body is then represented as ReadableStream that the consumers of the response event can read.
  • XMLHttpRequestInterceptor
    • request event is emitted once the request body is sent (in the .send() method).
    • response event is emitted once the response body is finished reading (once the load is emitted). The response body is then represented by whichever response body type came in (not a stream).
  • FetchInterceptor
    • request event is emitted once the request is constructed. Fetch doesn't expose any events to differentiate between request constructed/request writing/request sent. You can read the response body in the request event listener since its represented as ReadableStream.
    • response event is emitted once the response is received (in the .then() callback to the request promise). The response body is then represented as a ReadableStream the consumers of the response event can read.

With this in mind, the behavior of the request event seems a bit inconsistent. We should consider changing when the request event is emitted in ClientRequestInterceptor and XMLHttpRequestInterceptor to be aligned with how it's emitted in the FetchInterceptor, that being:

  • The request event should be emitted as soon as the request is constructed. The consumer should be able to read the sent request body as request.body stream in the request event listener.

The response event is consistent except for the XMLHttpRequestInterceptor, as you've rightfully pointed out. We should change it to be emitted as soon as request received headers (readyState 3), create a ReadableStream to represent the response body chunks as they come in, and close the stream on loadend (I think it should be safer there to account for errors while reading the response; loadend always executes).

My only concern is whether we'd be able to represent the incoming XHR response body as a stream. I wonder if the value of .response will gradually change in the progress event. I didn't find the specification mentioning this but I might have overlooked it.

Do you have any insight regarding the reading of the XHR response body? That'd be great.

@kettanaito
Copy link
Member

If we can find a way to represent the incoming XHR response body as ReadableStream, I'm for this change. I may even have a minute this week to implement it. The research seems to be the biggest challenge here. I could use some help with it.

@kettanaito kettanaito added scope:xhr enhancement New feature or request labels Aug 14, 2023
@JAspeling
Copy link
Author

Thank you for the detailed explanation! I will take the discussion up with my team and perhaps try to get them involved as well - maybe we can find a solution together. I'll do some research on the subject 👍

@smo043
Copy link

smo043 commented May 21, 2024

@kettanaito could you pls help with a fix? I am encountering the same issue. The response is sent before the interceptor request is completed.

@kettanaito
Copy link
Member

Released: v0.32.0 🎉

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request scope:xhr
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants