mswjs
fix(ClientRequest): prevent stacked proxies when intercepting node http(s) requests
It was possible to end up with the http(s).request/get proxies wrapping existing proxies, which can cause odd behaviour: https://github.com/nock/nock/issues/2802
This PR attempts to fix the issue by only creating a proxy once, and just updating the callbacks the proxy uses when it's called a second time. it's not idea (since it could cause confusion with the previous instance, which will now no longer be invoked), but it seems like a preferable scenario.
instead we could error when an existing proxy is detected, but it seems like it would cause a lot of errors in practice (due to nodes module loader cache being inconsistent).
Not sure why but some tests failed when running locally on macos (node 18). they all seem to be due to header being lowercase on my machine:
any idea why?
How does the code pass the check for an already applied interceptor? It seems like this is the problem we need to solve.
How does the code pass the check for an already applied interceptor? It seems like this is the problem we need to solve.
I'm doing some more digging, but the core problem/discrepancy seems to come from this scenario:
- Interceptors have been setup and they work
- MSW's
applyis executed again globalThisstate has been lost (so the global symbols are lost, and the interceptors are re-initialised).- fetch isn't affected (presumably because it's been reset along with the symbols), so MSW just re-patches it
- however, the core
node:http(s)modules are persisting the patch, which causes the Proxy-stacking.
How we end up with globalThis being reset I'm not sure. So we can narrow it down I'm trying to replicate it without including the jest/nock. Will update...
@sam-super, I think Jest contributes to this by creating a test sandbox. It may wipe out globalThis but not the module cache.
@mikicho yup just tested it and globalThis is reset between each test.
I guess it's then node env:
https://github.com/jestjs/jest/blob/main/packages/jest-environment-node/src/index.ts
So MSW's attempt to use globalThis to maintain a single instance of each interceptor wont work in jest via nodejs.
Because the FetchInterceptor is bound to globalFetch, it currently behaves differently from ClientRequestInterceptor (which is bound to the http module cache).
Ideally maybe jest should reset the native-module cache between each test to remove any state that is attached (i.e. the interceptor proxy). Not sure how feasible it will be though. From my testing jest's own jest.resetModules(); method doesn't fix the issue (the proxy persists between the tests).
I know this PR isn't ideal, but i think preventing the stacking of proxies is probably still a good thing, and if MSW errors on detecting the proxy, it might break a lot of code that uses MSW in node/jest, so updating the proxy to the latest seems the least-bad way IMO.
Hi, @sam-super. Thanks for proposing this.
I would like to start from the beginning and have an isolated failing test for this scenario. Ideally, without using Jest.
This bit concerns me:
globalThis state has been lost (so the global symbols are lost, and the interceptors are re-initialised).
If your environment gets globalThis wiped out, you have bigger problems to care about then Interceptors applying twice. May be one of those Jest shenanigans we shouldn't tailor to. This is where a good automated test is crucial.
So MSW's attempt to use globalThis to maintain a single instance of each interceptor wont work in jest via nodejs.
Do you have a test to prove this? MSW has been used with Jest for years, I don't believe this has been a problem. The root cause may be elsewhere for all I can see. MSW applies itself per process. Process cannot really change its globals. If it does, that's an unexpected behavior in whichever agent makes it so.
So, if we can reproduce this in isolation without Jest, I am up to discussing the fix. If not, I will treat this as a Jest-specific shenanigan we won't cover. MSW doesn't officially support Jest since last October.
@kettanaito I investigated this. I'm pretty sure it's because jest reset all modules except the builtin modules.
@mikicho, thanks for an update. And that happens while the test runs? Have you found the reasoning behind this behavior?
Have you found the reasoning behind this behavior?
It's jest feature to isolate tests between files in runInBand mode.
Have you found the reasoning behind this behavior?
It's
jestfeature to isolate tests between files inrunInBandmode.
I haven't needed to set runInBand to cause this issue.
@mikicho @kettanaito this is a repo i put together to demonstrate how jest resets global state and how the proxy MSW creates persists regardless: https://github.com/sam-super/msw-proxy-bug
I haven't needed to set runInBand to cause this issue.
You run one worker, it's practically the same. in both cases, jest has only one process for all test files.
I haven't needed to set runInBand to cause this issue.
You run one worker, it's practically the same. in both cases,
jesthas only one process for all test files.
It's true in this specific case, and i have set a single worker to demonstrate the issue, but because jest re-uses worker threads, i don't think you need to set --runInBand or --workers=1 to cause the issue. When a second test-suite is executed in a worker that has already been used to setup an MSW interceptor, it will exhibit the same proxy-stacking behaviour if you setup MSW again.
I didn't think the example repo (https://github.com/sam-super/msw-proxy-bug) was clear enough in what it was doing (due to the tests passing). I updated it so the tests fail in an attempted to make it clearer what is going on.
It now has an assertion based on a few incorrect assumption of global state in jest and shows how it causes MSW to create multiple proxies:
I hope it helps to clarify things.
@kettanaito given my above PR showing that globalThis is wiped out by jest (by default on nodejs), and considering MSW doesn't support jest, is it even worth me adding a test for it? I am happy to do it but I'm not sure if it's going to help get this PR over the line.
@sam-super, we do not promise any Jest support across the MSW toolchain. So if the issue only happens in Jest, that is a Jest bug and has to be addressed there. Feel free to close this pull request if that's the case! I am still grateful for your time spent investigating and working on this.