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

Attempt to add undici interceptor #3

Closed
wants to merge 21 commits into from

Conversation

milesrichardson
Copy link

wip

The tests appear to pass, but there is a new console.error produced in
the browser tests, apparently due to page-with
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: https://jestjs.io/blog/2022/04/25/jest-28#all-nodejs-globals

Also upgrade related dependencies and update import path
of `AsymmetricMatchers` which was moved in this version.
Starting in Node v18, Node resolves an address (like `localhost`)
by returning IP addresses in the order they are returned from the
OS resolver/DNS. [0] This means that on default-IPv6 systems like
a GitHub Actions runner, the link-local address `::1` is returned
instead of `127.0.0.1`.

Unfortunately this new behavior causes a bug in `@open-draft/test-server`,
which serializes an IPv6 host into an invalid URL, because it
incorrectly does not surround the host with square brackets
like it should (e.g. `http://[::1]:8181`).

Since I could not locate the source of `@open-draft/test-server`
to fix this bug, the work-around is to replace all references
to `localhost` with `127.0.0.1` directly - AFAICT there is no
good reason to test this resolution behavior anyway.

[0] nodejs/node#40537
Replace all imports _in test files_ that imported `@open-draft/http-server`
with the `TestHttpServer` shim that applies the bugfix to
serialize IPv6 hosts into valid URLs.

Note this does not replace any imports in `src/`, of which there
are two, but they import `until` from `@open-draft`, and do not
use it for creating any `HttpServer` instance
@milesrichardson milesrichardson force-pushed the node18-undici-compat branch 7 times, most recently from 2f2862f to 9884938 Compare August 26, 2022 00:13
@milesrichardson milesrichardson force-pushed the node18-undici-compat branch 2 times, most recently from c391749 to 9605d42 Compare August 26, 2022 00:42
@milesrichardson milesrichardson force-pushed the node18-undici-compat branch 9 times, most recently from ee3f9ff to 6f20a12 Compare August 26, 2022 04:47
@milesrichardson milesrichardson force-pushed the node18-undici-compat branch 2 times, most recently from 226378e to e565011 Compare August 26, 2022 05:12
…ndency

The `page-with` package introduces a transitive dependency on `memfs`,
and specifies a semver range that excludes the latest version of `memfs`
which is compatible with Node v18.

Use the `resolutions` field of `package.json` to force Yarn classic
to install the (currently) latest version of `memfs` regardless of
semver range requested by `page-with`

This fixes at least one bug in `memfs` running in Node v18, which
caused excessive `console.error` spam which rightfully complained
of leaking file descriptors.

For docs on `resolutions` field behavior of Yarn classic, see:
https://classic.yarnpkg.com/en/docs/selective-version-resolutions
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.

1 participant