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

Future of the Node HTTP Client #38533

Open
ronag opened this issue Apr 30, 2021 · 41 comments
Open

Future of the Node HTTP Client #38533

ronag opened this issue Apr 30, 2021 · 41 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@ronag
Copy link
Member

ronag commented Apr 30, 2021

I see a slight problem with the situation with the node core http client. We are getting issues reported in the issue tracker. However, there is little interest by contributors (that are familiar with the code) to look into these issues and resolve them.

Part of the reason for this is that some of the people that usually engage with http client issues is @mcollina, @dnlup or myself. Whom I believe are mostly focusing on nodejs/undici, which we feel is a better alternative and the future, and have limited interest in putting further effort into the node core http client.

This is a slightly suboptimal situation. I would personally like to discuss whether and how we could encourage users over to NodeJS/undici and eventually deprecating the current node client.

Maybe add something in the docs that point to undici. However, I don’t think this is something we have done in the past.

Thoughts?

@DerekNonGeneric
Copy link
Contributor

Maybe add something in the docs that point to undici. However, I don’t think this is something we have done in the past.

I found some prior art that may help: our util.log(string) core function recommends using a third party module, but it's deprecated, which I'm unsure whether would be an appropriate designation in this situation (perhaps more likely legacy).

Stability: 0 - Deprecated: Use a third party module instead.

Refs: https://nodejs.org/dist/latest-v16.x/docs/api/util.html#util_util_log_string

@mcollina
Copy link
Member

mcollina commented May 4, 2021

I share @ronag concerns and I would also add one of my own.

The reason why very few would like to work on the HTTP client is because every change breaks the huge number of modules in the ecosystem that monkeypatch it. Fixing the majority of those bugs requires either breaking some other users or finding some clever hacks that will come hunting us back over the years.

@Trott
Copy link
Member

Trott commented May 4, 2021

Since this is on the TSC agenda: @nodejs/tsc

@benjamingr
Copy link
Member

I think @jasnell is talking about shipping WHATWG ReadableStream this year (right?), we can also land the polyfill in core in the meantime and just ship fetch (on top of unidici perhaps).

Then we can direct people to the new API and put the old API in maintenance mode and possibly eventually even deprecate it.

Another possible alternative would be to land unidici as part of core (or as a dep) and build the existing HTTP API on top of it (kine of like what browsers did with fetch and XHR or how domains build on async_hooks now).

@mcollina
Copy link
Member

mcollina commented May 4, 2021

Another possible alternative would be to land unidici as part of core (or as a dep) and build the existing HTTP API on top of it (kine of like what browsers did with fetch and XHR or how domains build on async_hooks now).

It's not possible. The current API is an extremely leaky abstraction that multiple modules in the ecosystem monkeypatch. Doing so would not reduce the friction for the community.

@benjamingr
Copy link
Member

It's not possible. The current API is an extremely leaky abstraction that multiple modules in the ecosystem monkeypatch. Doing so would not reduce the friction for the community.

I'm sure this is pretty common in servers - but who's patching the clinet?

@targos
Copy link
Member

targos commented May 4, 2021

I'd prefer we keep the existing API with a legacy status and introduce a new one based on undici.

@benjamingr
Copy link
Member

I'd prefer we keep the existing API with a legacy status and introduce a new one based on undici.

Note that if landing fetch is an eventual goal that would mean we'd have to maintain 3 HTTP APIs. I am fine with this if the fetch we land is based on unidici to the point we only de-facto maintain two APIs and fetch is just a wrapper around that

@ronag
Copy link
Member Author

ronag commented May 4, 2021

Just a side note. I actually prefer if undici is not included in core. It is much easier to maintain and develop as npm module.

@ronag
Copy link
Member Author

ronag commented May 4, 2021

I'd prefer we keep the existing API with a legacy status and introduce a new one based on undici.

Note that if landing fetch is an eventual goal that would mean we'd have to maintain 3 HTTP APIs. I am fine with this if the fetch we land is based on unidici to the point we only de-facto maintain two APIs and fetch is just a wrapper around that

There is https://github.com/Ethan-Arrowood/undici-fetch which we are considering merging into undici once it's a bit more mature.

@targos
Copy link
Member

targos commented May 4, 2021

