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

Do we need Agent.get? #616

Closed
ronag opened this issue Mar 24, 2021 · 6 comments
Closed

Do we need Agent.get? #616

ronag opened this issue Mar 24, 2021 · 6 comments
Assignees
Labels
question [Use a Discussion instead]
Milestone

Comments

@ronag
Copy link
Member

ronag commented Mar 24, 2021

I think Agent.get is a bit weird now that we have Agent.dispatch and adds unnecessary API surface.

I know some of my colleagues use Agent.get directly instead of the helper methods (they want to use Client.dispatch). I could easily see that they would get confused and start using RedirectAgent.get (they would assume they can just replace Agent with RedirectAgent in existing code) thinking it does something it does not. Most people are not very meticulous with reading the docs and/or release notes.

Refs: #603 (comment)

I know @mcollina has some use cases in mind though. Could you maybe expand a bit on those?

@ronag ronag added the question [Use a Discussion instead] label Mar 24, 2021
@ronag ronag added this to the 4.0 milestone Mar 24, 2021
@ronag ronag self-assigned this Mar 24, 2021
@ronag
Copy link
Member Author

ronag commented Mar 24, 2021

@mcollina @dnlup

@dnlup
Copy link
Contributor

dnlup commented Mar 25, 2021

In a way I think we do, otherwise, we would have to reimplement it each time we extend from Agent. I am pretty sure it would look the same in all custom Agents. It makes sense to me as a low-level API. But I also understand your concern, so maybe we should expose this to the end-user in a different way.

For example:
https://github.com/nodejs/undici/pull/603/files#diff-0c8b0ffe565a54bbb79a3b35000d152df16910fe193065de4e2f10f3ede8ec42R84

We would have to reimplement the get logic in dispatch there.

@dnlup
Copy link
Contributor

dnlup commented Mar 25, 2021

Maybe, when we extend/create a custom Agent, we could let the developer provide a _dispatch method (like _write in streams) which has a Client/Pool instance passed as an argument. This way, get could be internal, but it would part of the default prototype of the Agent so that we don't have to re-implement it each time we extend from it.

@ronag
Copy link
Member Author

ronag commented Mar 25, 2021

I’m just concerned with the public API for end users.

@mcollina
Copy link
Member

The reason why we need get() is to provide a way for end users to reach into the Pool instance that is being used for a given host. From Pool, they should be able to reach for Client.

Those things are deeply needed for observability and introspection. Removing access is not the solution.
As a theme, we need more introspection.


Overall I do not see the problem you are mentioning @ronag. If they use .get() their system will crash or not redirect.
I also do not see a problem in having the RedirectAgent forward the calls to the encapsulated agent, e.g. close() and others. For all intent and purposes a RedirectAgent should behave like an Agent minus the behavior in dispatch() that is overridden.

I think we might want to consider adding .request() .pipeline() and .stream() to Agent as well.

One thing I would change is that we might want to use a fresh Agent and not the globalAgent as the backing of the RedirectAgent.

@ronag
Copy link
Member Author

ronag commented Mar 25, 2021

Ok, I think I might have a suggestion we can all agree on. Will open PR.

ronag added a commit that referenced this issue Mar 25, 2021
ronag added a commit that referenced this issue Mar 25, 2021
ronag added a commit that referenced this issue Mar 25, 2021
ronag added a commit that referenced this issue Mar 25, 2021
ronag added a commit that referenced this issue Mar 25, 2021
ronag added a commit that referenced this issue Mar 25, 2021
ronag added a commit that referenced this issue Mar 25, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 27, 2021
ronag added a commit that referenced this issue Mar 28, 2021
ronag added a commit that referenced this issue Mar 28, 2021
ronag added a commit that referenced this issue Mar 28, 2021
ronag added a commit that referenced this issue Mar 30, 2021
ronag added a commit that referenced this issue Mar 30, 2021
ronag added a commit that referenced this issue Mar 30, 2021
ronag added a commit that referenced this issue Mar 31, 2021
* refactor: Client.clients & Agent is a Client.

Refs: #616

* fixup: rename clientt to dispatcher

* fixup: keep promise chain

* fixup

* fixup

* fixup
@ronag ronag closed this as completed Mar 31, 2021
crysmags pushed a commit to crysmags/undici that referenced this issue Feb 27, 2024
* refactor: Client.clients & Agent is a Client.

Refs: nodejs#616

* fixup: rename clientt to dispatcher

* fixup: keep promise chain

* fixup

* fixup

* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question [Use a Discussion instead]
Projects
None yet
Development

No branches or pull requests

3 participants