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

Implement window.fetch into core #19393

Closed
thecodingdude opened this issue Mar 16, 2018 · 255 comments
Closed

Implement window.fetch into core #19393

thecodingdude opened this issue Mar 16, 2018 · 255 comments
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem.

Comments

@thecodingdude
Copy link

thecodingdude commented Mar 16, 2018

Edit: Fetch is available through the --experimental-fetch flag in Node. This is still very new :]


Edit: please note that this issue is pretty old and a lot of the information in the first few comments isn't up to date - for current status please see #19393 (comment) .


https://github.com/bitinn/node-fetch

It would make sense if window.fetch was implemented into core. it seems to be a stable enough API that would make a good candidate for inclusion. Not sure what the process is from here but thought I'd raise an issue :)

@jasnell jasnell added the feature request Issues that request new features to be added to Node.js. label Mar 16, 2018
@jasnell
Copy link
Member

jasnell commented Mar 16, 2018

this has come up from time to time but has not had much traction just yet. Let's see what folks think tho :-)

@jasnell jasnell added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. labels Mar 16, 2018
@devsnek
Copy link
Member

devsnek commented Mar 16, 2018

bradley and i were discussing this in a roundabout way on the subject of importing from urls. if that feature was introduced (and i think in general we do want it) we would need to implement this: https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script which uses the fetch spec. as another note if this was added in core i would want to pull in an existing c++ implementation from one of the browsers. however at a bare minimum node will definitely be adding Request and Response objects, it just might not add a function called fetch

@mscdex
Copy link
Contributor

mscdex commented Mar 16, 2018

-1 this kind of higher-level functionality is best left to userland

@guybedford
Copy link
Contributor

This would definitely be useful in simple cross-platform APIs and I think is what a lot of people use node-fetch for already. Also it would be nice if HTTP/1 v HTTP/2 negotiation can be handled automatically like in browsers as well.

@styfle
Copy link
Member

styfle commented Mar 17, 2018

I would love this! ❤️

Isomorphic JS is one of the big reasons people who start with JS on the front end, eventually pick up Node.js on the backend.

You get to run the exact same function in the browser and the server and the one place of contention I keep finding is window.fetch.

Some of the code that runs on the server and client needs to make HTTP requests to another server (think microservices with server side rendering and client side rendering).

One case for bringing it into core is that making HTTP requests (client) is closely tied to responding to HTTP requests (server).

And we now have isomorphic URL parsing so now all we need is fetch! Let’s make fetch happen!

@mikemaccana
Copy link
Contributor

Fetch is 'low level' according to it's author and major supporter hence missing a bunch of features like support for content types, JSON not being default, no query string encoding. Apps that need a high level quality HTTP client available in all JavaScript environments can continue using superagent.

@devsnek
Copy link
Member

devsnek commented Mar 17, 2018

@mikemaccana the fetch we are talking about is https://fetch.spec.whatwg.org/ and i don't think its appropriate to be plugging other http libraries

@mikemaccana
Copy link
Contributor

mikemaccana commented Mar 17, 2018

@devsnek Yes I know, that's the one I was specifically referring to. I don't have any particular enjoyment of superagent asides from it being:

  • a full featured HTTP client
  • available in node and the browser
  • more popular than fetch
  • has JSON as a default
  • encodes query strings
  • uses content types to determine response body
  • uses HTTP verbs as method names, so you can happily .get() and .post() things rather than 'fetching with method POST' which is a somewhat odd mental model

If fetch supported these I'd suggest it be included in node. To repeat: I've asked fetch's author and main proponent, Jake Archibald, why it doesn't support these things and it's stated that fetch is designed to be a low level API and things like sensible defaults can/should be added by higher level APIs. As a result I see no reason to use fetch in node, or in the browser.

@joyeecheung
Copy link
Member

joyeecheung commented Mar 18, 2018

@mikemaccana

These are actually desirable properties for the argument of including fetch in Node.js core, if we want to keep the core low-level and small. I believe we will need to eventually provide a promisified API for http/http2 anyway, and fetch as an existing spec for a similar set of low-level functionality as our http is something worth considering. In my experience the missing pieces in fetch feels pretty similar to the missing pieces in Node.js's http, the major difference is that you have a spec'ed API to count on.

Also I kind of doubt the "more popular than fetch" part, I don't think there are as many people using super-agent in the browser as in Node, given that fetch simply exists in the browser (modulo situations needing polyfills)?

Although, before we start implementing fetch, I believe we will need to introduce the stream API in the browser? Is introducing yet another stream in Node.js on the table?

