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

Undici support #2183

Open
ruiaraujo opened this issue Apr 8, 2021 · 15 comments · May be fixed by #2517
Open

Undici support #2183

ruiaraujo opened this issue Apr 8, 2021 · 15 comments · May be fixed by #2517
Labels

Comments

@ruiaraujo
Copy link

Context
NodeJS has a new vertical http client named undici which is much more performant.
Nock does not mock responses for undici users.

Alternatives
None at the moment.

Has the feature been requested before?
No results on undici search, nodejs/undici#531 has a small discussion on the mocking topic in the undici repo.
I wish to use the nock API to handle the mocking.

If the feature request is accepted, would you be willing to submit a PR?
Yes, pointer to what nock expects would be helpful.
The first step will be undici adding support for interceptors or moving the interception to a layer below (tcp level).

@gr2m
Copy link
Member

gr2m commented Apr 8, 2021

Here is the reason why it is currently not working

AFAIK nock intercept requests at Node.js built-in http module, while undici is built from scratch directly on top of Node.js built-in net module, so nock doesn't have a clue about the requests done by undici.

I think this would be quite a big effort, the whole request interception logic would need to be redone. Without looking further into it, I think this sounds great. But we should split up nock into different modules first, one being @nock/intercept, that way we will have a smaller scope to implement this feature.

A good first step would be to create a separate package as discussed in the thread you linked. Once that is working, we can incorporate it into nock.

What do you think?

@ruiaraujo
Copy link
Author

I agree, we have 2 major fronts

  1. having nock core decoupled from the interceptor method, the module approach you suggest makes sense. I guess we bootstrap this in a separate repo in the org.
  2. Working with undici team and ensuring that the capabilities required by nock are present.

Building the first helps the latter.

@gr2m
Copy link
Member

gr2m commented Apr 8, 2021

that sounds good. We discussed the decomposability of nock a while ago, we want to do it, just didn't get to it yet.

Independent of that work, the first step would be a separate module which intercepts requests at the net module level. It would be much easier to get the undici maintainers to collaborate on a small package such as that, without the legacy of nock needing to be considered or even understood

@jsumners
Copy link

For what it's worth, I think this would be quite useful. I was attempting to use nock with undici today and was initially baffled as to why the request was not being intercepted. Once I looked into the undici code and discovered it relies on bare sockets (https://github.com/nodejs/undici/blob/9a7d068cb2b8b29c2457bb6caf699295debcee44/lib/core/connect.js#L28), I understood the issue and was disappointed.

True, unidici has added a very nock-like mocking feature to its latest version. But I am currently more confident that nock has covered mocking much more thoroughly.

cc: @mcollina

@cspotcode
Copy link
Contributor

It was suggested above that nock should attempt to intercept requests at the network level.

An alternative proposal: nock can implement an undici Dispatcher

undici sends all requests to the Dispatcher to be sent over the network. nock can implement a wrapper Dispatcher to perform interceptions, delegating to undici's own dispatcher.

Could be as simple as:

undici.setGlobalDispatcher(nock.wrapUndiciDispatcher(undici.getGlobalDispatcher()));

If code decides to pass an explicit dispatcher into undici calls, they will bypass nock's wrapper. But I'm guessing the complexity of this approach is way less than attempting to proxy networking sockets and parse http headers as they pass through.

The implementation might be built on top of undici's own mocking API, if it's sufficient.

Benefit for end-users:
A single nock API for mocking that works across any code or (transitive) dependencies that call undici, request-promise, got, etc.

@gr2m
Copy link
Member

gr2m commented Jul 21, 2022

I like it, but that might be a separate project. I started working on decomposing nock but unfortunately didn't finish and then multiple family emergencies occurred: #2247

I hope to get back into it in August/September during my parental leave, but no promised ✌🏼 Once decomposed, the interceptor could be swapped out to one that supports undici, while still supporting the high-level nock APIs for mocking and assertions.

@bricss

This comment was marked as off-topic.

@jsumners

This comment was marked as off-topic.

andersk added a commit to andersk/zulipbot that referenced this issue Aug 6, 2022
We need to disable the new fetch implementation for now due to
nock/nock#2183.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
@ddolcimascolo
Copy link

Hi @gr2m and the nock community !

I was just wondering where we are regarding the ability to use nock to intercept undici and most importantly I suppose, fetch requests. Node 18 is moving to LTS this month so you can anticipate a pretty high number of users gradually moving away from node's historical http module in favor of fetch that is gaining a lot of traction these days. Not even mentioning the Axios 1.0 fiasco that makes me want to move away from this 100k stars lib 😢

Should you need any help in supporting this, just ask. nock is a great library that we use extensively, it'd be super cool to keep on using that with newer JS features !

Cheers,
David

@neurosnap
Copy link

As a temporary work-around, for anyone coming here using node v18 and nock, I just disabled node.js fetch via --no-experiemental-fetch and then relied on a fetch polyfill like normal.

travi added a commit to travi/github-scaffolder that referenced this issue Dec 23, 2022
simenandre added a commit to bjerkio/google-cloud-logger-slack that referenced this issue Apr 30, 2023
BREAKING CHANGE: Dropping axios in favor of undici might break tests. For example, if using nock. See nock/nock#2183
simenandre added a commit to bjerkio/google-cloud-logger-slack that referenced this issue Apr 30, 2023
BREAKING CHANGE: Dropping axios in favor of undici might break tests. For example, if using nock. See nock/nock#2183
simenandre added a commit to bjerkio/google-cloud-logger-slack that referenced this issue Apr 30, 2023
BREAKING CHANGE: Dropping axios in favor of undici might break tests. For example, if using nock. See nock/nock#2183
@perrin4869
Copy link

A pro-tip I found when upgrading to node 18 is that you can define per-project NODE_OPTIONS inside .npmrc:

$ cat .npmrc
node-options=--no-experimental-fetch

In case someone finds this approach useful!

@nikitaeverywhere
Copy link

nikitaeverywhere commented Jun 14, 2023

What about having a setup flag (or even set as a default behaviour) for nock to polyfill fetch with node-fetch when it's detected it is native? (using undici) fetch standard is well-defined and as both libraries implement the same standard I assume it should be acceptable as a default, giving how many test suits with nock are getting broken when upgraded to NodeJS 18.

Right now, the solution (thanks for everyone's input above!) isn't very elegant and will stop working somewhen in NodeJS 22+ I suppose, when the no-experimental-fetch flag will be dropped from node. I ended up with:

package.json

{
  "scripts": {
    "test": "mocha --node-option no-experimental-fetch -r tests/_fetch-polyfill.ts tests/**/*.test.ts ...",
  },
  ...
}

tests/_fetch-polyfill.ts

// nock doesn't support native fetch, and hence we need this polyfill.

import fetch, { Headers, Request, Response } from 'node-fetch';

if (!globalThis.fetch) {
  (globalThis as any).fetch = fetch;
  (globalThis as any).Headers = Headers;
  (globalThis as any).Request = Request;
  (globalThis as any).Response = Response;
}

@abdala
Copy link

abdala commented Nov 29, 2023

Undici has mock classes, maybe nock could just use it: nodejs/undici#587

@Uzlopak
Copy link
Member

Uzlopak commented Nov 29, 2023

Actually I worked on this, it was not easy. But if we can use MockAgent, than we can also mock native fetch

@mikicho
Copy link
Contributor

mikicho commented Feb 7, 2024

We have introduced experimental support for fetch. Please share your feedback with us. You can install it by:

npm install --save-dev nock@beta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.