-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Comments
@nodejs/undici |
Note: the same applies to |
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 |
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 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.
I don't think enumerability matters here, most checks are just 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. |
@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 |
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 |
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
Exposing it via a |
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. |
I think we should seriously consider exposing |
I like the idea! |
This isn’t hindsight, to be clear; the risk was brought up many times and intentionally gambled. |
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. |
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. |
Will Undici dispatchers be completely separate from node's agents? |
That's a design goal, yes. |
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 |
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. |
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 |
I think it would be pretty confusing to have |
nodejs/undici#1331 would solve this. |
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. |
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:
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 |
We are working on both those points @dlongley: Hopefully those will ship in the next version of Undici and then updated in Node.js |
#43059 will fix this |
if an export is being provided, it should probably be |
@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 Is |
As for now, calling
Not in the short term. |
@ljharb can you maybe enlighten me a bit out of wintercg view: Questions left
|
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 |
This is not entirely correct. Node.js includes the bare minimum of |
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 - 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 I've just done a quick test on 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 |
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. |
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:
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 asetGlobalDispatcher
method to configure a dispatcher globally (like node'sglobalAgent
) 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?
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 useshttp
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.The text was updated successfully, but these errors were encountered: