nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.57k stars 29.58k forks source link

Support MockAgent for non-undici requests as well #51979

Closed mikicho closed 1 month ago

mikicho commented 8 months ago

What is the problem this feature will solve?

For many years Nock and other libraries have provided HTTP mocking capabilities for testing by monkey patching Node (e.g. http.request/get functions, Socket class, etc). Monkey patching is a technique used in Node to override certain functions or properties. While it can be useful, it also has its downsides. One of them is that it increases the maintenance burden on the library creators, who now have to update their library every time Node changes. Additionally, monkey patching can decrease the accuracy and reliability of these libraries.

What is the feature you are proposing to solve the problem?

Add undici MockAgent-like API for ClientRequest, which let users and library creators to intercept HTTP requests efficiently and reliably.

What alternatives have you considered?

  1. Monkey patch http.get/request functions (example)
  2. Monkey patch the Socket class (example)
  3. I have another way to solve this by setting a custom http.globalAgent, but I'm not sure it would work or be easy.
kettanaito commented 8 months ago

I would certainly love to see Node.js shipping with a built-in network observation/interception API. However, I'm not sure if the MockAgent is a good example of that. By design, MockAgent implies changing the request code to use a different dispatcher.

The only way to achieve a good experience when mocking APIs is to leave the client code intact. This is why we've spent half a decade already at MSW refining the interception of requests in Node.js.

The current state of working with the network in that regard in Node.js is a bit disjoined. We have Undici, which is the global fetch, and its MockAgent but it requires you to modify the actual request you make to use that agent. Then we have async_hooks that, technically, exposes you some info about the network, like HTTPCLIENTREQUEST, but it's (a) incomplete; (b) disjoined (a single request is split across multiple layers: DNS resolution, actual ClientRequest instance, etc).

Moreover, the network doesn't end at HTTP. If we are approaching this as a solution that'd work for years, I'd highly suggest we consider non-HTTP protocols as well. That likely means designing an API that hooks into the net.Socket directly and allows you to observe and modify the outgoing/incoming traffic in a Node.js process. Something we are currently building in MSW. I believe we can start as a singleton that the network module adds and dispatches the right events at the right time since Node.js has direct access to that time, unlike third-parties that have to hack their way around countless things just to get there.

We also have to consider the mocking capabilities of such API. It's not only about observing the network but also allowing that API to emulate connections where they'd otherwise errored/timeout/failed. This bit makes it trickier.

kettanaito commented 8 months ago

I have another way to solve this by setting a custom http.globalAgent, but I'm not sure it would work or be easy.

The custom global Agent has the same issue as MockAgent: you have to explicitly rely on it. The client can provide a custom agent instance, voiding the mock (and you have to take the custom agent into account, which is out of scope for this feature).

kettanaito commented 8 months ago

On a slightly less relevant note, we have to consider cross-environment compatibility. A third-party mocking module that relies on ClientRequest will work in any environment, be it Node.js or Deno or Bun, as long as those environments implement ClientRequest faithfully. A brand-new API such as the one proposed would require each environment to implement it anew, making this adoption a rather slow process.

github-actions[bot] commented 2 months ago

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the https://github.com/nodejs/node/labels/never-stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment. For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] commented 1 month ago

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.