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

[WIP] experimental fetch #27979

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@MylesBorins
Copy link
Member

commented May 30, 2019

First off... this is bad and probably shouldn't land exactly how it is implemented.

I've vendored node-fetch and exposed it as http.fetch. There are no docs or tests.

MylesBorins added some commits May 30, 2019

@renanbastos93

This comment has been minimized.

Copy link

commented May 30, 2019

How nice, case you need some help can call me.

@mscdex
Copy link
Contributor

left a comment

-1 this kind of higher-level functionality is best left to userland (just reiterating this from the previous discussion on this).

@addaleax
Copy link
Member

left a comment

👍

LGTM once docs and tests are added

@lpinca

This comment has been minimized.

Copy link
Member

commented May 31, 2019

I'm also -1, quoting @bnoordhuis from #19393

Having two different APIs (fetch() and http.request()) for doing the same thing is uncohesive.

There's nothing wrong with having two APIs to do the same thing, when one builds on top of the > other to provide a compelling user experience.

But it's not 'on top of' - they do the same thing, just with different interfaces.

Arguments in favor of fetch() boil down to 'convenience' - not a good reason for inclusion in core.

Also this is literally taking node-fetch as is and putting it in core. How is this better than npm i node-fetch?

@reconbot

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

This gives us the opportunity to use a c++ browser implementation which may be faster (mentioned in a previous thread) where the binary module story provides a lot of friction. As suggested in the description this pr isn’t the end goal.

Having it in core by default provides a better user experience for the majority of js devs who are less familiar with node.

@lpinca

This comment has been minimized.

Copy link
Member

commented May 31, 2019

If we get better performance in C++ why don't we rewrite our existing client instead so that all already existing apps/tools would benefit?

You know better than me that there are a lot alternatives for user experience like got, axios, superagent, etc. but those are not even considered because they are not a standard?. I think it's silly to add two HTTP client interfaces in a project that lists "small core" as one of its values.

There is a precedent with the URL interface but I doubt this is a good reason to add fetch in core.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Not a 👍 or a 👎, but doesn't vendoring an existing npm module ignore all of the concerns listed in https://docs.google.com/document/d/1tn_-0S_FG_sla81wFohi8Sc8YI5PJiTopEzSA7UaLKM/edit? If we vendor this in now, those concerns will probably never be addressed out of fear of breaking existing code.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

So to be super explicit. I am not expecting the vendored version to land. This is mostly to talk about API concerns and to be a WIP to get towards something we can ship. We should likely go through that doc and figured out what can and can't be fixed. It may also be possible to land something behind a flag and work towards getting those things fixed before unflagging

This PR could also be an opportunity to start playing with web platform tests and figure out exactly how we will test.

but those are not even considered because they are not a standard

A standard API that ships in other platforms is one of the primary reasons to adopt it. Lots of code is shared between environments and having shared interfaces is a major benefit. Further, these apis are specified and unlikely to significantly change, other APIs do not offer this contract. Adopting these APIs also gives us skin in the game and the ability to engage with standards orgs to get changes made up stream to allow us to be spec compliant

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

it dawns on me that I should probably @ the maintainers of node-fetch to get their 2c

@bitinn @TimothyGu

@bitinn

This comment has been minimized.

Copy link

commented Jun 1, 2019

@renanbastos93

This comment has been minimized.

Copy link

commented Jun 1, 2019

About this discuss I believe if us based on a browser implement method fetch, example https://github.com/chromium/chromium/search?p=1&q=fetch&unscoped_q=fetch we have an implement more consistent

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I've generatlly started to come around to thinking that Node.js should implement applicable Web standard APIs, for uniformity and pridictability of developers working across node and browser environments. Also,because they are "standardized" APIs, while not frozen, they are pre-defined, so evolve very slowly, if at all, so we don't get API churn on them, and don't have to make choices about the API, we just implement them.

However, these points challenge the applicability of fetch to Node.js

API Wise:
It uses WHATWG streams which we don’t currently support. Supporting it is a lot of hard work. Because it’s tied to whatwg streams there is an ecosystem problem, if we want to send a file over POST over fetch we need to bridge Node streams and WHATWG streams.
It uses EventTarget (DOM events) and not EventEmitter (Node events). We’d need to add EventTarget to core.

I gather npm packages that implement "fetch" (quotes very much intentional) for Node.js just hack the API so they use Node.js streams and EventEmitter? Thus creating a non-standard variant? That seems less than ideal. It makes sense for npm packages, since they can be chosen by users who like the general API "shape" of fetch, but its not clear that would be good for Node.js

