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

Make it possible to use Fetch with proxies or other agents #42814

Closed
Tracked by #1
pimterry opened this issue Apr 21, 2022 · 34 comments · Fixed by #43059
Closed
Tracked by #1

Make it possible to use Fetch with proxies or other agents #42814

pimterry opened this issue Apr 21, 2022 · 34 comments · Fixed by #43059
Labels
feature request Issues that request new features to be added to Node.js. fetch Issues and PRs related to the Fetch API

Comments

@pimterry
Copy link
Member

What is the problem this feature will solve?

The new fetch API as implemented cannot be used with an HTTP proxy, which is required for connectivity in many environments.

For normal HTTP this is implemented via agents, but there's no way to use any agents with this fetch API. Many of us working in environments with proxies use libraries like global-agent which set node's globalAgent to a proxy agent based on the system settings to automatically configure all libraries, but that doesn't work with fetch either.

Notably, this means the docs are wrong right now, when they say:

http.globalAgent: Global instance of Agent which is used as the default for all HTTP client requests.

This agent is not used for HTTP client requests if you use the fetch API.

Node's fetch implementation comes from Undici, and although Undici doesn't offer an explicit way to set this per fetch request (see nodejs/undici#1350) it does offer an agent-equivalent dispatcher option on all other request methods, and a setGlobalDispatcher method to configure a dispatcher globally (like node's globalAgent) which does work for fetch.

That means it is possible to use proxies with Undici's fetch right now, but not in Node as this isn't exposed anywhere (AFAICT).

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

  • Provide a way to specify a Node.js agent or a Undici dispatcher for a fetch request
  • Provide a way to set a global agent/dispatcher, which applies to all HTTP & fetch requests

Since they're very closely related, it seems like it would be sensible to aim to move everything to either agents or dispatchers for all HTTP APIs in future. While fetch is experimental though it seems reasonable to me to implement this only with Undici's existing dispatchers for now and pick one direction or the other to commonize later.

What alternatives have you considered?

As far as I can tell, there's currently no alternative or workaround available to use proxies with fetch in Node. If you need to use an HTTP proxy for connectivity, the current fetch API is unusable.

This is particularly bad because some libraries that support both browsers & node will use the fetch global automatically when available or node-fetch otherwise (which uses http internally) for their requests. Although it used to be possible to use these libraries in a proxy environment by using global-agent or passing an agent explicitly, it's now impossible to use these libraries at all, because they use the new fetch global which ignores all agent configuration.

@pimterry pimterry added the feature request Issues that request new features to be added to Node.js. label Apr 21, 2022
@targos
Copy link
Member

targos commented Apr 21, 2022

@nodejs/undici

@VoltrexKeyva VoltrexKeyva added the fetch Issues and PRs related to the Fetch API label Apr 21, 2022
@panva
Copy link
Member

panva commented Apr 21, 2022

Note: the same applies to undici's mocking ability and common userland modules that achieve mocking for node's http/https modules nock. Undici has the affordance, but it is not exposed in Node.js and so node's global fetch cannot be mocked.

@mcollina
Copy link
Member

fetch in Node.js is experimental, all of this is planned for when it exits experimental. Could you check out undici and see if it matches your expectations?

On a different note, would it be easier to detect if the global fetch property was non-enumerable?

@pimterry
Copy link
Member Author

Could you check out undici and see if it matches your expectations?

Undici's dispatchers & setGlobalDispatcher on the core request methods (request/stream/etc) do work great for me, yes.

Undici's fetch doesn't have its own dispatcher option available though (nodejs/undici#1350), which is a bit inconvenient (I'm mostly setting a global dispatcher anyway, so in my case it's not a big issue).

It would be a little more useful if it directly supported agents, or there was a dispatcher<->agent compatibility wrapper, since there's so many existing node.js agent implementations that could be reused, but again not a big deal.

On a different note, would it be easier to detect if the global fetch property was non-enumerable?

I don't think enumerability matters here, most checks are just if (!global.fetch) { addPolyfill() } e.g. in isometric-fetch and cross-fetch.

Those libraries are quite widely used (5 million + 8 million installs a week via npm, plus other similar libs & inline checks elsewhere). Everybody using those with Node 18 is automatically using this new experimental implementation everywhere now.

That's how I ran into this myself: my test code had the same inline if-no-fetch-then-ponyfill check, and when I ran my tests in node 18 they all failed because they switched to node's fetch and started ignoring the configured HTTP proxy.

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 22, 2022
@mcollina
Copy link
Member

@nodejs/tsc did we expose fetch too early?

@targos
Copy link
Member

targos commented Apr 22, 2022

@nodejs/tsc did we expose fetch too early?

Not sure. It's difficult to say if we would have gotten useful feedback with the API still behind a flag.

@pimterry note that you can opt out of node's fetch with --no-experimental-fetch.

@panva
Copy link
Member

panva commented Apr 22, 2022

@nodejs/tsc did we expose fetch too early?

The problem isn't exposed fetch. It's that it doesnt utilize node's global agents due to its use of undici. There's a whole ecosystem of mocking and proxying libraries that relied on the fact that regardless of what http client / library you used in node, it always boiled down to http/s.request and its use of global agents

@panva
Copy link
Member

panva commented Apr 22, 2022

fetch in Node.js is experimental, all of this is planned for when it exits experimental

In hindsight, a global that implements a Web Platform API that's actively being polyfilled probably shouldn't be exposed when still experimental / unstable. That being said

It's difficult to say if we would have gotten useful feedback with the API still behind a flag.

Exposing it via a node:fetch module first would probably allow for more feedback to trickle in. WebCrypto API was also not exposed as global and was first added to the crypto module as an export while experimental.

@ronag
Copy link
Member

ronag commented Apr 22, 2022

https://github.com/orgs/nodejs/teams/tsc did we expose fetch too early?

Maybe. But I'm not sure what waiting even more would have made any difference? The same things break. The polyfills are still using node streams and http global agents so the primary breakage would be the same.

@mcollina
Copy link
Member

I think we should seriously consider exposing node:undici, or possibly node:http-next. What do you think?

@ronag
Copy link
Member

ronag commented Apr 22, 2022

I like the idea!

@ljharb
Copy link
Member

ljharb commented Apr 22, 2022

This isn’t hindsight, to be clear; the risk was brought up many times and intentionally gambled.

@devsnek
Copy link
Member

devsnek commented Apr 22, 2022

I don't think we need to remove the global. it's an experimental api added in a major version. feature requests and bug reports are to be expected, and we should handle them normally.

@ljharb
Copy link
Member

ljharb commented Apr 22, 2022

The global, combined with its lack of feature parity, is what’s breaking many things in userland. That it’s experimental doesn’t really matter when it’s a global, because users aren’t opting in to it being present.

@tniessen
Copy link
Member

Will Undici dispatchers be completely separate from node's agents?

@mcollina
Copy link
Member

That's a design goal, yes.

@tniessen
Copy link
Member

tniessen commented Apr 28, 2022

In that case, does that imply having to implement HTTP/2 and HTTP/3 both in node core and in undici? Or can we reuse some efforts in either way?

Related: #38478 #38233 nodejs/undici#399

@mcollina
Copy link
Member

No, we can reuse what's in Node Core for HTTP/2. The fundamental design issue of the HTTP/1.1 client implementation in Node is that it is tied to HTTP/1.1 logic where 1 connection is run on 1 request.

If you want to finish the HTTP/2 implementation, there is a draft at nodejs/undici#1014.

@varanauskas
Copy link

varanauskas commented May 2, 2022

I think we should seriously consider exposing node:undici, or possibly node:http-next. What do you think?

This would solve my issue by being able to do:

import { getGlobalDispatcher } from "node:undici";

In fact, at least for me, ^ this is exactly what I tried to do intuitively before looking for this issue or other ways to expose the internal node undici client, after discovering setGlobalDispatcher from undici did not work with Node's internal fetch

@ljharb
Copy link
Member

ljharb commented May 2, 2022

I think it would be pretty confusing to have node:undici and undici mean very different things, unless the non-core undici is planning to become deprecated - imagine a "duplicated peer dep" problem that the package manage is incapable of resolving.

@mcollina
Copy link
Member

mcollina commented May 2, 2022

nodejs/undici#1331 would solve this.

@ljharb
Copy link
Member

ljharb commented May 2, 2022

Exposing one piece under its own name (not under “undici”) and then having undici and core both use it seems great - my concern is over a likely common overlap between npm and core, both conceptually and in users’ actual dep graphs.

@dlongley
Copy link

dlongley commented May 2, 2022

Just want to leave a note here that the full solution to this problem should involve both of these from the OP, not just one or the other:

  • Provide a way to specify a Node.js agent or a Undici dispatcher for a fetch request
  • Provide a way to set a global agent/dispatcher, which applies to all HTTP & fetch requests

I see that discussion so far seems to have focused on the latter, which is fine for addressing part of the problem, but it will not address it entirely. Hopefully passing a dispatcher as an option to the fetch API (or whatever is decided is an appropriate replacement for the pattern of passing an agent today) becomes possible soon to enable targeted (vs. global) configurations.

@mcollina
Copy link
Member

mcollina commented May 4, 2022

We are working on both those points @dlongley:

Hopefully those will ship in the next version of Undici and then updated in Node.js

@mcollina mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label May 11, 2022
@mcollina
Copy link
Member

#43059 will fix this

@dnalborczyk
Copy link
Contributor

I think we should seriously consider exposing node:undici, or possibly node:http-next. What do you think?

if an export is being provided, it should probably be fetch/node:fetch. undici sounds like a codename for something, it's just not intuitive at all.

@pimterry
Copy link
Member Author

#43059 will fix this

@mcollina that makes sense for the Undici lib side, but can you explain how that will work in Node? It's not clear how you'd create an Undici Agent to pass to fetch() or how you'd call setGlobalDispatcher from Node.

Is require('node:undici') or require('fetch') or similar going to be available in the end to make those APIs accessible?

@mcollina
Copy link
Member

@mcollina that makes sense for the Undici lib side, but can you explain how that will work in Node? It's not clear how you'd create an Undici Agent to pass to fetch() or how you'd call setGlobalDispatcher from Node.

As for now, calling setGlobalDispatcher() from undici will configure it for Node.js fetch as well, there is no need to expose the module.

Is require('node:undici') or require('fetch') or similar going to be available in the end to make those APIs accessible?

Not in the short term.

@frank-dspeed
Copy link
Contributor

@ljharb can you maybe enlighten me a bit out of wintercg view:

Questions left

@pimterry
Copy link
Member Author

I've been testing this - it does solve the problem @mcollina (thanks!) and I understand there are short-term constraints, but medium-term it would be great to aim to not need a duplicate Undici dependency just to be able to set an agent.

As a motivating example: I'd love to automatically support global Fetch in https://www.npmjs.com/package/global-agent (drop-in lib to make all Node HTTP use the system proxy) but even though Node bundles Undici AFAICT this isn't possible without adding a full separate Undici dep to that lib, which would more than double its size, just to ship code that's already present in Node itself.

This would be immediately solved if ProxyAgent and setGlobalDispatcher were exposed explicitly somewhere in future.

@mcollina
Copy link
Member

just to ship code that's already present in Node itself

This is not entirely correct. Node.js includes the bare minimum of undici to support fetch(), therefore ProxyAgent or setGlobalDispatcher are not bundled in.

@pimterry
Copy link
Member Author

Ah, that's interesting! I think the point largely stands though, since adding the dependency does duplicate the vast majority of Undici's other code - lib/fetch and lib/llhttp alone are more than 50% of the size of the Undici package, and are both included in node's bundle. Needing to install & import all that code that's already present is suboptimal.

I briefly had a go at creating an Undici fork that contained only ProxyAgent & setGlobalDispatcher, but Undici's agent.js depends on client.js which is a full HTTP client implementation, so there's no way to avoid this that way.

On the flip side though, that overlap suggests that ProxyAgent & setGlobalDispatcher would probably not significantly increase the size of Undici in Node. Looking at the Undici.js bundle (I assume that's the right place?) I can see that the entirety of setGlobalDispatcher, and the Dispatcher, DispatcherBase and Agent classes are already included there. It's only ProxyAgent itself that's missing, which is tiny.

I've just done a quick test on main in Undici: exposing ProxyAgent & setGlobalDispatcher explicitly increases the Undici bundle size from 334.2kb to 336.0kb (+1.8kb / +0.5%).

Could these APIs be included and exposed in future? Agents for HTTP are a very core API that it would be useful to have usable out of the box, the equivalent functionality is usable OOTB for the legacy http module APIs, and 2kb is not a significant jump in bundle size for this functionality.

@mcollina
Copy link
Member

I would recommend opening up a separate issue about this topic and bringing it to the TSC. I don't think the whole of Undici has the stability guarantees needed to be part of the Node.js LTS cycle yet.

@pimterry
Copy link
Member Author

@mcollina Moved into a new issue here: #43187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fetch Issues and PRs related to the Fetch API
Projects
None yet
Development

Successfully merging a pull request may close this issue.