The one time I've been forced to resort to XHR in the browser was when I needed progress, although I think with the browser's ReadableStreams it's possible to do the same thing with an API that's a bit awkward (res.body.read().byteLength)- if I'd implement progress in Node.js I think I'll need to use chunk.byteLength from the chunk being emitted in the data event, which is where the difference between the two streams start to matter.

Also, the fetch spec does not seem to include timeout or agents (yet?) at the moment, there might be more functionalities missing compared to our existing http API. For reference, node-fetch seems to implement these non-standard options. Again, not sure if our implementation should implement non-standard functionalities even if they supply the missing pieces compared to the old API.

@joyeecheung
Copy link
Member

joyeecheung commented Mar 18, 2018

also cc @TimothyGu you might be interested?

@joyeecheung
Copy link
Member

joyeecheung commented Mar 18, 2018

@thecodingdude

entirely disagree; Node uses v8, and by extension, should implement as many as v8 features as possible that make sense. fetch is one of those where developers wouldn't need to npm install request or node-fetch which are very popular libraries so this functionality warrants being in core.

Technically fetch is not a v8 feature though, it's an API spec'ed by WHATWG, whereas v8 implements ECMA-262, a spec by ECMA TC39 - if v8 implemented fetch then there would not be this feature request, because we basically just expose what v8 exposes.

BTW: I don't think we are talking about pulling npm packages into the core? Rather, we are talking about implementing the spec on our own and pulling in the WPT to test compliance, possibly with a bunch of code written in C++, much like what we did for WHATWG URL.

@mikemaccana
Copy link
Contributor

mikemaccana commented Mar 18, 2018

@thecodingdude the continued popularity of non-fetch libraries isn't a personal opinion, it is a fact - superagent had 1.6 million downloads this week . Nor is the lack of reasonable high-level defaults in fetch: again (again) that is acknowledged by fetch's author. Please don't reframe verifiable objective technical facts as irrelevant subjective opinions because you do not like them.

please don't plug other libraries here, they are irrelevant to our discussion.

developers wouldn't need to npm install request or node-fetch

😂👍

@bnoordhuis
Copy link
Member

I'm personally -1 for two reasons:

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

  2. fetch() is not flexible enough to support things like proxying, custom TLS certificates, etc. It's handled transparently by the browser but that won't work in node.js.

currently, 84 people agree

I wouldn't put too much stock in that. GH polls are easily gamed through Twitter brigading and whatnot. I'd take them more seriously if it cost a dollar to cast an upvote.

@mikemaccana
Copy link
Contributor

mikemaccana commented Mar 19, 2018

thecodingdude I have commented with a bullet pointed list of technical reasons why fetch is unsuitable as a general purpose HTTP client. I am aware you "do not care how/good bad fetch is". I think this is wrong: node should pick and adopt quality APIs. That you're personally uninterested in fetch's quality does not make it irrelevant to the discussion. The browser vendors were comparing fetch to window.xhr, node is not.

@Jamesernator
Copy link

@mikemaccana Why does fetch need to have high level features like "HTTP verbs as method names" for it to be included in core? You can trivially write your own methods that do this, in addition to the other high level features you've mentioned.

The nice thing about having standard APIs shared across both the browser and Node is that by writing the high-level API to target the low-level one (in this case fetch) you can get consistent behaviour much more easily than having to make your API work around differences between different APIs in node and the browser.

@devsnek
Copy link
Member

devsnek commented Mar 19, 2018

@thecodingdude any reason this was closed? discussion still seems to be moving

@thecodingdude
Copy link
Author

thecodingdude commented Mar 19, 2018

@devsnek yeah, I am not satisfied with the way this discussion and issue went, and I realise that github issues are simply not the appropriate forum for discussing whether features should land into core. I much prefer the node-eps since that was a proper document outlining the proposal. My intention was to gather the thoughts of the core team, and whomever else is involved in merging features into core (planning, coding, testing etc). Instead the discussion here so far is basically meaningless. It should be about feasibility, what would be required, and when such a feature could be merged.

Go take a look at this issue and try and follow the avalanche of conversation about websockets in core. There's too much noise and not a straightforward approach to proposing new features. I think PHP's rfc system is a good example of getting new features added. Clear, concise, accountable.

I'm down for discussing an rfc and its merits in a separate issue but it's clear this isn't going anywhere, with little to no interaction from TSC or anyone else that matters so there's no point leaving it up just to spam people's inboxes.

I'll refrain from making such issues in the future, my bad.
cc @jasnell

@mikemaccana
Copy link
Contributor

@Jamesernator Why does fetch need to have high level features like "HTTP verbs as method names" for it to be included in core? You can trivially write your own methods that do this, in addition to the other high level features you've mentioned