On the other hand, introducing an API that uses underlying stream and event APIs that are incompatible with Node.js' is also not good.

Basically, while I'm in favour of fetch in Node.js in theory, if these issues can't be well addressed, I'm not in favour.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Even with incomplete compatibility this will cover most part of usecases.

There are projects that depend on node-fetch simply for the sake of working both in node and browser (node-fetch is a one-line reference to window.fetch in browser). E.g.: tensorflow -- tensorflow/tfjs#410, tensorflow/tfjs-core#1663, but there are a lot of those.

That said, I'm +1 to a fetch implementation in node core for the sake of better portability, even if it does not 100% implement the spec in parts that are mostly browser-oriented.

@devsnek

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

for those asking, we can't really pull the fetch implementation out of chrome because of various incompatibilities surrounding boring vs open ssl. at the point that we got it working we might as well just rewrite it.

if we agree to add fetch to core I'd love to help work on a napi module of it.

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Just a bit of background and discussion around this.

First... The current http.request() implementation is limited to just supporting HTTP/1. It does not work with HTTP/2, which has it's own separate low level client API. We did not integrate the APIs because it is not clear how to change http.request() in a way that would be backwards compatible and would allow exposing HTTP/2 features in a reasonable way. Now that work in progressing on not only HTTP/2, but QUIC and HTTP/3, we need a high-level client API that works with all consistently.

Given that, what I want to see here is a full eventual deprecation of http.request() and the current low-level HTTP/2 client API in favor of a single high-level, more flexible, and modernized API that will work transparently with HTTP/1, HTTP/2, and the forthcoming HTTP/3, complete with proper session management and automatic version discovery/negotiation. There is too much existing code that is reliant on http.request() for us to be able to make the necessary changes in a non-disruptive way. Like we've done with URL, a separate parallel implementation option is the better and least disruptive approach.

@mscdex, "-1 this kind of higher-level functionality is best left to userland" ... I have to fundamentally disagree with this premise and I'd really like us to get away from these kinds of blanket, substance-free arguments. We already have a high-level HTTP client in Node.js and discussions and ideas around improving or standardizing such are perfectly in scope for core... unless, of course, you're suggesting that we could just move the entire HTTP client implementation out to userland.

Second... We cannot implement a spec-compliant version of fetch in Node.js right now for a variety of reasons, not the least of which is the current lack of WHATWG streams support. We would need to address that issue, and others, as a prerequisite for this landing

Third... I'm -1 to vendoring in an implementation of this but it also needs to be recognized that we cannot simply take one of the native code browser implementations of this. To implement fetch correctly with acceptable performance, we will need to have an implementation of both WHATWG streams and fetch implemented specifically for Node.js. It will be a non-trivial exercise.

What I would suggest, as I have suggested before, it would be ideal to work on this in a separate working fork the same way we have successfully done with HTTP/2, modules, n-api, and most recently QUIC. I would suggest that separate working repo focus on three specific pieces that are going to be important here:

  1. A minimal introduction of bob streams to core with support for network i/o (/cc @Fishrock123)

  2. An implementation of WHATWG streams on top of bob streams (@MylesBorins @mcollina)

  3. An implementation of fetch on top of these

It will need to be recognized that the fetch implementation will have necessary differences from that in the browser... not the least of which will be the fact that in Node.js it will not be a global. It would likely be worthwhile for us to open a discussion with the WHATWG folks to see about iterating on the fetch specification.

In summary, http.request() is a legacy API that is no longer adequate in it's current form and needs to be replaced with something that works long term.

@devsnek

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

@jasnell sounds good overall, but can you just quickly clarify why you wouldn't want it to be global in node? i'm assuming for security+capability reasons but its good to be explicit.

@devsnek devsnek added the discuss label Jun 1, 2019

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Yeah, it is a combination of security and capability, really, with a heavy weight towards the security side. The current spec generally assumes a single-user model and the assumption just does not hold in Node.js

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

We already have a high-level HTTP client in Node.js

Exactly, that's why I believe this doesn't belong in core.

Another reason why I am -1 on bringing these kinds of browser APIs is because they often have dependencies on other browser-specific APIs that either don't make sense in node (e.g. they require a DOM) or are similar to something we already have in node (and duplicating such features can be confusing and/or cause performance issues due to spec/design).

I'm generally a proponent of a small core.

unless, of course, you're suggesting that we could just move the entire HTTP client implementation out to userland.

No, because that wouldn't make sense. HTTP is a low level protocol that is core to the internet that is also hard to get right, the Fetch API is not.

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

  1. A minimal introduction of bob streams to core with support for network i/o (/cc @Fishrock123)
  2. An implementation of WHATWG streams on top of bob streams (@MylesBorins @mcollina)
  3. An implementation of fetch on top of these

I agree that this is a good plan, but IMO it could be a bit more ideal than what we are capable to pull off if there is not any timing planned for this. bob has been proposed for a while but it it still not in core yet, if it becomes a blocker of fetch in core, what would be a reasonable estimate for us to complete the plan?

Note there are several new server-side JS runtimes (e.g. cloudflare workers, deno) that just expose some form of fetch implementation to users, even when not every section of the spec make sense for servers, and they don't do it for no reason.

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

About this discuss I believe if us based on a browser implement method fetch, example https://github.com/chromium/chromium/search?p=1&q=fetch&unscoped_q=fetch we have an implement more consistent

@renanbastos93 If this is talking about reusing the code from chromium...I don't think we could technically do that. The fetch in chromium is implemented in C++ which depends on too much chromium internals (at the very least they don't even use STL but use WTF instead).

@addaleax

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I think vendoring in the implementation – whether built by us or somebody else – is a great idea, and we should absolutely do it, similar to the way we did it for node-inspect. It makes having a separate pool of maintainers with lower barrier to entry a lot easier, and having separate code bases makes sense to me as well.

I’m also kind of against a C/C++ implementation – that’s just a sign that there are performance improvements left to be made in Node.js’ existing core, and maintainability is generally a lot better for JS-based implementations.

(Both of these reasons are why I approved this PR.)

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I would take a different approach to that argument @addaleax... Our core source tree is far too monolithic. I would like to see us move in the direction of splitting features into individual core libs that would be generally indistinguishable from vendored in dependencies rather than a big ball of mud.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

@mscdex @lpinca the arguments raised in the summit echo what James says here and you can find the discussion here.

Basically, I see two big API concerns that justify this:

  • There is no HTTP client for http2 or quic in core - we need an API for those anyway and if we can make it the same API for http, https, http2 and quic that would be awesome from a user PoV.
  • There is a promises API for our core APIs like fs/dns, I'd really like to avoid http.promises if we can for the http client if we can have the same API browsers do.

It's also worth mentioning that we can and should work with whatwg to see what parts of fetch we can make node compatible.

@lpinca

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

There is no HTTP client for http2 or quic in core.

This is not true. There is a client API for HTTP2. QUIC is a WIP. There are even userland module like https://github.com/grantila/fetch-h2.

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

This is not true. There is a client API for HTTP2. QUIC is a WIP.

See my earlier comment. There is a separate http2 low level api, and on the current trajectory there would likely be another low level api. The goal is a single high level api that works with http1, 2, and 3, that can be implemented without breaking changes to the existing impl, is standardized so we can avoid creating or extenuating Yet Another Node.js Specific API.

@lpinca

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

There is a separate http2 low level api, and on the current trajectory there would likely be another low level api.

This is what core should do in my opinion. Provide small and efficient building blocks.
If we didn't already have an HTTP(2) client I would have been in favor of fetch in core but the convenience of the fetch API which is sold as one of the reason to add fetch in core can be already built with the API we have.

@targos

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

But fetch is a building block. High-level browser libraries in the ecosystem use it under the hood.

@lpinca

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

Yes, when there is no alternative, or the alternative is XMLHttpRequest.

@bitinn

This comment has been minimized.

Copy link

commented Jun 2, 2019

Personally, I think nodejs should be involved in WHATWG stream spec, and Fetch is the best excuse.

See discussion summary here (I am not a part of the discussion, but it covers a lot of things people have gone over and over again...)

#19393 (comment)

@philsturgeon

This comment has been minimized.

Copy link

commented Jun 2, 2019

Yes let’s please stop calling this high level as a pejorative. It’s as low level as it gets, other HTTP clients use this like other languages rely on Net::HTTP or curl_exec() or other very unopinionated libraries.

Let’s also not use “there are already multiple node fetch implementations” as a argument against adding fetch to core. Standardisation is how you simplify a situation where there are loads of slightly different implementations, not an argument against doing it.

Making HTTP requests is a pretty common thing to need to do, and to have completely different ways of doing things in browser and Node puts a huge burden on tool vendors who need to now support both these approaches.

So please, let’s keep this thread about improving the implementation ration, we’ve already discussed all the pros and cons and I’ve not spotted any arguments in here that have not already been made in #19393 (all covered above).

If there are concerns with the implementation in the PR, good, there should be, it has a WIP glad and starts off saying “this is bad...” 😉

Let’s make it good instead of trying to talk the whole idea down. 🙌🏻

@benjamingr

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

