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 the global dispatcher truly global #1331

Closed
delvedor opened this issue Apr 12, 2022 · 7 comments · Fixed by #1405
Closed

Make the global dispatcher truly global #1331

delvedor opened this issue Apr 12, 2022 · 7 comments · Fixed by #1405
Labels
enhancement New feature or request

Comments

@delvedor
Copy link
Member

The current implementation of the global dispatcher is that it stores within the library the global agent. This works well when the user is directly using undici. Still, suppose the user is using a library that internally uses undici and the user didn't install the exact same version of the library. In that case, the global dispatcher API won't work as the user would expect.

To clarify what I think a typical user expects: it should work in the same way as nock.
nock does not suffer from this problem because it will always use the same version of Node.js, while for undici this is not true (yet).

Even if it's kinda ugly, I would suggest attaching the global dispatcher to the Node.js's global object, so different versions (withing the major possibly) of undici can use the same global dispatcher.

If this solution is fine for you (or you have a better idea), I can work on it.

@ronag
Copy link
Member

ronag commented Apr 12, 2022

@mcollina I think this is more your area. Wdyt?

@delvedor
Copy link
Member Author

A problem might be compatibility between major versions, but that can be detected by adding some metadata to the global object.

For example:

global.___undiciGlobalDispatcher = {
  version: 5,
  dispatcher: Dispatcher
}

And have undici checking if the global dispatcher uses the same major version as the one configured in the global.

@mcollina
Copy link
Member

mcollina commented May 2, 2022

I thought quite a lot about this in the last few weeks.
The fundamental problem is the incompatibility of multiple versions of Undici. However, the interface for Dispatcher looks quite simple to me: https://undici.nodejs.org/#/docs/api/Dispatcher.

I think we could do this. However, we should be extra careful when doing so and validate compatility.

@ronag do you think it would be possible to use the Agent from one version of undici in the one inside node core?

@mcollina
Copy link
Member

mcollina commented May 2, 2022

This could solve it nodejs/node#42814.

@ronag
Copy link
Member

ronag commented May 2, 2022

@ronag do you think it would be possible to use the Agent from one version of undici in the one inside node core?

Yes. But then we can't break the interface. Ever

@mcollina
Copy link
Member

mcollina commented May 2, 2022

Pretty much. Are we reading for it?

@delvedor
Copy link
Member Author

delvedor commented May 3, 2022

Yes. But then we can't break the interface. Ever

Yes and no IMHO. This problem could be solved similarly to what we are doing with Fastify plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants