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

feat: support Node.js 18 #283

Closed

Conversation

milesrichardson
Copy link

@milesrichardson milesrichardson commented Aug 29, 2022

In this PR

This PR migrates the development environment to Node v18 (which replaces v16
as active LTS in October
), and adds tests in CI for v14, v16 and v18. It fixes
compatibility issues running existing tests in a Node v18 environment and does
not break tests in earlier environments. It does not include any new user-facing features.

It does not add support for "native fetch" (if that's even missing - see #159)
but it creates the baseline for adding it, which is the next step. (I assume
this will be easy, and will hopefully only require adding new integration
tests to use the native fetch instead of the one from cross-fetch.)

It's smaller than it looks. Most of the file changes are changing import
paths in 8de6a91, which patches a bug in upstream @opendraft/test-server
by mutating and re-exporting it, hence the import path changes.

Fixes:

Makes progress on:

  • Support Undici #159
    • the next step is to add support for global fetch interception, which might not
      need undici, but that could be added as a third-party interceptor

Related:

Upstream PR:

How to review

I suggest reviewing this commit-by-commit, because unfortunately one of the
commits (8de6a91) required updating an import
path in every test file.

If you want to run this locally, make sure you switch to Node 18. You might
also need to delete node_modules to fix the memfs resolution issue
(see 1c4309e), but I'm not sure.

Here's how I do that with nvm, while retaining my global package installations:

nvm install --reinstall-packages-from=current $(cat .nvmrc)
nvm use $(cat .nvmrc)

# maybe needed?
rm -rf node_modules

yarn install --frozen-lockfile

Fix IPv6 compatibility issues

Multiple bugs shared the same root cause, which was incorrect serialization of
IPv6 hosts in URLs. This only surfaced as an issue because Node v18 now
binds to a hostname by choosing the first IP address returned by the OS,
including for the localhost domain name. Previously, this would only resolve to 127.0.0.1,
but now on some hosts (such as GitHub Actions), it resolves to ::1.

We use this IP address, which is returned by HttpServer.listen() when we bind
on localhost, to construct a URL in multiple places, mostly in tests but also
user facing (anyone using the library and binding to localhost would encounter
this same bug). The bug is caused by code that incorrectly
assumes ${scheme}://${host}:${port} serializes to a valid URL even for an IPv6
host, which should be surrounded by square brackets, like ${scheme}://[${host}]:${port}.

This bug was repeated in multiple places:

  • In this interceptors codebase
    • I fixed these bugs directly. I believe all changes should only affect tests, except for the
      two commits marked fix (eb46b83 and 22abd1c)
  • In the @opendraft/test-server codebase (dev dependency, only affects tests)
    • closed source, can be fixed upstream
    • I fixed this with a patch and re-export in 8de6a91, which is
      the commit that changes a bunch of files, because it needs to update the imports to use
      the re-exported server.
      • See the diff of OpenDraftTestServer.ts in that commit for the patch changes which
        could be replicated to the upstream codebase
    • If you fix the bugs in upstream, you should be able to simply revert this commit
      to remove the patch (I was careful to make it atomic)
  • In the page-with codebase (dev dependency, only affects tests)

@milesrichardson
Copy link
Author

Note: the change includes an edit to ci.yml to add tests for Node v18, but it looks like those new checks are not running for this PR, probably for security reasons. FYI you can see the Node v18 tests passing successfully in the run against my fork: milesforks#4

Comment on lines -43 to +49
const url = typeof input === 'string' ? input : input.url
const url =
typeof input === 'string'
? input
: input instanceof URL
? input.href
: input.url

Copy link
Author

@milesrichardson milesrichardson Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the only user-facing change, to select .href in the case that input is an instance of URL.

I don't fully grok the root cause of this - update to JSDom maybe? But I note that it fixed a type error, which was reported in my VSCode with latest TypeScript but not by any build script. It also fixed some tests, but I can't remember exactly which, since this was one of multiple bugs fixed simultaneously.

It looks like a safe change to me, though, since the new branch should only evaluate when input instanceof URL, which (AFAICT) previously could only result in an error since there is no valid url property on an instance of URL. And all tests still pass in Node v14/v16, although that's not necessarily unexpected if the root cause was an update to jest-environment-jsdom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change for type compliance only? Afaik Fetch supports URL as input. Is that no longer the case for Node.js fetch?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'm looking more closely at this now. Reverting this change does not cause any tests to fail on my local (non-IPv6) system. I also checked in GitHub Actions, and no tests failed there either. So we could revert it, but let me first check if it breaks any tests in the next branch following this one. I think it could fix a real issue that was only exposed by a TypeScript error, but I don't have any regression test to prove that.

The reason it's a type error in VSCode but not any of the build scripts is that VSCode uses version 4.7.3 while this package uses version 4.3.5, and the type error is related to the latest version of the fetch API from lib.dom.d.ts. So the type error is likely indicating a real bug, even if there are no tests for it. Likely the bug would surface in some environments (browser vs. Node) but not others.

Specifically, the error is:

Property 'url' does not exist on type 'Request | URL'.
  Property 'url' does not exist on type 'URL'.ts(2339)

as reported in VSCode:

image

Note that it's inferring the type of input as RequestInfo which it infers from the definition of globalThis.fetch in the latest TypeScript lib.dom.d.ts:

declare function fetch(input: RequestInfo | URL, init?: RequestInit): Promise<Response>;

visually:

image

Whether or not this indicates a bug at runtime depends on the fetch API provided by the environment. If we're using a subset of that API, then we should use module augmentation to keep the TypeScript API consistent with what we expect at runtime (although admittedly it won't be an issue until we upgrade the version of TypeScript used in the build scripts).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so reverting this change does have an effect on a downstream dependency - namely, the msw branch where I'm adding support for node-native interception. It causes tests to fail on IPv6 systems that were otherwise already fixed with this commit.

