fix: add guard for missing context requests

[lisatassone]

• Fixes https://github.com/mswjs/msw/issues/2053

The current code for createResponseListener assumes the request is always present in context but when bypass and passthrough functions are used with fetch, requests never get added to the context. Example usage from the docs for a proxy request.

This is the most straightforward fix that doesn't change the original logic, but something else may be desired if the intent of the code should be that those types of requests are added to context and handled specifically. Its unclear why they do not go through the normal flow if there are provisions to deal with these in core/utils/handleRequest.ts which is used in createRequestListener.

The tests are TypeScript. If the non-null assertion was not there, the type system would have demanded undefined be handled.

Further, to ensure the Response URL is always set as intended in createResponseListener, perhaps the original request can be passed explicitly to the RESPONSE event to use when its not in context.

[thepassle]

I also ran into this issue :) Seems like this would fix it

[kettanaito]

The current code for createResponseListener assumes the request is always present in context but when bypass and passthrough functions are used with fetch, requests never get added to the context. Example usage from the docs for a proxy request.

This is interesting. So the issue is when the response listener is called by the worker on bypassed request, right? This needs an extremely careful examination, we don't want to jump to the wrong conclusion and introduce a wrong fix.

[kettanaito]

Here's what I think happens now to a bypassed request (can be wrong, feel free to correct):

• It's caught by the worker, the worker sends the REQUEST message to the client;
• The request listener generates the request ID and sets that request in context.requests. This happens before the request resolution logic that takes the bypassed state into account:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/browser/setupWorker/start/createRequestListener.ts#L40

The resolution then happens after:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/browser/setupWorker/start/createRequestListener.ts#L43

At this point, context.request.get(requestId) must return the reference to that request object, even if the request has been bypassed.

The bypassing logic itself lives in handleRequest(), which is called after the request has been set on the context:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/core/utils/handleRequest.ts#L55-L60

However, I can see that the worker has its own logical branch for bypassing requests, and in that case it won't even send the REQUEST message to the client:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/mockServiceWorker.js#L205-L210

I'm trying to remember why this duplication is necessary and my hunch tells me that handleRequest() is environment-agnostic. That x-msw-intention request header handling is there to support bypassed requests in Node.js. For the browser usage, it seems that the intention was to never go to the client since a bypassed request cannot be affected by MSW by design.

What happens next is that the worker receives the original response and proceeds to message the client with the RESPONSE event:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/mockServiceWorker.js#L116

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/mockServiceWorker.js#L125-L140

While this message will contain the requestId, the request listener has never been invoked on the client so context.requests.has(requestId) will be false.

This means that bypassed requests cannot be persisted in the context while it's promised in the contract that even original responses will have the request object reference.

Further, to ensure the Response URL is always set as intended in createResponseListener, perhaps the original request can be passed explicitly to the RESPONSE event to use when its not in context.

The thing is, the worker doesn't know what's in the context. This suggestion would imply to always send the request alongside the RESPONSE event, which can be an overkill for the cases when the request has been stored in the context. Besides, we'd have to clone the request once in order to preserve its body so it'd be transferable in the RESPONSE event too.

I'm considering the change that all requests trigger the REQUEST event and the request listener, and then we decide whether to go into the request resolution or not, or let this handle the bypassed requests also to stay consistent across environments:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/core/utils/handleRequest.ts#L55-L60

This proposal has an implication of involving an additional message roundtrip:

• Before: request -> worker -- is bypassed -> fetch() -> RESPONSE
• After: request -> worker -- is bypassed -> REQUEST -> (client) NOT_FOUND/other -> (worker) fetch() -> RESPONSE

Even with this considered, I think I like for the worker to consistently send REQUEST/RESPONSE events to the client, no matter the request type (bypassed/mocked). The worker should really only be used as the interception mechanism, with no logic that affects the request resolution. If we make it this way, it's precisely what it'll become. The surface that decides what to do with the request will always be the client, and it's the client's responsibility to take bypassed requests into account and send the worker the right instruction message on how to proceed.

@thepassle @mattcosta7, what do you think about this, folks?

[kettanaito]

Proposal

Briefly summarizing my proposal from above.

Step 0: Add a failing test for a bypassed request

We need to catch this issue in an automated test before even considering a fix. It sounds like it's a straightforward issue to solve.

Step 1: Remove the bypassed requests handling from the worker

This has to go:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/mockServiceWorker.js#L205-L210

Step 2: Ensure that handleRequest will behave correctly with the bypassed scenarios in the browser.

I believe that's the case already but verifying won't hurt.

[mattcosta7]

Even with this considered, I think I like for the worker to consistently send REQUEST/RESPONSE events to the client, no matter the request type (bypassed/mocked). The worker should really only be used as the interception mechanism, with no logic that affects the request resolution. If we make it this way, it's precisely what it'll become. The surface that decides what to do with the request will always be the client, and it's the client's responsibility to take bypassed requests into account and send the worker the right instruction message on how to proceed.

That makes sense to me, I can take a bit of time later to understand this a bit deeper, but on the surface that sounds like a good path, and the consistency between request handling sounds better than having multiple paths to doing that

[lisatassone]

The current code for createResponseListener assumes the request is always present in context but when bypass and passthrough functions are used with fetch, requests never get added to the context. Example usage from the docs for a proxy request.

This is interesting. So the issue is when the response listener is called by the worker on bypassed request, right? This needs an extremely careful examination, we don't want to jump to the wrong conclusion and introduce a wrong fix.

This is correct and why I wanted to draw your attention to it. It felt like a larger decision needed to be made on whether the context should always contain bypassed requests - which would require a more extensive solution.

[lisatassone]

However, I can see that the worker has its own logical branch for bypassing requests, and in that case it won't even send the REQUEST message to the client:

https://github.com/mswjs/msw/blob/b3e47f71d3a0333aaeab3da5839cb9068a30d7ef/src/mockServiceWorker.js#L205-L210

I'm trying to remember why this duplication is necessary and my hunch tells me that handleRequest() is environment-agnostic. That x-msw-intention request header handling is there to support bypassed requests in Node.js. For the browser usage, it seems that the intention was to never go to the client since a bypassed request cannot be affected by MSW by design.

This is what I understood as well and mentioned it in the original Issue tagged 👍 . Like you, I couldn't reason why that specific branch of logic was there and not in the dedicated request handlers.

While this message will contain the requestId, the request listener has never been invoked on the client so context.requests.has(requestId) will be false.

This means that bypassed requests cannot be persisted in the context while it's promised in the contract that even original responses will have the request object reference.

Yes, this is what is causing the error to occur at the moment.

I'm considering the change that all requests trigger the REQUEST event and the request listener, and then we decide whether to go into the request resolution or not, or let this handle the bypassed requests also to stay consistent across environments.

Much prefer this solution!

[kettanaito]

Merged a few preliminary changes to make this one easier:

• 2090

• 2091

Now we can move on with updating the worker script as I described above. @lisatassone, does this sound interesting to you?

[kettanaito]

I'm tackling this issue in https://github.com/mswjs/msw/pull/2094. Also discovered that we are missing #2093. Nice find.

[kettanaito]

Released: v2.2.6 🎉

This has been released in v2.2.6!

• 📄 Release notes
• 📦 npm package

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.

---

Predictable release automation by @ossjs/release.

[lisatassone]

Nice one! You got through these quickly :D Thanks for this.