It would be unfortunate to have to recommend people to use an external module for doing HTTP requests...

@benjamingr
Copy link
Member

@ronag

Kind of OT on your side note:

Just a side note. I actually prefer if undici is not included in core. It is much easier to maintain and develop as npm module.

I think that's a bigger problem and we need a process for deps to live in core without requiring a full CI run or the same process. The reason it's much nicer to maintain/develop outside of core is (presumably, from stuff I was involved in):

  • You don't need to wait 48 hours (or a week) to land PRs.
  • You don't need to wait for mostly flakey CI that's mostly irrelevant.
  • You can easily set up your own infra without needing to bother more people.
  • You don't need to go through the consensus seeking process.

It would be great if we could solve that and make contributing relatively standalone stuff (like unidico or AbortSignal) more convenient and less tedious.

One approach you may want to take is to keep the nodejs/unidici repo and treat it like a dep - that way you'd get the same "freedom" stuff like libuv gets

@benjamingr
Copy link
Member

There is https://github.com/Ethan-Arrowood/undici-fetch which we are considering merging into undici once it's a bit more mature.

That would be cool though I would prefer it if users had fetch as a convenient API and unidici (presumably with a more "standard" name) as a low-level API (assuming callback-land http becomes legacy).

@ronag
Copy link
Member Author

ronag commented May 4, 2021

@ronag

Kind of OT on your side note:

What does OT mean?

I think that's a bigger problem and we need a process for deps to live in core without requiring a full CI run or the same process. The reason it's much nicer to maintain/develop outside of core is (presumably, from stuff I was involved in):

Yea I get that. Just thinking that it would be nice if possible.

@ronag
Copy link
Member Author

ronag commented May 4, 2021

There is https://github.com/Ethan-Arrowood/undici-fetch which we are considering merging into undici once it's a bit more mature.

That would be cool though I would prefer it if users had fetch as a convenient API and unidici (presumably with a more "standard" name) as a low-level API (assuming callback-land http becomes legacy).

Yea, that's the idea. In terms of API we have a bit of a layered approach in undici. Whether or not we split undici into minimal packages (e.g. undici-core, undici-pool, undici-agent, undici-api, undici-fetch) or just have everything together (undici) is something to consider.

@MylesBorins

This comment has been minimized.

@ronag

This comment has been minimized.

@Trott

This comment has been minimized.

@mhdawson
Copy link
Member

We discussed this before, agreed with the ask for references in the Node.js docs. Taking off the agenda for now.

@StarpTech
Copy link

StarpTech commented Jun 10, 2021

I see two concerns in this issue.

  • How can we motivate people to contribute?
  • The current client is not good. Could you add more insights?

Is the goal of unidici to be spec-compliant? As far I know it's not.

@ronag
Copy link
Member Author

ronag commented Jun 10, 2021

  • How can we motivate people to contribute?

What do you mean?

Is the goal of unidici to be spec-compliant? As far I know it's not.

What does spec compliant mean here? If you mean http spec, then yes, as far as make sense.

@StarpTech
Copy link

StarpTech commented Jun 10, 2021

What do you mean?

However, there is little interest by contributors (that are familiar with the code) to look into these issues and resolve them.


What does spec compliant mean here? If you mean http spec, then yes, as far as make sense.

https://github.com/nodejs/undici#specification-compliance

@mcollina
Copy link
Member

There are a lot of issues with the current http client which are impossible or very hard to fix due to legacy compact.

Out of curiosity. "Compat at all costs even across major releases." Is this the strategy for innovation and reworks in core?

Breaking most of the http clients and backend frameworks is currently a blocking concern.

@jasnell
Copy link
Member

jasnell commented Jun 10, 2021

For other pieces, we've sidestepped the backwards compat concerns by simply leaving the existing stuff untouched and launching a new parallel track. Maintain the current. Innovate in the new.

@StarpTech
Copy link

Breaking most of the http clients and backend frameworks is currently a blocking concern.

This is clear. It's about the long term. For me it sounds like a "can't" rather a "shouldn't" and this is not good.

For other pieces, we've sidestepped the backwards compat concerns by simply leaving the existing stuff untouched and launching a new parallel track. Maintain the current. Innovate in the new.