So, from a backwards compatibility perspective, it would be safer to revert this change. But from a forward compatibility perspective, we need it if we want to operate properly on RequestInfo objects in Node 18. The balance seems to weigh in favor of keeping the commit.

If you're okay with keeping this, then the PR is ready to merge

milesrichardson added a commit to splitgraph/madatdata that referenced this pull request Aug 31, 2022
- Basically, `msw` can mostly work with node-native `fetch` powered
by undici, but it does not currently create its lone export `createServer`
with the `FetchInterceptor` that is necessary to intercept `globalFetch`
- Clone the library locally, fix this issue, build it locally, and copy the build
files into this Yarn patch

Upstream, I've started the chain of PRs necessary to fix this:

- mswjs/interceptors#283 (migrate to Node 18)
- milesforks/msw-interceptors#5 (wip, add native fetch tests)
- Last: one line change to msw that I made in our patch here
milesrichardson added a commit to milesforks/msw that referenced this pull request Aug 31, 2022
…rver

This actually seems to work fine, even without any tests or changes
necessary. However, for tests to pass with Node v18 and also I think
to support some https cases, the PR from mswjs/interceptors should be merged:

mswjs/interceptors#283
milesrichardson added a commit to milesforks/msw that referenced this pull request Aug 31, 2022
- Point `@mswjs/interceptors` to a tarball that was built from
the code in this pending PR: mswjs/interceptors#283
milesrichardson added a commit to milesforks/msw that referenced this pull request Aug 31, 2022
…o fix IPv6 issue

- Fix bug in IPv6 URL serialization that isn't available upstream
- Source is not available, so waiting on new release

Same reasoning as: mswjs/interceptors#283
milesrichardson added a commit to milesforks/msw that referenced this pull request Sep 1, 2022
…o fix IPv6 issue

- Fix bug in IPv6 URL serialization that isn't available upstream
- Source is not available, so waiting on new release

Same reasoning as: mswjs/interceptors#283
milesrichardson added a commit to milesforks/msw that referenced this pull request Sep 1, 2022
…o fix IPv6 issue

- Fix bug in IPv6 URL serialization that isn't available upstream
- Source is not available, so waiting on new release

Same reasoning as: mswjs/interceptors#283
milesrichardson added a commit to milesforks/msw that referenced this pull request Sep 2, 2022
- Point `@mswjs/interceptors` to a tarball that was built from
the code in this pending PR: mswjs/interceptors#283
milesrichardson added a commit to milesforks/msw that referenced this pull request Sep 2, 2022
…o fix IPv6 issue

- Fix bug in IPv6 URL serialization that isn't available upstream
- Source is not available, so waiting on new release

Same reasoning as: mswjs/interceptors#283
@milesrichardson milesrichardson mentioned this pull request Sep 8, 2022
4 tasks
@kettanaito
Copy link
Member

Btw, Node 18 pipeline on CI is passing 🎉 I believe it wasn't running because all CIs need to be approved my maintainers before running. I've encountered a flaky browser test in Node 14, rerun, and now the entire CI is green.