Sure. I, and everyone else that needs to use query strings, can write a method to encode query strings. Better yet: let's include a good quality, batteries included HTTP client in node where everyone doesn't need to fix it.

@thecodingdude looks like you deleted your comment, but to answer anyway: HTTP2 is necessary because it is required to speak the current version of HTTP. fetch is not necessary, because it is but one of many HTTP clients, and not a very good one, for the reasons discussed.

@Jamesernator
Copy link

@mikemaccana But the reason for including fetch in Node core isn't about batteries included, it's so that you can write code that targets both the browser and Node at once. That's why Node also supports the URL constructor (and it's even now global like in the browser in node 10) and the docs even refer to the old url.Url objects as the legacy url API.

This is a good thing, as it means some module that uses dynamic import for instance they don't need to include any library to be able to load resources relative to it e.g.:

...

export default async function isWordForLanguage(language="english") {
    const wordDataUrl = new URL(`./resources/${ language }.csv`, import.meta.url)
    const words = await loadSomehow(wordDataUrl)
    return makeIsWord(words)
}

Now that code has no platform specific behavior (assuming a loadSomehow method which could potentially be fetch). The benefits to this should be fairly obvious:

  • Using APIs that are standardised and included in both the browser and Node means you'll get support in both contexts if there's any issues with implementations (and hence no chance of dead or deprecated libraries)
  • Smaller libraries for high-level features as they don't need to smooth over platform differences, they just use the universal API (URL or fetch or whatever) and build directly on top of that
  • Ditto for your own libraries that need high-level behaviour

I think it's a pointless endeavour to create another http library in Node if it isn't for interoperability. If Node creates another one that isn't in the browser then you simply get the same situation you have now, if you want browser inter-operable code you another library built on top of it just to get interoperability with browsers.

If you think it's important for browsers to support high-level APIs out of the box then it's probably worth supporting the efforts to make high level APIs part of the browser and make clear your goals what you want to see out of high-level APIs. I'm sure many developers would appreciate getting high-level API specifications.

Even better would be if there was a way to get Node and Browsers to collaborate on high-level features, I'm not sure how this might work but if it could be done then it'd be a lot easier to propose high-level APIs that target all environments instead of being silo-ed into Node or the browsers.

@mikemaccana
Copy link
Contributor

Thanks @Jamesernator - it's nice to know there's an effort to resolve this on a larger scale. Thanks also for being polite and actually reading my comments before responding.

@guybedford
Copy link
Contributor

I tend to think that were NodeJS to have been developed today, there's no doubt that fetch would have been implemented in the name of aligning with browser APIs. Yes there are some really interesting edge cases, but @TimothyGu has handled these really well in the node-fetch integration.

@thecodingdude do you mind if I reopen? We potentially could still have things line up here - I don't think the detailed discussion has run its course fully quite yet and I think there's still fruitful discussion to be had personally.

@bnoordhuis node-fetch handles the differences here by supporting the NodeJS-specific agent option that delegates features like proxy https://github.com/bitinn/node-fetch#options. It breaks the universal API, but could also be done conditionally quite easily, so I tend to think it seems a fairly good compromise with this stuff. It could also be decided that such a divergence is a bad idea as well though certainly.

The key thing though is that this fetch API is purposely restricted - HTTP and HTTP/2 APIs still remain fundamental, this could just provide the easy universal experience. 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.

@thecodingdude
Copy link
Author

thecodingdude commented Mar 25, 2018

@guybedford feel free to make a new issue discussing this further if that's the direction that is most suitable. I'd much prefer technical discussions about the implementation itself, and not what fetch does/doesn't do vs alternatives which is why I closed the issue in the first place.

Quite honestly, I don't see why this discussion needs to happen on github - at the least is should be locked to collaborators who are going to be working on the code and discussions focused entirely on implementing fetch, identical to the browser where possible, and nothing more and nothing less.

@guybedford
Copy link
Contributor

@thecodingdude I'm still not quite sure I follow why you need a new issue here. Discussing the problem boundaries should surely be the first step for any new feature, and allowance should be made for a wider discussion at the start as well. 16 comments hardly seems off the rails as well.

Certainly the technicalities of whether it should be JS or C++, or where to draw the line on pooling and proxies are important, but things have to run their own course.

@bnoordhuis
Copy link
Member

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.

@guybedford
Copy link
Contributor

Thanks @bnoordhuis these are important points as to where the line is in Node, and I must admit I'm not familiar with these sorts of discussions historically, but so long as Node strives to provide core utilities, this seems like an important one to include to me.

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

