feat: fix IPv6 url serialization, use node 18#9
Conversation
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
|
Note: see my fork for example of successful test run: milesforks#1 |
| // 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 |
There was a problem hiding this comment.
Nitpick: a rather complex expression. Consider:
const host = family === 'IPv6' ? `[${address}]` : address
const url = `http://${host}:${port}`There was a problem hiding this comment.
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.
| port, | ||
| host: address, | ||
| url, | ||
| family, |
There was a problem hiding this comment.
Who/how is this family be consumed further down the chain?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@milesrichardson, thanks for all your work on this. I'm also addressing related things in mswjs/interceptors#354.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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).
a39788c to
22d2ce6
Compare
|
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
|
Thank you for making this library better, @milesrichardson! 🎉 I will introduce this change in |
|
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 tl;dr Adding the |
…ssue Patch `page-with` IPv6 bug until upstream is merged Tracking: kettanaito/page-with#9
|
@milesrichardson, I've just released 0.6.0. Thank you for making this happen. |
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/interceptorsdescribed 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-withpackage, 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.