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

Node 17 and 18 silently don't get intercepted #246

Closed
jqpe opened this issue May 22, 2022 · 6 comments · Fixed by #269
Closed

Node 17 and 18 silently don't get intercepted #246

jqpe opened this issue May 22, 2022 · 6 comments · Fixed by #269

Comments

@jqpe
Copy link

jqpe commented May 22, 2022

Node 17 comes with the --experimental-fetch flag and from Node 18 this flag is enabled by default.

Under the hood Node's fetch uses undici. There has been work towards implementing undici intercepting here: #159.

Running Node 18 or Node 17 with the --experimental-fetch flag (and using global fetch), depending on your requests, everything seems to work, but instead of requests being intercepted you are hitting actual endpoints.

Moreover, server.listen({ onUnhandledRequest: 'error' }) doesn't error as expected.


There should be documentation about this behavior before undici interception lands. Alternatively or additionally there should be an error about using Node's native fetch.

From searching the issues I've seen that the node interceptor is intended to work up to Node 16, but there is neither an engines restriction or sufficient documentation about this limitation.

Reproduction:
https://github.com/jqpe/msw-node-18

  • to test with node 17 --experimental fetch run this instead
    NODE_OPTIONS=--experimental-fetch yarn --ignore-engines test

I used vitest to run the tests here, but due to the nature of this issue the test runner shouldn't matter.


Workaround
I also included a workaround, instead of using the native fetch overwrite globalThis with node-fetch's implementation, the tests should pass if you run yarn test:workaround with supported Node versions.

https://github.com/jqpe/msw-node-18/commit/44bcbf075bc98f09323c3345db52feb12c7e86de

@kettanaito
Copy link
Member

Hey, @jqpe.

There should be documentation about this behavior before undici interception lands.

Makes sense. I'd want to prevent confusion and clearly state that this library does not support Node.js <= 17.

From searching the issues I've seen that the node interceptor is intended to work up to Node 16, but there is neither an engines restriction or sufficient documentation about this limitation.

We can limit that in the engines property in the package.json. The issue with this approach is that change will halt installation for all consumers on Node.js 17+. But the only thing we don't support is Undici, and thus global fetch. I believe ClientRequest will still be intercepted correctly in any version of Node.js, including 17 and 18.

@jqpe
Copy link
Author

jqpe commented May 23, 2022

Yeah, I agree with your point about the changing the engines not being ideal. In my repro node-fetch works as expected in Node 18. I also ran this library's test suite with Node 18 and everything expect test/features/remote/remote.test.ts appears to pass.

To clarify, this issue is not about Node 18 or Node 17 with --experimental-fetch support, but rather providing some feedback or documentation about what is intercepted. As (native) fetch support is mentioned in the readme, requests not being intercepted and hitting actual URLs is obviously undesirable.

The fetch interceptor is for browser, but as the current Node now comes with global fetch enabled by default I think this should be clearly distinguished.

@kettanaito
Copy link
Member

The next action we can take from this discussion is:

  • Make sure the fetch mentioned in the README relates to the window.fetch, not the global fetch in recent versions of Node.js.

The fetch interceptor is for browser, but as the current Node now comes with global fetch enabled by default I think this should be clearly distinguished.

Will the clarification above help to distinguish this?

Once again, the library cannot assume that you will run into issues with Node 17/18 because it doesn't spy on the exact features you may be using. This reads like an issue with establishing a promise from the library, which the window.fetch clarification should resolve.

@kettanaito
Copy link
Member

I don't have any tests to prove this but since 0.15.2 we should be able to intercept native fetch requests in Node.js as well (#242). As long as fetch is exposed as globalThis.fetch in Node 17/18.

Could you please verify this? I can't access your reproduction repo anymore, looks like you've deleted it.

I will close the issue but still add the window.fetch clarification to the README.

@jqpe
Copy link
Author

jqpe commented Jul 4, 2022

Hey @kettanaito, cheers on updating the readme! I've restored the repro and out of interest checked if it would work on the most recent version of msw (seems to depend on 0.16.6. of this library) Unfortunately changing from window to globalThis doesn't fix the native Node.js fetch issue as it uses undici and not XMLHttpRequest and whatnot.

As is, if people on recent versions of Node.js (probably increasing amounts as there's stuff going on with OpenSSL) they will need to override the native undici fetch with a polyfill like node-fetch that use Node.js APIs this library supports.

@kettanaito
Copy link
Member

Released: v0.17.2 🎉

This has been released in v0.17.2!

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
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants