feat: fix IPv6 url serialization, use node 18 #9

milesrichardson · 2022-08-29T18:58:58Z

This PR upgrades the development environment to Node v18 (which will replace v16 as Active LTS in October) and adds CI tests for Node v14, v16 and v18. It fixes the bugs in Node v18 affecting downstream package mswjs/interceptors described in mswjs/interceptors#282 and patched in mswjs/interceptors#283.

I believe this also closes #8 (fixes the underlying bug so there is no need for it).

This PR does not update the version for release, since I'm not sure the process for that. Once you merge it, and release a new version of the page-with package, you can update mswjs/interceptors#283 to revert the commit that patches over this bug. You could also revert the commit pinning memfs, since that will be a no-op once this package depends on the latest version of it.

See commits for more details. The bugfix is in 3e48ce2 and the rest of the commits are fixing compatibility issues and resolving some warnings.

AFAICT, this PR contains no breaking changes.

Use modern GitHub Actions workflows for installing Node and running `yarn install`

* Upgrade the dev environment to Node 18.8 by adding a `.nvmrc` file which did not previously exist (we're testing in all of Node 14, 16, 18, so the development environment may as well be the latest one) * Upgrade `memfs` to latest major version, to fix a bug that occurs in Node 18 causing warning spam and file descriptor leaks

milesrichardson · 2022-08-29T19:01:10Z

Note: see my fork for example of successful test run: milesforks#1

kettanaito · 2022-08-30T10:02:14Z

src/server/PreviewServer.ts

+        // IPv6 host requires surrounding square brackets when serialized to URL
+        // note: IPv6 host can take many forms, e.g. `::` and `::1` are both ok
+        const url = `http://${
+          family === 'IPv6' ? `[${address}]` : address


Nitpick: a rather complex expression. Consider:

const host = family === 'IPv6' ? `[${address}]` : address const url = `http://${host}:${port}`

Good idea! I've implemented your suggestion, except I named the variable "serializedAddress," which seems more contextually appropriate, especially given the existing host variable in the outer scope, set by the second argument to listen (default "localhost").

I modified the existing commit and force pushed the change. Assuming the tests pass, I think this PR is ready to merge.

kettanaito · 2022-08-30T10:02:34Z

src/server/PreviewServer.ts

        this.connectionInfo = {
          port,
          host: address,
          url,
+          family,


Who/how is this family be consumed further down the chain?

Ironically, the code that could benefit from it in mswjs/interceptors#283 is the patch to page-info that won't be necessary once this is merged 😸 I think the patch for @open-draft/test-server could also use it, but I don't remember the exact code path.

Basically, any logic that needs to differentiate between IPv6 and IPv4 based on the URL currently "guesses" with logic like isIPv6 = address.includes(":"). So returning the family here simply preserves information that we already have available from the AddressInfo object returned by listen.

It's not strictly necessary but makes life easier downstream and shouldn't break anything. I guess it could break somebody's snapshot tests or something, but that would be surprising (especially since this.connectionInfo is never actually returned - in the patch, I accessed it by exporting the mutable server in the browser integration tests setup function).

If you want to remove this, feel free to revert this commit (95f78f0) (or let me know your decision and I can force push the change).

@milesrichardson, thanks for all your work on this. I'm also addressing related things in mswjs/interceptors#354.

Awesome! Sorry for all the back and forth (or no forth, as the case may be 👀). Let me know if I can help.

I haven't tried the latest version of msw in my Node 18 project yet (still using the hacky tarball I built), but I'll try switching it soon. (btw, that project is now open source, if you want to see how msw is used there: https://github.com/splitgraph/madatdata)

Last I remember, everything was good to go. There were some failing tests, but the reason was because of a memory leak in the test runner, and the solution (which I didn't implement) would be to split it up into two calls to the test runner (I couldn't fix the memory leak after two days of trying).

I'm assuming you won't be using the code I wrote directly, so maybe that won't matter to you, but you can at least be assured that the latest commit in each respective project was known to be working at least once. Feel free to copy paste 😅

I think this diff should contain the few necessary fixes to IPv6 serialization in msw-interceptors: https://github.com/milesforks/msw-interceptors/pull/6/files

And this is where the bulk of the work on msw was. This is the one where tests are only failing because of the memory leak, but otherwise everything else works. There is some IPv6 stuff in here too: https://github.com/milesforks/msw/pull/2/files

The commit messages are pretty detailed, e.g. milesforks/msw@b57db77 ("fix(coercePath): widen regex to escape : in IPv6 address as well")

kettanaito

Thanks for seeing this through, @milesrichardson!

I think the changes are good. I've left one nitpick but leave it up to you to resolve it or not (just ping me in any case so we could merge this).

* Ensure IPv6 host is surrounded by square brackets when serialized into a URL * Use the Node-provided `family` property of the address returned by `.listen()` to check whether the address is IPv6, instead of relying on guesswork of looking at the IP

…` object This is not strictly necessary for any bugfix, but it will make life easier for code that consumes the connectionInfo object, which would otherwise need to parse `connectionInfo.address` itself and/or assume that any address containing `:` is an IPv6 address (which is probably a valid assumption, albeit not a robust one).

milesrichardson · 2022-08-30T18:13:00Z

Thanks for the review! I addressed your nitpick and answered your question. :)

Since I force pushed the change, you need to approve the workflow run again, but assuming all tests pass, then it's ready to merge IMO

…ssue Patch `page-with` IPv6 bug until upstream is merged Tracking: kettanaito/page-with#9

kettanaito · 2022-09-01T12:38:39Z

Thank you for making this library better, @milesrichardson! 🎉

I will introduce this change in test-server as well.

milesrichardson · 2022-09-01T17:50:35Z

Awesome, thanks! Let me know when the new npm package is released and I'll swap out the patch from mswjs/interceptors#283

p.s. I'm very close to having msw itself fully migrated to Node 18 and intercepting native fetch by default. I'll open a PR when it's ready but you can follow here milesforks/msw#2

tl;dr Adding the FetchInterceptor is easy. Upgrading to Node v18 so that we can actually test its behavior is not so easy - basically due to IPv6 issues (which I suppose would be present in earlier versions too). I've fixed those though and am now working on the last few errors

…ssue Patch `page-with` IPv6 bug until upstream is merged Tracking: kettanaito/page-with#9

kettanaito · 2022-09-09T09:45:40Z

@milesrichardson, I've just released 0.6.0. Thank you for making this happen.

milesrichardson added 2 commits August 29, 2022 19:17

ci(build): modernize actions workflow and test node 14, 16, 18

c8ea6d0

Use modern GitHub Actions workflows for installing Node and running `yarn install`

milesrichardson mentioned this pull request Aug 29, 2022

Click here to see successful tests (same PR, but in my fork, where I can run checks in GitHub Actions) milesforks/page-with#1

Closed

This was referenced Aug 29, 2022

Where is the source code for page-with and @open-draft/test-server packages? mswjs/interceptors#282

Closed

feat: support Node.js 18 mswjs/interceptors#283

Closed

kettanaito reviewed Aug 30, 2022

View reviewed changes

kettanaito approved these changes Aug 30, 2022

View reviewed changes

milesrichardson added 3 commits August 30, 2022 19:02

chore(lint): fix eslint warning (@typescript-eslint/no-explicit-any)

22d2ce6

milesrichardson force-pushed the chore/fix-ipv6-and-node-18 branch from a39788c to 22d2ce6 Compare August 30, 2022 18:04

milesrichardson added a commit to milesforks/msw that referenced this pull request Aug 31, 2022

test(browser integration): patch upstream PreviewServer to fix IPv6 i…

e351393

…ssue Patch `page-with` IPv6 bug until upstream is merged Tracking: kettanaito/page-with#9

kettanaito approved these changes Sep 1, 2022

View reviewed changes

kettanaito changed the title ~~Upgrade to Node v18; fix IPv6 URL serialization; add tests for Node 14, 16 and 18~~ feat: fix IPv6 url serialization, use node 18 Sep 1, 2022

kettanaito merged commit ac45b06 into kettanaito:main Sep 1, 2022

milesrichardson added a commit to milesforks/msw that referenced this pull request Sep 2, 2022

test(browser integration): patch upstream PreviewServer to fix IPv6 i…

4c0e774

…ssue Patch `page-with` IPv6 bug until upstream is merged Tracking: kettanaito/page-with#9

milesrichardson mentioned this pull request Sep 8, 2022

Support Node.js v18 mswjs/msw#1388

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fix IPv6 url serialization, use node 18 #9

feat: fix IPv6 url serialization, use node 18 #9

milesrichardson commented Aug 29, 2022 •

edited

Loading

milesrichardson commented Aug 29, 2022

kettanaito Aug 30, 2022

milesrichardson Aug 30, 2022

kettanaito Aug 30, 2022

milesrichardson Aug 30, 2022

kettanaito Mar 1, 2023

milesrichardson Mar 1, 2023

milesrichardson Mar 1, 2023

milesrichardson Mar 1, 2023 •

edited

Loading

kettanaito left a comment

milesrichardson commented Aug 30, 2022

kettanaito commented Sep 1, 2022

milesrichardson commented Sep 1, 2022

kettanaito commented Sep 9, 2022

feat: fix IPv6 url serialization, use node 18 #9

feat: fix IPv6 url serialization, use node 18 #9

Conversation

milesrichardson commented Aug 29, 2022 • edited Loading

milesrichardson commented Aug 29, 2022

kettanaito Aug 30, 2022

Choose a reason for hiding this comment

milesrichardson Aug 30, 2022

Choose a reason for hiding this comment

kettanaito Aug 30, 2022

Choose a reason for hiding this comment

milesrichardson Aug 30, 2022

Choose a reason for hiding this comment

kettanaito Mar 1, 2023

Choose a reason for hiding this comment

milesrichardson Mar 1, 2023

Choose a reason for hiding this comment

milesrichardson Mar 1, 2023

Choose a reason for hiding this comment

milesrichardson Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

kettanaito left a comment

Choose a reason for hiding this comment

milesrichardson commented Aug 30, 2022

kettanaito commented Sep 1, 2022

milesrichardson commented Sep 1, 2022

kettanaito commented Sep 9, 2022

milesrichardson commented Aug 29, 2022 •

edited

Loading

milesrichardson Mar 1, 2023 •

edited

Loading