msw icon indicating copy to clipboard operation
msw copied to clipboard

Support Node.js v18

Open ericmasiello opened this issue 3 years ago • 3 comments

Prerequisites

Environment check

  • [X] I'm using the latest msw version
  • [X] I'm using Node.js version 14 or higher

Node.js version

v18.8.0

Reproduction repository

https://github.com/ericmasiello/msw-node-18

Reproduction steps

  1. Verify you are using node 18.8.0
  2. npm ci
  3. npm run test

Note that the tests pass but they actually should not. I'm using a toMatchInlineSnapshot(). That data snapshotted is the live data coming from real Pokemon API. what should be snapshotted is the mocked response I created in foo.test.ts

Current behavior

The tests pass because the msw is not intercepting the response. Therefore, the toMatchInlineSnapshot matches live data from the API.

Expected behavior

The tests should fail because the data should match the mocked response in foo.test.ts.

ericmasiello avatar Sep 04 '22 11:09 ericmasiello

Hey! I actually have a PR for this. Well, I have one simple PR, and then a series of more robust PRs. Let me summarize...

Background

  • Polyfilled fetch continues to be intercepted in Node 18 (mostly, I think)
  • Requests to IPv6 addresses will cause errors, regardless of fetch implementation. This might happen in tests that use localhost on an IPv6 system, because in Node 18, the resolution order has changed to prefer the order of OS resolution, instead of the previous behavior of preferring IPv4. You can revert to the previous behavior with the optional Node flag node --dns-result-order=ipv4first. This exposes the actual bug in msw (and some of its dependencies), which is incorrect URL serialization of IPv6 hosts (should be surrounded by brackets but is not, e.g. http://::1:80 should be http://[::1]:80).
  • "Node-native fetch" (aka Undici), which is enabled by default since Node 18, is not intercepted
  • The existing globalThis.fetch interceptor is able to intercept native fetch, but msw does not instantiate FetchInterceptor during createSetupServer, nor is it exported, so there is no easy way to call it from user code
  • We do not need to add any "special" interceptor for Undici, but we do need Undici as a dev dependency in the test/CI environment. Node bundles undici as internal code, but there is no way to import its package directly, so if you want to e.g. setGlobalDispatcher, you must add undici to your project and import it from there.

Quick fix

  • Draft PR in my fork: https://github.com/milesforks/msw/pull/1/files
  • Changing two lines in setupServer will make it "mostly" work.
  • This might not be the right way to make it available, since it could have impact on bundle size
  • This will not be covered by tests, and it will break with IPv6. It might also have some issues with TLS, can't remember (lots of moving parts here).
  • I wouldn't advocate merging this PR into this repository, but you might find it helpful as a quick fix in your own codebase. (For example, you could make the change to your local dependency with yarn patch or equivalent.)

Robust fix

  • Use that "quick fix," then migrate to Node 18, fix newly broken tests, and add tests for native fetch
  • For Node 18 fetch to be testable, we need to migrate the development and CI environment of this repository to Node 18
  • Migrating to Node 18 breaks tests in GitHub CI runners due (primarily) to new IPv6 resolution behavior described earlier
  • These problems unfortunately cascade through a few dependencies, including mswjs/interceptors, page-with and open-draft/test-server (source unavailable). In my PRs, I fixed these problems with hacky prototype patching, and @kettanaito also merged the fix into page-with (but still needs to release its package to npm, so I can remove the prototype patching and bump the page-with dependency version)
  • In addition to migrating this repository, I also have a PR to migrate mswjs/interceptors to Node 18: https://github.com/mswjs/interceptors/pull/283
  • I successfully migrated most of this main repository (msw) to Node v18, and added v18 to the testing matrix (along with v14 and v16), but there are still a few tests failing due to a memory leak. Here's the draft PR from my fork: https://github.com/milesforks/msw/pull/2

Summary

To fully migrate to Node 18, we need to open/merge the following PRs:

  • https://github.com/kettanaito/page-with/pull/9
  • https://github.com/mswjs/interceptors/pull/283
    • open, needs rebase and version bump of page-with
  • https://github.com/milesforks/msw/pull/2
    • unopened, draft in my fork, mostly complete, but failing a few tests due to memory leak

Note: These PRs are not as complicated/dangerous as they look. Most of the complexity only affects development/CI environment, and there are hardly any user-facing changes. And the high count of files changed is due to changing an import path to use the patched @open-draft/test-server.

In the meantime, as a "quick fix," we could make the two-line change described above (https://github.com/milesforks/msw/pull/1). If @kettanaito is okay with that approach, I can rebase my PR and open it here, or he can just make the change himself, since it's literally two lines.

Blockers

  • I need @kettanaito to release the latest version of page-with to npm so I can rebase https://github.com/mswjs/interceptors/pull/283
  • Optionally, @kettanaito needs to fix IPv6 handling in @open-draft/test-server and release the new version of it. (optional because the hacky prototype patching in my PR works fine)
  • I need to fix the last failing tests in my fork (https://github.com/milesforks/msw/pull/2) (also, @kettanaito maybe you can help me track down the source of the memory leak in the most recent GitHub Actions run?)

Next steps

I spent most of last week on this (thanks to ipv6...), so I would love to get this merged! Please let me know how you'd like to coordinate the order of merges here, and if you have any input on that last memory leak blocking my PR from being ready.

milesrichardson avatar Sep 08 '22 20:09 milesrichardson

@milesrichardson has put is perfectly. There's no Node 18 support at this moment but you can learn more about the strategy to support it above. I'm slowly getting through those pull requests to have it all in place. Be patient.

kettanaito avatar Sep 09 '22 09:09 kettanaito

The preparatory work on the Node 18 support should be done. Both page-with and @open-draft/test-server packages are bumped to support IPv6 in their server address handling. Both are internal packages but are still crucial for our testing setup.

kettanaito avatar Sep 09 '22 10:09 kettanaito

What is left for this? Node 18.12.0 is now the LTS and this is preventing us from updating.

joshuajaco avatar Oct 26 '22 07:10 joshuajaco

I would also like to understand the status of this. I want to move my testing framework to Vitest from Jest. However, Vitest is unstable on anything less than Node 18. But I can't yet move to Node 18 (or Vitest) until MSW supports Node 18.

ericmasiello avatar Oct 26 '22 10:10 ericmasiello

We also have issues with it in Node 18. Any updates?

max-sym avatar Nov 01 '22 11:11 max-sym

Are there viable alternatives to MSW that are Node18 compatible?

ericmasiello avatar Nov 01 '22 16:11 ericmasiello

I was actually able to solve it using cross-fetch. Tried node-fetch too, but it was buggy as well 🤦‍♂️

max-sym avatar Nov 01 '22 16:11 max-sym

I was actually able to solve it using cross-fetch. Tried node-fetch too, but it was buggy as well 🤦‍♂️

also works for node 19. thanks ❤️

tobilen avatar Nov 09 '22 13:11 tobilen

I was actually able to solve it using cross-fetch. Tried node-fetch too, but it was buggy as well 🤦‍♂️

Can you point to an example of what it means to use cross-fetch with msw?

ericmasiello avatar Nov 09 '22 15:11 ericmasiello

use the fetch from this library instead of the native one from node

import { setupServer, rest } from "msw/node";
import fetch from "cross-fetch"; // <-- this is what it means

// application code
const myFetch = async () => {
  const response = await fetch("https://some-url.com");
  return response.json();
};


// test code
const server = setupServer(
  rest.get("https://some-url.com", (req, res, ctx) =>
    res(ctx.status(200), ctx.json({ someKey: "some value" }))
  )
);

test("msw", async () => {
  server.listen();

  expect(await myFetch()).toEqual({ someKey: "some value" });
});

tobilen avatar Nov 09 '22 15:11 tobilen

Thank you, @tobilen. Ill give this a try.

ericmasiello avatar Nov 09 '22 15:11 ericmasiello

For those looking for another workaround on Node 18 until this issue is completed, you can do this in your package.json:

"scripts": {
    "dev": "NODE_OPTIONS='--no-experimental-fetch'  node server.js", // or any script to start your server
    ...
   }

Adding this Node flag will use the old Node fetch (fetch from node version < 18)

yairkukielkazunzunegui avatar Dec 09 '22 14:12 yairkukielkazunzunegui

I was actually able to solve it using cross-fetch. Tried node-fetch too, but it was buggy as well 🤦‍♂️

cross-fetch uses [email protected] under the hood, so downgrading to [email protected] also works.

matt-pawley avatar Jan 09 '23 15:01 matt-pawley

I was actually able to solve it using cross-fetch. Tried node-fetch too, but it was buggy as well 🤦‍♂️

cross-fetch uses [email protected] under the hood, so downgrading to [email protected] also works.

This is very curious to me. I don't see anything obvious between v2 and v3 on node-fetch that would have broken this implementation.

I guess why does that work, is my real question.

austin-thrive avatar Jan 09 '23 19:01 austin-thrive

Wasn't the point of Node 18 if we're not removing polyfills? We use whatwg-fetch and msw works fine with Node 18. However, with fetch being native now to Node, we'd like to remove that dependency if possible.

rodoabad avatar Jan 13 '23 16:01 rodoabad

looks like its more then 4 months since this issue of node 18 .. meaning this project is abandoned? not beeing able to use msw with latest stuff makes it kind of absolute right even pr's not beeing merged..

pascalvos avatar Jan 23 '23 14:01 pascalvos

looks like its more then 4 months since this issue of node 18 .. meaning this project is abandoned? not beeing able to use msw with latest stuff makes it kind of absolute right even pr's not beeing merged..

I wouldn't quite go that far. There was a commit 4 days ago.

ericmasiello avatar Jan 23 '23 14:01 ericmasiello

yea thats true sorry.. , still this is big problem making creating new project or updating to latest lts of node .. it not being supported leaves mswjs in the dark

pascalvos avatar Jan 23 '23 14:01 pascalvos

@kettanaito do you mind commenting on this thread? Are there plans for a quick or robust fix? Or is it the intent that consumers use polyfills for the long haul?

ericmasiello avatar Jan 23 '23 14:01 ericmasiello

i dont mind polyfills if there is a proper solution on how to work with this, now this is stuck

pascalvos avatar Jan 23 '23 14:01 pascalvos

@rodoabad can your share your setup?

vospascal avatar Jan 23 '23 17:01 vospascal

It's not abandoned, there are just 3 different branches that need to be rebased and merged all over the place and I haven't had the bandwidth to do it myself yet.

If you want a quick hack workaround, see my comment here: https://github.com/mswjs/interceptors/pull/283#issuecomment-1333954204

milesrichardson avatar Jan 24 '23 00:01 milesrichardson

@milesrichardson if i read correctly @kettanaito is pretty close on the ball there ill keep an eye on it i can pospone a bit wih this thank you for clearing this up that helps a lot :)

pascalvos avatar Jan 24 '23 09:01 pascalvos

Update

My bad for letting this thread derail for so long. Posting an update on Node 18 support in MSW below.

https://github.com/mswjs/interceptors/pull/283 addresses the most part (if not all) of that support in the Interceptors library. Things remaining:

  • Review those changes (anybody's welcome to help me here).
  • Test those changes with the current stable build of MSW ("latest" on NPM). Gather any issues and address those as a part of that pull request.

Roadmap

I once again remind you to see the Roadmap that @milesrichardson has written down. It's still relevant and you can follow dependent issues referenced there.

Answering questions

@pascalvos: looks like its more then 4 months since this issue of node 18 .. meaning this project is abandoned?

No, the project is not abandoned. It's just maintained by a single person with a full-time job and quite a number of other interests in his life. I'm doing my best but if you'd like me to dedicate more effort to MSW, I'd be thankful for Sponsoring the project. Maybe one day I will be able to move forward with this project with a faster pace but that day is not today.

@milesrichardson has put an incredible amount of effort into this but he's also a person and he approaches open source contributions with the pace and extent that are comfortable to him. I'm thankful for his contributions and deeply respect that because I'm doing the exact same thing.

Also, please bear in mind that the lack of official Node 18 support doesn't mean MSW won't work on Node 18. I know a few projects where it works without issues on Node 18. It really depends on the APIs you're using (for instance, it doesn't support the global fetch at the moment) so I highly encourage you to give it a try before concluding that it blocks you.

@austin-thrive: This is very curious to me. I don't see anything obvious between v2 and v3 on node-fetch that would have broken this implementation.

node-fetch@3 introduces a breaking change by becoming ESM-only. This breaks the package pretty much everywhere you wanted to use it. For example, the latest Jest won't even run node-fetch because Jest has no support for ESM at the moment. I was preparing an example repo with node-fetch and MSW recently and had to downgrade to node-fetch@2 for the things to work. This may be the reason you experience a different behavior between these two major versions.

@ericmasiello: Are there plans for a quick or robust fix?

Not really. The plan is to go through all the open related pull requests, review and rebase them, test them on the current MSW build in Node 18 and then merge and release them if everything goes smoothly.

@ericmasiello: is it the intent that consumers use polyfills for the long haul?

The intention is that MSW should execute in Node 18 without issues, including the support for the global fetch there. You shouldn't be needing a Fetch polyfill when using the global fetch from Node. You will need a polyfill, just as you do now, when using older versions of Node, which is 14 and 16.

kettanaito avatar Jan 24 '23 10:01 kettanaito

Thank you for the detailed reply this helps i think every one looking in to this issue. And im sorry for poking the bear i understand the OSS challenge i also do much in this regard (other projects) where i can. I didnt know there was only one/two maintainers on this :)

thank you for your time and effort

pascalvos avatar Jan 24 '23 13:01 pascalvos

Update

MSW v2.0 will drop Node.js < 18 and will ship with Node.js 18+ support. This also includes proper ESM support that people have been requesting for quite some time now.

kettanaito avatar May 12 '23 10:05 kettanaito

Do you have an ETA please? 🙂

ValentinH avatar May 12 '23 11:05 ValentinH

@ValentinH, as soon as it's sufficiently tested. You can contribute to that by giving the next version of MSW a try. I'd aim for sometime in late summer as I won't have much availability in the upcoming months.

You can adopt msw@next even now. The release candidate is stable and this is very similar, if not identical, to what we will ship in v2.0. You don't have to wait.

kettanaito avatar May 12 '23 12:05 kettanaito

Thanks for the answer. I missed this info, I'll give the next version a try next week then 🤞

ValentinH avatar May 12 '23 13:05 ValentinH