Deprecate "X-Request-Id" in favor of another request deduplication algorithm

[ido323]

Hi!

Thanks for your work. I appreciate the recent addition of the X-Request-Id header for each request. However, it seems to be causing a header CORS issue when the server doesn't allow it. Is there a way to make it optional or find an alternative solution without modifying the server? Your help would be greatly appreciated.

Link to the code:

interceptors/src/interceptors/XMLHttpRequest/XMLHttpRequestController.ts

Line 173 in 94f0369

this.request.setRequestHeader('X-Request-Id', this.requestId!)

[kettanaito]

Hi, @ido323. Thanks for bringing this up.

I agree with you, we shouldn't modify the outgoing requests. This means we should find a better way to implement the internal logic dependent on the X-Request-Id request header.

The intention here is that in some environments, the request client can be implemented using multiple nested APIs. For example, when you construct an XMLHttpRequest instance in JSDOM, you are constructing what is, effectively, a polyfill for that browser API. Underneath it, the polyfill uses the native http module of Node.js to actually perform requests.

This library ships with both XHR and ClientRequest (http) interceptors. Moreover, the higher consumer, like MSW, is likely to use both of those interceptors at the same time. This means that whenever an XHR request happens in Node.js, it will trigger the XHR interceptor first and then the ClientRequest interceptor second (based on the invocation order of XHR -> http.ClientReuqest; this is not the library's specific behavior). When that happens, we will see the same request attempted to be handled twice, which is a mistake given that both interceptors will try to resolve the XHR request against a single request event listener.

To prevent this, and halt the double resolution of XHR requests in Node.js, we've added an internal X-Request-Id request header on the XHR requests. This way when the request is unhandled, it will bubble to the ClientRequest interceptor and will be skipped altogether based on the presence of that request header:

interceptors/src/interceptors/ClientRequest/NodeClientRequest.ts

Lines 143 to 150 in fec0789

	// Prevent handling this request if it has already been handled
	// in another (parent) interceptor (like XMLHttpRequest -> ClientRequest).
	// That means some interceptor up the chain has concluded that
	// this request must be performed as-is.
	if (this.getHeader('X-Request-Id') != null) {
	this.removeHeader('X-Request-Id')
	return this.passthrough(chunk, encoding, callback)
	}

By design, the outgoing request should never have this internal header set since the XHR request journey will always be:

• new XMLHttpRequest() (XMLHttpRequestInterceptor)
  • new http.ClientRequest() (ClientRequestInterceptor)
    • remove X-Request-Id request header

Do you have a reproduction repository for me to see when this issue occurs for you, @ido323?

[avivasyuta]

@kettanaito Hello. I faced exactly the same problem.

I realized that X-Request-Id is added to requests and not removed. Because of this x-request-id in the header Access-Control-Request-Headers. If the server does not support this header, then the request fails due to a CORS violation.

The problem is that the header X-Request-Id is only removed in the ClientRequestInterceptor but not in the XMLHttpRequestInterceptor or FetchInterceptor.

You can reproduce problem in this repo.

Run the following commands and open address http://localhost:5173 in browser

npm i
npm run dev
node server.js

As a result you can see that request to http://localhost:3000/get-user failed with CORS error.

Access to XMLHttpRequest at 'http://localhost:3000/get-user' from origin 'http://localhost:5173' has been blocked by CORS policy: Request header field x-request-id is not allowed by Access-Control-Allow-Headers in preflight response.

[kettanaito]

Thanks for the input, @avivasyuta.

The problem is that the header X-Request-Id is only removed in the ClientRequestInterceptor but not in the XMLHttpRequestInterceptor or FetchInterceptor.

Yes, this is intentional. In the older versions of Node.js, various request clients could be implemented via the http module. But what's more important, both fetch (from third parties like node-fetch) and XMLHttpRequest (again, from third parties like jsdom) were also implemented via the http module. As we move forward and deprecate support for Node.js < 18, perhaps it's time we reconsider the necessity of the X-Request-Id header.

I'm still unsure we can remove it completely due to the XMLHttpRequest -> http.ClientRequest relationship. Since the Interceptors only hooks at specific request points, and doesn't replace the entire request client (which is, again, intentional to retain the integrity of your code), then XHR implementations through http will always trigger one interceptor through another. We need to design a different deduplication algorithm.

One of the things I considered was using Symbols on request instances. If an XHR request happens, the underlying http.ClientRequest will also happen in the same process. This also implies that they will share global state as well as (I assume) request instance reference. Perhaps we can set some internal Symbol on the XMLHttpRequest instance that would let the underlying ClientRequestInterceptor know that it should dedupe the request (or the other way around).

[avivasyuta]

@kettanaito Unfortunately, I have no idea how you can pass an identifier between xmlhttprequest and http instances, given that these are completely unrelated entities.

It seems to me that it makes sense to use the header entirely. In modern solutions, it is not necessary.

[kettanaito]

@avivasyuta, actually, they are not. XMLHttpRequest is often implemented by http.ClientRequest, which is very fortunate since we may try to find a way to let one know about the other.

[avivasyuta]

@kettanaito However http.ClientRequest is not present in the browser. As far as I understand, this deduplication mechanism should not work at all on the client. Alternatively, you can set the header X-Request-Id only if the runtime is nodejs. What do you think?

[kettanaito]

We can easily check in XMLHttpRequestInterceptor if we are in a browser. If we are, there's nothing to de-duplicate, so no need for the header, so we can skip setting it. Do you think that won't work?

[avivasyuta]

We can easily check in XMLHttpRequestInterceptor if we are in a browser. If we are, there's nothing to de-duplicate, so no need for the header, so we can skip setting it. Do you think that won't work?

Yes. this is exactly what I mean. It should work.

[avivasyuta]

I think we can simply make such logic.
#397

[avivasyuta]

@kettanaito Hello. It would be great if you can review my PR and make a release with bug fix.

[kettanaito]

AsyncLocalStorage in Node.js works great for this but it's not a universal algorithm. Interceptors are meant to be environment-agnostic. The deduping of events must work the same in Node.js and in the browser.

[kettanaito]

Since the request that hits multiple interceptors will happen in the same context by design, perhaps we can try using symbols on the request somehow to let the underlying interceptor know it should ignore it?

Affecting request headers, which is a user-defined input, is not nice.