@philsturgeon we haven't actually reached consensus to add fetch into core yet - objections about the API are certainly fine and unless we make a formal process (a wg, team or eps etc) it is entirely appropriate for people to raise objections here IMO

@philsturgeon

This comment has been minimized.

Copy link

commented Jun 2, 2019

I didn’t say it had reached consensus?

Objections based around the API sound great, I was advocating for that.

@3rd-Eden

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

A standard API that ships in other platforms is one of the primary reasons to adopt it. Lots of code is shared between environments and having shared interfaces is a major benefit.

This has never been a reason for Node.js to incorporate an API in it's core, so why should it now? It's 2019, that is 10 years after the first WebSocket implementation shipped in Google Chrome. And in all those years, an API never landed in the Node.js core. Instead we've had an healthy ecosystem of competing packages that was driving innovation. I've worked on Socket.IO, Engine.io, Primus, and ws, I used to be in camp "WebSocket in Core" but if we take a step back, we would have never had this amazing ecosystem with competing packages that were unique in their own ways, each with their own unique focus on either features, DX, or performance. IMHO that is what Node is about, empowering developers, to build and re-invent, to mix and match and creating a healthy competition to make some thing better.

Lets be perfectly honest here, the fetch API is far from perfect, I would even argue it's flawed and failed to deliver it's original promise as outlined in Google's original Introduction to fetch

fetch() allows you to make network requests similar to XMLHttpRequest (XHR). The main difference is that the Fetch API uses Promises, which enables a simpler and cleaner API, avoiding callback hell and having to remember the complex API of XMLHttpRequest.

When the specification was written, it excluded support for timing out and aborting requests. Functionality that IMHO is vital when you write an API client as you don't want to wait indefinitely for an HTTP server to answer. Later on they added support for AbortController, an new API that allows you to "signal" the fetch to reject. That means that if you want to have fetch in Node.js, you MUST also include AbortController or you will risk leaking HTTP requests as they cannot be aborted as that API was "forgotten". That means that we're no longer adding 1 API, but 2. But as others pointed out, we also need WHATWG Stream, EventTarget API's as part of this API, thats a total of 4 additional API's that the core now needs to maintain.

That means that we're not just shipping a worse implementation of http.request but also a worse version EventEmitter, and Stream, just for the sake of supporting HTTP/2/3/Infinity, or a different Non-standard API, which means you're back to the original issue.

TL;DR: 👎

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

This has never been a reason for Node.js to incorporate an API in it's core, so why should it now?

I may have interpreted this sentence incorrectly but do note that Node.js has already implemented URL / URLSearchParams, TextEncoder / TextDecoder and (parts of) the Performance Timing API (the first two are already shipped)

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@Joyee that we already have absorbed some webstandard APIs doesn't mean its our intention to. I suspect that each of the APIs you mentioned was argued for based on its own merits. Traditional url has many nice features, but its forgiving parser was too forgiving, and couldn't be changed without massive incompatibility. As you know, URL misses some requirements for node's use case that browsers don't have, but it works fairly well. We needed something like the performance reporting API. TextEncoder I guess is useful, I suspect its so unused by most devs and irrelevant to node services that noone cared to debate it.

I was (am?) resigned to fetch ending up in core, that it transparently works over http1/2/3 is particularly attractive. But... general intentions aside, the technical criticisms of it as an API are mounting. Perhaps I'm missing it, but I haven't seen any concrete proposals for how to deal with the fact it clashes with node's streams and event emitter.

@yordis

This comment has been minimized.

Copy link

commented Jun 12, 2019

Folks constructive criticism, I hope you understand.

If you all will argue if we should or shouldn't add the feature or anything unrelated to how to make this to work, please I beg you, use this thread #19393

Let's try to keep the conversation over here in a proactive way on how to tackle this, that means that we try to give feedback on how to outcome the challenges and try to come out with a solution.

Arguing about what is right or not, or limitations rather than how to fix problems will send us back to 0, it helps nobody.

I would rather fail to try to align the JavaScript community and prove to others that it is technically impossible than keep having this disagreement.

Please.

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@yordis #19393 was actually not opened for this kind of discussion either, see #19393 (comment) but evidently there is probably no way to discuss about the feasibility of implementing fetch without discussing about the merit of it first in core.

@MylesBorins MylesBorins referenced this pull request Jun 17, 2019

Open

lib: port rimraf to core as shutil.rmtree #28208

2 of 5 tasks complete
@ChALkeR
Copy link
Member

left a comment

The folder structure of node-fetch seems trivial, but there are four alternative versions there of which we need only one. I recommend removing the files we don't need.

In general, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.