Currently node-fetch is built on top of the http module, I'd personally prefer such a JS-based implementation here, working from exactly what @TimothyGu has already built as one of the most popular libraries. The pattern of having high and low level APIs for the same thing is an established one.

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

Convenience is a hugely important argument, the question is by how much to justify the maintenance costs. Node doesn't just support modules and native bindings - it provides server utilities and universal APIs where appropriate. As I've said, fetch seems to me a critical part of the universal toolbox these days.

I'd even push for fetch to transparently integrate HTTP/2 in future, allowing such a feature to not only build on top of the existing builtins, but also to provide a unification that we don't currently have. I know there are complexities to be tackled here, and it's not a given, but it feels like it would be huge win for Node and its users.

@Ethan-Arrowood
Copy link
Contributor

There's an branch on undici-fetch where I was starting to do a whole "run node-fetch tests using a different fetch implementation", but I stopped working on that to focus on v1 and benchmarking.

There's also a fetch headers class hopefully landing in node core soonish; I've been very very busy at work and haven't had time to work on it recently but it is top of my priority list when I get time for open source again (shouldn't be more than a week or two at most)

@jimmywarting
Copy link

@Ethan-Arrowood That's grate news, more spec'ed things to make fetch happen in core

@devsnek
Copy link
Member

devsnek commented Jul 11, 2021

i realized core fetch means core instantiateStreaming :dab:

@jimmywarting
Copy link

One thing many have asked for in node-fetch is the ability to measure speed. We have long thought of plugin system, but really the right way to go according to some spec is to implement PerformanceResourceTiming I know Deno is looking into it also. Would be cool to have a spec to follow that works the same across everywhere

@ronag
Copy link
Member

ronag commented Aug 11, 2021

We're pretty close to landing an initial fetch implementation in https://github.com/nodejs/undici (which we are discussing whether to include in core). Jump over to the PR if you want to help with review.

@tom-sherman
Copy link

@ronag Where can I find discussions about including undici/fetch in core?

@jfsiii
Copy link

jfsiii commented Sep 15, 2021

@tom-sherman #38533

@tom-sherman
Copy link

Now that the undici-fetch implementation has landed, can work begin on including the fetch (and friends) as globals? I'd love to lend a hand with this

@mcollina
Copy link
Member

I'm not sure we are exactly ready yet, I would prefer to have a few things stabilize in core and undici before adding to core.

If you would like to help, check out https://github.com/nodejs/undici/issues?q=is%3Aopen+is%3Aissue+milestone%3A%22stable+fetch%22.

@voxpelli
Copy link

voxpelli commented Sep 15, 2021

If the conclusion here is that when/if window.fetch is going to be implemented in core, then it's going to be through undici, then I guess this issue can be closed in favor of #38533 as that one is discussing how to move forward with the future of the Node HTTP Client and how undici fits into that and how it can be the way forward.

It in turn depends on nodejs/TSC#1041 / #39779 which discusses the principles around what belongs in core or not.

Anyone of a different conclusion?

@mcollina
Copy link
Member

I concur with @voxpelli

targos added a commit to targos/node that referenced this issue Jan 30, 2022
targos added a commit to targos/node that referenced this issue Jan 31, 2022
GeoffreyBooth pushed a commit to targos/node that referenced this issue Feb 1, 2022
@SimenB
Copy link
Member

SimenB commented Feb 1, 2022

I never thought to see the day. Thank you! ❤️

@sillyslux
Copy link

Is there still time to rename it? In electron we suddenly have two different fetches right next to one another. If nobody else, i know i'll be confused one day.

@Repugraf
Copy link

Repugraf commented Feb 2, 2022

We all know it takes effort to make things work and not break anything.
You had to overcome pressure from a lot of ungrateful people not seeing the full picture.
So thank you, guys. We all appreciate your hard work.
Keep it up!

@mbodm
Copy link

mbodm commented Feb 2, 2022

We all know it takes effort to make things work and not break anything.
You had to overcome pressure from a lot of ungrateful people not seeing the full picture.
So thank you, guys. We all appreciate your hard work.
Keep it up!

Sign that!

Thx for all your hard work, to keep Node going. Thx a lot!

ruyadorno pushed a commit that referenced this issue Feb 8, 2022
Fixes: #19393

PR-URL: #41749
Refs: nodejs/undici#1183
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this issue Apr 13, 2022
Fixes: nodejs#19393

PR-URL: nodejs#41749
Refs: nodejs/undici#1183
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this issue Apr 20, 2022
Fixes: nodejs#19393

PR-URL: nodejs#41749
Refs: nodejs/undici#1183
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 21, 2022
Fixes: #19393

PR-URL: #41749
Backport-PR-URL: #42727
Refs: nodejs/undici#1183
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests