Open pimterry opened 2 years ago
setGlobalDispatcher
is exposed:
global[Symbol.for('undici.globalDispatcher.1')] = yourDispatcher;
Replying to the last comment from @mcollina in the previous issue:
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.
This makes sense! I'm not aware of Undici's process around stability guarantees like this, but I can understand how there are constraints there.
I do understand this isn't a top top priority since installing & importing Undici elsewhere is a usable workaround, so there's certainly no rush just would justify shipping an unstable API unnecessarily. I do think that the current situation isn't a good end state though, and there are good options we can aim for to fix this, so it would be great to find agreement to aim in that direction in the medium term.
I'm not sure how to take anything to the TSC, but very happy to be included on any discussion of this any time.
global[Symbol.for('undici.globalDispatcher.1')] = yourDispatcher;
That's does work as a workaround, but it's not especially nice as the official API for this, and I think ProxyAgent needs to be available too to make this usable for global Fetch.
I don't think we should expose undici
further until we want to consider fetch
as stable.
which part of the fetch spec is unstable or problematic i do not get it fetch is the same as XMLHTTPRequests but async so it is equal to the https http and so on request methods/get
and i found zero parts of the spec in fetch that could harm nodejs
so my question what is blocking why not adopting it? The cache can be in memfs or even turned off by design or not implemented but what else could be blocking?
is it about context related policy header handling?
@frank-dspeed I don't understand how this comment is relevant to this issue. We shipped fetch()
in Node v18 unflagged. How are we not adopting it?
Node.js fetch()
is not stable yet because
1) it misses features
2) there are incompatibilities between Browser envs and Server envs that needs to be sorted (spec deviations)
3) there are plenty of bugs still
We still need time to figure out all those details before calling it stable and then having to do breaking changes to correct mistakes.
Anyhow, I think this discussion is out of topic. I'll hide both of our comments - this issue is about a specific detail of the implementation, not fetch in Node.js.
@mcollina I don't think we should expose
undici
further until we want to considerfetch
as stable.
2022-11-14, Version 19.1.0
That does not mark it stable, on purpose. It's the first step on that journey. This is something we might want to consider right now.
In the TSC meeeting of today, we decided that we are going to support HTTP_PROXY, HTTPS_PROXY and NO_PROXY env variables directly in Undici: https://github.com/nodejs/undici/issues/1650.
How is it going to handle different casing rules?
setGlobalDispatcher
is exposed:global[Symbol.for('undici.globalDispatcher.1')] = yourDispatcher;
Is there a workaround to obtain ProxyAgent
?
Hi All,
I would like if you expose the whole undici
.
If you expose only a certain feature of undici then there will be someone who needs some unexposed feature, so better to expose everything in 1 step.
I think it is not worth polluting the global namespace so I suggest using node:undici
.
While I would like to see ProxyAgent
exposed too, I believe that it'd be even better if a proxy
option would be added into fetch
itself.
With this, node's fetch would diverge from web fetch, but imho for good reason because the option is not relevant in browsers where proxies are handled outside of JS.
I think it's time for the TSC to make some plans about this. I'll cover this at the collab summit too.
In addition to ProxyAgent
it would also be nice to expose Agent
. I currently use it to set keepAlive
and connections
options in undici because they are not exposed in fetch
.
In addition to
ProxyAgent
it would also be nice to exposeAgent
. I currently use it to setkeepAlive
andconnections
options in undici.
We're using HttpAgent similarly now so that we can override DNS resolution with our own caching resolver.
setGlobalDispatcher
is exposed:global[Symbol.for('undici.globalDispatcher.1')] = yourDispatcher;
In case anyone else was looking for a way to overriding the global dispatcher with a new agent with some custom config options, here's an example
// there is no official Node API to get the global dispatcher (as of Node 20.11.1)
// getting the dispatcher from global is considered a public API https://github.com/nodejs/undici/discussions/2167#discussioncomment-6265039
// hard-coded symbol for the undici global dispatcher https://github.com/nodejs/undici/blob/main/lib/global.js#L5
const unidiciGlobalDispatcherSymbol = Symbol.for('undici.globalDispatcher.1');
const undiciGlobalDispatcher = global[unidiciGlobalDispatcherSymbol];
if (!undiciGlobalDispatcher) {
throw new Error('Could not find the global Undici dispatcher, maybe not in node runtime?');
}
// initialize a new agent/dispatcher https://undici.nodejs.org/#/docs/api/Agent
// inspired by https://github.com/orgs/nodejs/discussions/51260#discussioncomment-7949240
const newDispatcher = new undiciGlobalDispatcher.constructor({
// limit the number of client (host) connections
// https://undici.nodejs.org/#/docs/api/Pool
connections: 128,
});
// override the Undici global dispatcher
global[unidiciGlobalDispatcherSymbol] = newDispatcher;
so, there is no way to set a dispatcher on a per-call basis yet?
so, there is no way to set a dispatcher on a per-call basis yet?
You can already set dispatcher
in fetch
options. But to really make it useful, Agent
and ProxyAgent
need to be accessible without importing them from undici
.
But to really make it useful, Agent and ProxyAgent need to be accessible without importing them from undici.
right. @mcollina any updates from TSC?
There is no consensus on exposing more of undici.
My current plan is to add support for the HTTP_PROXY env variable behind a flag in v22 (and possibly v20), then unflag in v23.
@mcollina reading HTTP_PROXY is probably not powerful enough. I have (dev only, for use with Fiddler) a loop that monitors for the proxy server being up and only then enrolling requests in the proxy. This allows the use of Fiddler to be decided for an already running process and not depend on Fiddler running for calls to succeed.
I don't suggest that precisely this better but still imperfect mechanism be built into node - but we ought to be able to write our own. Exposing the primitives for this is better than choosing one underpowered implementation for all.
Agreed. If you need that level of sophistication you are better off in using undici directly for now.
const unidiciGlobalDispatcherSymbol = Symbol.for('undici.globalDispatcher.1'); const undiciGlobalDispatcher = global[unidiciGlobalDispatcherSymbol]; if (!undiciGlobalDispatcher) { throw new Error('Could not find the global Undici dispatcher, maybe not in node runtime?'); }
I find that global[unidiciGlobalDispatcherSymbol];
is undefined unless I call fetch() at least once.
workaround:
fetch().catch(_=>{});
const unidiciGlobalDispatcherSymbol = Symbol.for('undici.globalDispatcher.1');
const undiciGlobalDispatcher = global[unidiciGlobalDispatcherSymbol];
if (!undiciGlobalDispatcher) {
throw new Error('Could not find the global Undici dispatcher, maybe not in node runtime?');
}
const newDispatcher = new undiciGlobalDispatcher.constructor({
connections: 128,
});
global[unidiciGlobalDispatcherSymbol] = newDispatcher;
I've marked this as feature request
, as it seems to be asking for the exposure of additional items (ProxyAgent
and setGlobalDispatcher
), if you disagree, feel free to unlabel.
At a minimum, I'd love it if I could use the workaround without calling fetch()
first. I understand this is just the lazy-load feature in action and that has its own set of benefits. I may explore it a bit more and see if there's a simpler way to trigger the lazy loading
so, there is no way to set a dispatcher on a per-call basis yet?
The easiest way is to pass { dispatcher: x } on each undici call, otherwise there is a factory method if using a global dispatcher.
Below accesses the internals of undici to get Agent, the factory method uses either Pool, or Client.
/**
* Use the undici internals shipped with Node.js
*/
function getUndiciInternals() {
/* We need to call fetch for the globalDispatcher to be available */
fetch().catch(() => {})
const globalDispatcherSymbol = Symbol.for('undici.globalDispatcher.1')
const agent = globalThis[globalDispatcherSymbol]
/* get access to the factory to avoid needing to reference Pool, and Client */
const agentFactorySymbol = Object.getOwnPropertySymbols(agent).find((x) => x.toString() === 'Symbol(factory)')
const agentFactory = agent[agentFactorySymbol]
const Agent = agent.constructor
return {
globalDispatcherSymbol,
Agent,
agentFactory,
}
}
class MyDispatcher extends undici.Agent {
constructor(options) {
super({
...options,
factory(url, options_) {
// can pass other options here too :)
return undici.agentFactory(url, options_)
},
})
}
}
globalThis[undici.globalDispatcherSymbol] = new MyDispatcher()
@nodejs/tsc I think we need to make a decision here. Do we want to expose the bundled undici or not? Even if not, we should at least provide some API to get the undici Agent because people are doing all kind of hacks to get it.
Press ππ» if you think Undici should be exposed and ππ» if not
we should at least provide some API to get the undici Agent because people are doing all kind of hacks to get it.
Developers try to find a solution to their problem. We can call it hack.
This is why I suggested to expose the whole undici ~1 year ago.
Would be nice to expose using node:undici
(so that global fetch
is the same as import { fetch } from 'node:undici'
)
Would be nice to expose using
node:undici
(so that globalfetch
is the same asimport { fetch } from 'node:undici'
)
I like the idea of exporting node:undici
fetch
would be optional as it's already a global, just like it's optional to import process from node:process
. The main benefit for me is not having to ship undici
for AWS Lambda if all we want is to have a custom dispatcher.
9/4 Meeting
9/11 Meeting
there was openness to including more of Undici but not all
Any more background on this?
What about exposing normal Agent
from undici too? I just need custom DNS lookup function and I shouldn't use ProxyAgent.
We wound up going down the path of including undici directly for most of what we're doing (using request and overriding the global dispatcher to use a custom lookup function)
With Node 22 including the web socket implementation, we went ahead and preemptively switched to that on Node 20 as well.
Having to include it manually isn't that big of a deal as long as it plays nicely together (i.e.: not needing to set 2 "global" dispatchers to cover anything that feeds through the native fetch).
Originally posted by @pimterry in https://github.com/nodejs/node/issues/42814#issuecomment-1134535017