Does it work? I mean streams and now http client... all modules that suffer from compat for so many years and are still unpleasant. I'm not aware of any public plan how to improve that beside the direction to do everything in back compat way.

@ronag
Copy link
Member Author

ronag commented Jun 11, 2021

@StarpTech I'm not sure what you are asking here and what answer you are looking for? There is only so far we can go in certain things without breaking the entire ecosysem. There are things we simply can't fix. Might not be "good" but there isn't much to do about it. I have personally been pushing the limits on what we can change in both http and streams.

@StarpTech
Copy link

I'm interesting in how such things are handled here to estimate what the future will bring. Sorry, I didn't want to hijack the issue ☺

@ronag
Copy link
Member Author

ronag commented Jun 17, 2021

#39057

@Flarna
Copy link
Member

Flarna commented Jun 17, 2021

Is there already a conclusion regarding moving undici into core?
I fully understand that keeping it as a separate NPM module results in an easier workflow. But I assume the same would be valid for all core components.

core has a well defined maintenance/release strategy including LTS. Are the same rules applicable to undici once it is the recommended HTTP client?

node has citgm to watch the impact on the ecosystem. Currently it's verified that a new node release doesn't break undici but I assume once undici as the recommended HTTP client something similar should be done for undici releases.

Please note that my intend is not to block anything here. My main point is to understand the long term strategy regarding this.

@ronag
Copy link
Member Author

ronag commented Jun 17, 2021

Is there already a conclusion regarding moving undici into core?

No.

But I assume the same would be valid for all core components.

Indeed. This is an argument that goes back and forth, e.g. stream utils such as pipeline and finished (and now destroy) could also more easily live as npm packages. I'm not sure what the strategy in general is here in regards to what is and isn't in core.

core has a well defined maintenance/release strategy including LTS. Are the same rules applicable to undici once it is the recommended HTTP client?

I have a hard time seeing that it would. Not sure whether or not that would make sense. It would be as any other npm package, if it's not in core. I think LTS strategy makes sense for node core since it's not possible for users to cherry-pick what to or not to upgrade. This does not apply to npm packages where users can choose when and how to upgrade semver major.

node has citgm to watch the impact on the ecosystem. Currently it's verified that a new node release doesn't break undici but I assume once undici as the recommended HTTP client something similar should be done for undici releases.

That would be great. However, we would need help to put that into practice. I think there was some project that was working on making something like citgm for npm packages (something along the lines "will I break you" or something)?

Please note that my intend is not to block anything here. My main point is to understand the long term strategy regarding this

Good feedback. My understanding is that the consensus is that undici is the way forward but there are still questions that remain open.

I'm much in favor of leaving undici outside of core due to ease of development and maintenance. Those are my personal primary concerns as a undici developer and those priorities might not align with the priorities of node core and the ecosystem.

@jasnell
Copy link
Member

jasnell commented Jun 17, 2021

If undici is merged into core, it would follow the same LTS/maintenance cycle. If it remains separate, then it would have it's own cycle, similar to the readable-stream module. Personally, I'd like to see it merged into core sooner rather than later -- however, right now it's premature as that would hinder it's ability to evolve as quickly as it needs right now.

@Trott
Copy link
Member

Trott commented Jun 23, 2021

We didn't have the right people to talk about this at the TSC meeting today, but it seems like it ties into the larger issue of nodejs/TSC#1041.

@mhdawson
Copy link
Member

We agreed in the TSC meeting today to remove from the tsc-agenda and possibly add it back on after nodejs/TSC#1041
is resolved

@GrosSacASac
Copy link
Contributor

If I understand correctly undici has all fixes to node http that are long due, and changing http itself is an impossible puzzle since so much code in the wild monkeypatches it.

In that case I recommend to include undici in node alongside http and mark it as legacy.

Just a side note. I actually prefer if undici is not included in core. It is much easier to maintain and develop as npm module.

Why is is so much easier ?

@Chaphasilor
Copy link

Why is is so much easier ?

Faster release cycles, easier for others to contribute to, independent CI environments, etc.

@GrosSacASac
Copy link
Contributor

Faster release cycles, easier for others to contribute to, independent CI environments, etc.

To keep those benefits is it possible to include whatever the last release is from unici into node whenever node is released (similar to npm I think)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests