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

Support Undici #159

Closed
tompeace opened this issue Oct 17, 2021 · 22 comments · Fixed by mswjs/msw#1543
Closed

Support Undici #159

tompeace opened this issue Oct 17, 2021 · 22 comments · Fixed by mswjs/msw#1543

Comments

@tompeace
Copy link

undici is a node.js library written by some core node.js contributors. It offers many great features for making fast http request that largely outperform the native http & https modules by using a customer http parser called llhttp.

MSW does not work with this library as it does not mock any of the modules covered in ./src/interceptors/ClientRequest.

undici does offer its own mocking mechanism, but I love MSW's api and am wondering if there are any plans to add an interceptor for this in the future?

For context my use case is mocking graphql apollo-datasource-http requests.

Thanks!

@kettanaito
Copy link
Member

Hey, @tompeace. Thank you for suggesting this. I've head about undici and I think we should give its interceptor a try.

Do you have a confirmation that using @mswjs/interceptors with undici indeed doesn't work at the moment? I'd be thankful if you submitted a pull request to this repository with a simple test:

  1. Configures this library as shown in the README.
  2. Makes a request using undici
  3. Asserts that the request is intercepted (will fail if we don't support this, which is expected).

You can take a look at the existing interception tests for inspiration. Once we have the test we can move forward with undici support. Let me know if you have any difficulty writing that test. Thank you!

@chentsulin
Copy link

chentsulin commented Nov 10, 2021

Hi @kettanaito @tompeace,
I created a PR #180 to include the undici tests for undici.request and undici.fetch.

I'm not sure I'm doing everything right, but It might be a good starting point to work on the undici support.

@kettanaito
Copy link
Member

Hey, @chentsulin! Thank you for your work on that. I'll take a look at the pull request as soon as I have time. I'm sure it'll act as a great foundation to introduce the Undici support itself.

@SupremeTechnopriest
Copy link

Has anyone made any headway on this interceptor?

@kettanaito
Copy link
Member

Hey, @SupremeTechnopriest. No, there hasn't been any progress on this. We've started by adding some failing tests in #180 but the implementation is still missing.

Would you be interested in working on this? I'd be happy to support you with code reviews and discussions about bringing Undici support to this library.

@SupremeTechnopriest
Copy link

SupremeTechnopriest commented Mar 1, 2022

Thinking I will take a swing at it. Has the tests been merged? Not seeing them on the main branch.

@SupremeTechnopriest
Copy link

No worries, I just pulled the tests into my fork. I Imagine you want to try to intercept as close to the core of undici as possible. Maybe the connection from the tls and net module. I will start there and work my way up stack.

@kettanaito
Copy link
Member

@SupremeTechnopriest sounds like a good starting point. I've noticed that Undici uses a mocked Agent for their recommended mocking setup. That's a nice surface indeed, but I'm afraid it lacks some vital info about the request.

Overall, we should really aim at agnostic implementation, meaning there should be 0 things relevant to Undici in how we intercept requests. Currently, we intercept ClientRequest by swapping it with a child class that has certain methods mocked (more on that in the readme). Since Undici doesn't use ClientRequest we may want to move its particular interceptor deeper. My only worry here is for the two interceptors not to conflict.

@SupremeTechnopriest
Copy link

Here is where I'm looking: https://github.com/nodejs/undici/blob/main/lib/core/connect.js

It would be nice to get our interceptor in between net and tls core node modules. Then it would be generic for any custom implementation relying on these. Similar to how the ClientRequest intercepts http and https. I need to continue to do more some research as the path to achieving this is not quite clear enough to start an implementation yet.

@kettanaito
Copy link
Member

I think net and tls are too low-level, but I don't have enough insight to conclude that. When I was diving into tls I saw that a lot of contextual information about the request gest lost, as TLS focuses on connections and data transfer while the consumer-friendly info is usually abstracted higher (in ClientRequest, for example).

A fair warning that I'm still quite new to how network requests work in Node.js so I may be saying things that are not true. This is a good opportunity for me to learn more.

@SupremeTechnopriest
Copy link

I found the same. This would be the next level up https://github.com/nodejs/undici/blob/main/lib/core/request.js but we are venturing toward an interceptor specific to undici.

@kettanaito
Copy link
Member

That seems like a custom Request class, not something we can stub as a Node.js built-in.

@varanauskas
Copy link

varanauskas commented May 2, 2022

Additionally, regarding supporting native Node.js 17+ fetch, unfortunately undici used by Node.js is currently not exposed and it is not possible to use the undici mocking functionality (Like setGlobalDispatcher)

nodejs/node#42814 (comment)


EDIT: It was fixed in node and works now

@kettanaito kettanaito changed the title add interceptor for undici Add interceptor for undici Jun 9, 2022
@jtrein
Copy link

jtrein commented Jun 22, 2022

Took me awhile to figure out why my requests were not being mocked when I upgraded a third-party package! As a workaround, I've used undici's own mock class. An example of this in use at the discord.js/rest library

@milesrichardson
Copy link

This will become a more urgent issue as Node 18 moves to active status in October. In Node 18, native fetch is enabled by default using an undici implementation. At the moment msw cannot intercept undici requests. (In fact, my attempt to upgrade to node 18 broke even interception of cross-fetch, I'm guessing because the global polyfill behavior meant most files would implicitly import native fetch).

As mentioned, undici has its own mocking API. I want to upgrade to Node 18 to get the stable native fetch that is spec-compliant so I can use stream readers. But I also want to keep using msw. It's an early codebase though, so it wouldn't be hard to migrate existing tests to use undici mock interceptors, which could increase compatibility in my primary testing environment (vitets + jsdom). But then I'd be losing the wonderful msw API and support for in-browser testing with service workers.

FWIW, re: @varanauskas comment from a few months ago, setGlobalDispatcher does work for me in latest Node 18.8. I was able to configure a proxy for my outbound requests of 127.0.0.1:8080 by including this in my setup code (e.g. vitest-setup.ts) that runs before any modules are imported:

import { ProxyAgent, setGlobalDispatcher } from "undici";
setGlobalDispatcher(new ProxyAgent("http://127.0.0.1:8080"));

milesrichardson added a commit to splitgraph/madatdata that referenced this issue Aug 25, 2022
Node 18 beomes active LTS in October, and is desirable because
it includes a native `fetch` implementation and support for web streams,
meaning that `fetch` should work the same in browser and node for almost
all functionality (this all started when looking for `body.getReader()`).

Unfortunately this breaks a bunch of stuff, like all our tests using msw to
mock HTTP API, since msw can not currently intercept requests from Undici,
see: mswjs/interceptors#159

Currently looking for best solution to this
@kettanaito
Copy link
Member

kettanaito commented Aug 26, 2022

Ideas and approaches on how to intercept Undici are welcome. But, I'd rather focus on native fetch support than Undici (read the motivation below).

I know Undici has their own mocking setup but it's too high-level to be integrated in this library. The native fetch, however, despite being powered by Undici, should be intercepted without issues as we currently patch the global fetch:

globalThis.fetch = async (input, init) => {

Intercepting Undici as a standalone solution is tricky because I have a strict policy of not introducing third-party library-specific implementations. But, as I see Undici gaining adoption in Node natively, perhaps we can skip this step altogether.

The issue with intercepting Undici requests is because Undici drops http/ClientRequest and integrates more closely with net and such, making this interception way too low-level to be reliable. Undici becomes the sole surface that knows certain request information that gets lost/becomes irrelevant on the net level.

If you'd like to contribute, here's a roadmap:

  1. Prepare an example of native fetch with the current state of @mswjs/interceptors. Writing an integration test is a great example on its own. See the existing tests for inspiration.
  2. Assess what's missing from the native fetch support. What breaks? Why?
  3. If feeling confident, try writing a prototype of a solution.

@milesrichardson
Copy link

Hey @kettanaito thanks for the reply! I actually started working on this yesterday and came to mostly the same conclusions. I realized that the reason msw wasn't intercepting native fetch out of the box was because the fetch interceptor is not included in the node preset. I started adding some tests for this (basically copying the fetch tests from browser), and it seems promising so far, but I've gotten distracted by tangentially related issues with IPv6 URL serialization (#282 )

Hopefully will have a more meaningful update / some code to share soon

@kettanaito
Copy link
Member

Oh, that's an interesting find! Yeah, since we haven't considered global fetch in Node when writing the fetch interceptor initially, it never got added to the node preset. We wrote it for the browser mainly to enable the fallback mode in MSW.

I'm going to look at that issue you've linked to unblock you in your research. Superb work, by the way! Would love to see your findings as a pull request 👀

@kettanaito kettanaito changed the title Add interceptor for undici Support Undici Aug 30, 2022
@kettanaito kettanaito pinned this issue Aug 30, 2022
@kettanaito
Copy link
Member

kettanaito commented Feb 6, 2023

Undici update

We will not support Undici directly. It's a third-party module and we have a strict policy of implementing interception to be library-agnostic.

That being said, we will support global fetch in Node, which is implemented by Undici and is available since Node 17. We will add the FetchInterceptor into the node present, making it enabled by default when using that preset.

How you can help

Please give the FetchInterceptor a try with global fetch on Node 17/18 and comment your findings on this threat. Thank you!

import { FetchInterceptor } from '@mswjs/interceptors/lib/interceptors/fetch'

const interceptor = new FetchInterceptor()
interceptor.apply()

interceptor.on('request', handler)

Please refer to README.md for the documentation on using the interceptors.

@christianvuerings
Copy link

@kettanaito That does look promising. Would there be a way to beta test this out with msw?

I did have to update the import in your example:

- import { FetchInterceptor } from '@mswjs/interceptors/lib/interceptors/FetchInterceptor'
+ import { FetchInterceptor } from "@mswjs/interceptors/lib/interceptors/fetch";

When we tried out the following code in Node.js it worked as expected

import { FetchInterceptor } from "@mswjs/interceptors/lib/interceptors/fetch";

const interceptor = new FetchInterceptor();
interceptor.apply();

interceptor.on("request", (request) => {
  console.log(request.method); // Logs `GET`
  console.log(request.url);  // Logs `http://localhost:8080/api/ips/current?viewAs=&ip=`
});

@kettanaito
Copy link
Member

We can add the Fetch interceptor to the Node preset in MSW and release this feature.

@kettanaito
Copy link
Member

Opened global fetch support in MSW: mswjs/msw#1543

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 a pull request may close this issue.

8 participants