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

Draft: Add node v18 support with tests (WIP): use patched @mswjs/interceptors #2

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

milesrichardson
Copy link

This is based on #1 but assumes that mswjs/interceptors#283 has been merged. Currently it points to a tarball built from the code in that PR.

@milesrichardson
Copy link
Author

milesrichardson commented Aug 31, 2022

@milesrichardson milesrichardson changed the title Add node v18 support with tests (WIP): use patched @mswjs/interceptors Draft: Add node v18 support with tests (WIP): use patched @mswjs/interceptors Aug 31, 2022
@milesrichardson milesrichardson force-pushed the intercept-node-v18-native-fetch branch from 89a4902 to 3e5df40 Compare September 1, 2022 05:06
@milesrichardson milesrichardson force-pushed the intercept-node-v18-native-fetch branch 2 times, most recently from 8076ddf to f445fc1 Compare September 2, 2022 04:56
@milesrichardson milesrichardson mentioned this pull request Sep 8, 2022
4 tasks
…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
- Point `@mswjs/interceptors` to a tarball that was built from
the code in this pending PR: milesforks/msw-interceptors#5

Using commit:

milesforks/msw-interceptors@25a8ffa
…terface

* Upgrade to the latest available version of `@open-draft/test-server`,
which replaces `async createServer()` closure factory with `new HttpServer()`
and `async HttpServer.prototype.listen()`. That is, previously we awaited
a single function to create the server and start listening, but now we
create the server with a synchronous function and await its listener separately.

* Also (in a rebased version of this commit), upgrade to yet another version
of `@open-draft/test-server` which fixes URL serialization of IPv6 hosts

* Change all affected tests to use this new instantiation method
This fixes a bug in tests where the preview server is bound to an IPv6
URL, which causes the matcher to fail when comparing the URL to origin,
which causes the request to send wthout a `referer` header, which causes
an error in the service worker and incorrect error snapshots (e.g. using
full URL instead of relative URL because no match was found).

Fix:

* When escaping colons in path before passing to `path-to-regexp`,
make sure to include colons in the host, but only if it's a bracketed
IPv6 host (this means that you can use wildcards in a non-bracketed
host if you really want to, e.g. `:subdomain.foo.com`)

* Combine previously multiple regexes for scheme and port into a single
regex for scheme, port, and host (since their logic is interdependent)

* Add tests for various convoluted IPv6 scenarios and wildcard placements
It's useful to run tests in debug mode because it opens a live ("headful")
Playwright browser, e.g. `DEBUG=1 yarn test:integration passthrough` will
run the `passthrough.test.ts` tests in a headful Chromium instance

However, the live debug browser behaves slightly differently from
the headless browser, because there is a navigation request to the
page where the service worker has already been loaded, which causes
the service worker to intercept the initial navigation request.

This is a dangerous trap if you're trying to fix a test by running
it in debug mode. If this is detected, print a friendly warning to
the developer to hopefully save them some time.
… `waitForResponse`

Fix a bug where `waitForRequest` and `waitForResponse` would
hang and never match a request when the URL is an IPv6 URL
@milesrichardson milesrichardson force-pushed the intercept-node-v18-native-fetch branch from 5e9a2f9 to 4ded61d Compare September 29, 2022 02:15
milesrichardson added a commit to splitgraph/madatdata that referenced this pull request Sep 29, 2022
Use the package built temporarily as a tarball
from pending/WIP PR at milesforks/msw#2
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