@kettanaito kettanaito changed the title Upgrade to Node v18, part 1: fix existing tests (does not add native fetch support) chore: migrate to node v18 Sep 9, 2022
@kettanaito
Copy link
Member

I've released @open-draft/test-server@0.5.0 that adds IPv6 support similar to what you've added in page-with.

…matrix

* Update development environment (`.nvmrc`) to default to Node 18
* Update CI environment (`ci.yml`) to include Node 18 in testing matrix
* Upgrade to Jest 28, which is necessary because earlier versions do not
export the `fetch` global (among others). In Jest 28 the system for
injecting globals is changed to fix this issue.
    * see: See: https://jestjs.io/blog/2022/04/25/jest-28#all-nodejs-globals
    * also upgrade other Jest-related dependencies
@milesrichardson milesrichardson force-pushed the chore/migrate-to-node-v18 branch from 8de6a91 to 09198d9 Compare September 28, 2022 23:26
If `input` (first argument to fetch) is not a string, then
it can be a URL, which does not have a `.url` property
but does have a `.href` property

This fixes a type error and also fixes tests that
were failing in Node 18 without this fix.
…rgs`

* when intercepting requests to IPv6 hosts, rewrite `baseUrl` correctly
so that the host is surrounded with square brackets
* apply fix to `getUrlByRequestOptions` which is called by `normalizeClientRequestArgs`
* update a test that assumes `127.0.0.1` is the only valid
resolution of `localhost` to also accept `::1`
…e v18

Upgrade to the latest `page-with` package, which fixes issues with
IPv6 serialization in URLs caused by Node 18 resolving `localhost`
to IPv6 addresses on some dual-stack systems (like GHA runners)
…ibility in Node v18

Upgrade version of `@open-draft/test-server` to fix a bug in
URL serialization of IPv6 hosts, which can occur when Node v18
resolves `localhost` to an IPv6 address on systems such as GHA
@Y2zz
Copy link

Y2zz commented Nov 24, 2022

Is there a planned release time?

@kettanaito
Copy link
Member

@Y2zz, no there's not. There's a discussion above suggesting we need additional work to make this support happen. As absolutely everything that I ship, I do so if it's work personally interesting for me and I have time for it.

Anyone can help with this support, just as a few contributors above already did. Bundling the library is a standalone change and can be added and merged separately, unblocking Node 18 support.

@mattcosta7
Copy link
Contributor

@mattcosta7, I think you're right. We can add a build process to this library as well. I think tsup + esbuild like we have in MSW should be a good start. Does this sound interesting to you to work on?

I'd be happy to work on it, but not sure what level of time I have over the next month - If anyone else has time sooner, I'd be happy to offer support though!

@cdimitroulas
Copy link
Contributor

@mattcosta7 I might have some time to look into it. Could you give some guidance on what exactly we want here?

I see that in msw the tsup config has 4 main sections -> main, iife (browser), node and native + the type definitions. Do we need to follow the same kind of setup here for the interceptors lib?

@mattcosta7
Copy link
Contributor

@mattcosta7 I might have some time to look into it. Could you give some guidance on what exactly we want here?

I see that in msw the tsup config has 4 main sections -> main, iife (browser), node and native + the type definitions. Do we need to follow the same kind of setup here for the interceptors lib?

I don't think we need those specifically, but we probably need to consider separate imports (with shared code between them) for each of the individual interceptors, since they have code that might not evaluate in every environment, and a single file bundle won't necessarily shake things out.

https://github.com/mswjs/interceptors/tree/main/src/interceptors

I think we'd need to move the polyfills we want to bundle into devDependencies (like this web-std one) otherwise tsup will treat them as external.

@cdimitroulas
Copy link
Contributor

cdimitroulas commented Nov 30, 2022

@mattcosta7 first roadblack - the tsconfig.json in the interceptors lib targets ES5, but esbuild doesn't support such a target. A basic test with tsup yields various errors like "Transforming const to the configured target environment ("ES5") is not supported yet". How should we proceed here?

Edit: Even targeting ES6 results in errors such as "Transforming async generator functions to the configured target environment ("es2015") is not supported yet" (various node_modules have this problem such as @remix-run/web-fetch)

@mattcosta7
Copy link
Contributor

@mattcosta7 first roadblack - the tsconfig.json in the interceptors lib targets ES5, but esbuild doesn't support such a target. A basic test with tsup yields various errors like "Transforming const to the configured target environment ("ES5") is not supported yet". How should we proceed here?

Edit: Even targeting ES6 results in errors such as "Transforming async generator functions to the configured target environment ("es2015") is not supported yet" (various node_modules have this problem such as @remix-run/web-fetch)

I think we're probably ok to at least es2019 (or even esnext), but @kettanaito might have thoughts on what amount of downleveling we need there natively? I would edge towards minimizing that transpilation, and letting user applications do it if they need it?

cdimitroulas added a commit to cdimitroulas/interceptors that referenced this pull request Nov 30, 2022
@cdimitroulas
Copy link
Contributor

@mattcosta7 I've opened a draft PR for this work with some initial config based on our discussions above. Let's continue discussing the details of what exactly we want/need there.

cdimitroulas added a commit to cdimitroulas/interceptors that referenced this pull request Nov 30, 2022
@kettanaito
Copy link
Member

I think we're probably ok to at least es2019 (or even esnext)

I'd go for esnext 👍

@cdimitroulas, thank you for opening that pull request! I will take a look soon.

@milesrichardson
Copy link
Author

hey all - sorry for the delay on my end. I have this all working, but I haven't rebased it for a few weeks. I diagnosed the memory leak problem as being due to the number of tests being run (each test is leaking some memory for some reason), which could be fixed by just splitting it into two runs.

Otherwise, I haven't touched this since I saw some PRs in progress that mine would likely conflict with, and rebasing these PRs is pretty annoying since it touches multiple repositories

I'm not sure I will get to clean this up this week, or even if it will still be necessary. But all written in my comment above still applies.

In the meantime, if you want to do something really hacky you can use the tarball of msw built from my PR. If you're using Yarn you can just put this in your package.json:

"msw": "https://github.com/milesforks/msw/blob/released/patched/intercept-node-v18-native-fetch/unreleased-msw-1190f5560178babd232abbb9a96ce8ba4f1abfb1.tgz?raw=true",

Just to emphasize, this is really hacky and I offer no guarantees about the stability of that URL. That tarball is a build of my msw fork at this commit: https://github.com/milesforks/msw/commits/1190f5560178babd232abbb9a96ce8ba4f1abfb1 and it transitively depends on my fork of msw-interceptors via:

    "@mswjs/interceptors": "https://github.com/milesforks/msw-interceptors/blob/released/patched/pr-283-support-node-v18/msw-interceptors-6aaf5a14562f9deffc044aa3b2872e649612f957.tgz?raw=true",

which is a tarball built from https://github.com/milesforks/msw-interceptors/commits/6aaf5a14562f9deffc044aa3b2872e649612f957

@milesrichardson
Copy link
Author

Also, I just saw the comment from @kettanaito about my fork. If you still want to help with that, I am adding you as an external contributor now (to both the interceptors and msw forks). feel free to rebase whatever you need (although i'd prefer if you push to a new branch where possible, since I'm not sure where my local copy is)

cdimitroulas added a commit to cdimitroulas/interceptors that referenced this pull request Dec 6, 2022
cdimitroulas added a commit to cdimitroulas/interceptors that referenced this pull request Dec 6, 2022
@justrhysism
Copy link

Is it just the rebasing which needs to be fixed for this PR to get merged? I'm interested in pitching in to get this happening.

@kettanaito
Copy link
Member

I think we're about done with distributing this package as CJS and ESM in #313, which should be merged before this. I will rebase this branch and see if there are any loose ends.

@milesrichardson
Copy link
Author

@kettanaito Feel free to email me if you need help with this (miles [at] splitgraph.com). I don't have the bandwidth to do all the rebasing myself atm but happy to help if you get stuck with my code

kettanaito added a commit that referenced this pull request Feb 18, 2023
re #283

Bundle the library, including `remix-run` and other polyfill libs, so
that consumers of the lib can consume it without having to
bundle/transpile it themselves.

---------

Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com>
@kettanaito
Copy link
Member

Released: v0.21.0 🎉

This has been released in v0.21.0!

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.

kettanaito added a commit that referenced this pull request Feb 27, 2023
- Closes #340
- Closes to #283 (cherry-picks changes for IPv6 support from there)
@kettanaito
Copy link
Member

Addressed and merged this in #349. We are dropping support for Node < 18 from the next minor release. No more polyfills.

@kettanaito kettanaito closed this Feb 27, 2023
@kettanaito
Copy link
Member

Released: v0.22.0 🎉

This has been released in v0.22.0!

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 this pull request may close these issues.

8 participants