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

Stable Fetch #1737

Closed
ronag opened this issue Oct 27, 2022 · 75 comments
Closed

Stable Fetch #1737

ronag opened this issue Oct 27, 2022 · 75 comments
Labels
fetch question [Use a Discussion instead]

Comments

@ronag
Copy link
Member

ronag commented Oct 27, 2022

What is left for us to consider fetch stable?

@ronag ronag added the question [Use a Discussion instead] label Oct 27, 2022
@ronag
Copy link
Member Author

ronag commented Oct 27, 2022

@KhafraDev @mcollina

@mcollina
Copy link
Member

I think we would need to make a call on #1203.

@ronag
Copy link
Member Author

ronag commented Oct 27, 2022

Performance is not a significant factor, in my opinion, for stability. I think for most use cases, the existing performance is acceptable. If performance is important you shouldn't be using the fetch api to begin with.

@silverwind
Copy link
Contributor

silverwind commented Oct 27, 2022

If performance is important you shouldn't be using the fetch api to begin with.

Performance and Convenience don't have to be exclusive. That's like saying if you want performance, you shouldn't use Node.js/JavaScript/Non-Assembly language 😉.

@ronag
Copy link
Member Author

ronag commented Oct 27, 2022

Performance and Convenience don't have to be exclusive.

I'm not saying that. I'm saying performance is NOT a MUST for a stable release while correctness IS a MUST.

@jimmywarting
Copy link
Contributor

jimmywarting commented Oct 27, 2022

performance is NOT a MUST for a stable release while correctness IS a MUST.

agree,

It would be a different if there is a memory leak due to some performence issue or something else then that is a bug and something that needs to be fixed before calling it stable.

Performence can be dealt with over time

@mcollina
Copy link
Member

I think we should provide a way to solve the cookie issue before calling it stable: #1437.

@KhafraDev
Copy link
Member

Until whatwg/fetch#973 is added there's not much we can do about cookie headers, otherwise we'll be stuck with a non-spec compliant method that will eventually be removed in favor of getSetCookie anyways. Of course if we implement getSetCookie we also have a problem where any changes done to it before it becomes stable will be a breaking change.

It's currently stuck on getting implementer interest from two browsers.

This should probably be handled by wintercg

@mcollina
Copy link
Member

I think fetch without cookies is not really useful.

Apart from that I think we can call in stable in undici and drop the experimental warning in Node.js. I would still leave it experimental there for a bit longer.

@KhafraDev
Copy link
Member

Once fetch is stable, would changes to the spec that changed behavior be considered breaking? If so, would there be a new major release of undici for otherwise seemingly minor changes? Ref: #1681

@mcollina
Copy link
Member

We should create a policy for this and possibly share it also with the TSC.

My 2 cents is that approved spec changes are not breaking, otherwise Node.js will end up bein significantly behind and we'd end up maintaining too many lines. However we should validate this with the TSC.

@ronag
Copy link
Member Author

ronag commented Oct 29, 2022

There is maybe actually on thing that can be blocking here. Blob and the lack of file system backing.

@KhafraDev
Copy link
Member

undici does support third party Blobs, so you could use fetch-blob for blobs backed up by the file system.

@ronag
Copy link
Member Author

ronag commented Oct 29, 2022

But I think it's a problem for FormData? Since then we always use Node blobs... Any reason why we don't just simply use fetch-blob?

@KhafraDev
Copy link
Member

KhafraDev commented Oct 29, 2022

FormData will use the FileLike class if Blob isn't a node Blob, so both are supported. I think it's the best solution currently - allowing both the global blob and third party ones.

However I think this issue is more of a node issue, since we don't implement Blob, nodejs/node#45188

@ronag
Copy link
Member Author

ronag commented Oct 29, 2022

Yea, I'm kind of thinking we maybe need nodejs/node#45188 to be done before fetch is "truly stable".

@mcollina
Copy link
Member

My 2 cents is that this is ready to drop the experimental warning in v19 and v18.

@mcollina
Copy link
Member

I think we can mark fetch as stable now.

@KhafraDev is there anything missing on your end?

Would you like to get a WPT report published on how spec-complaint this is?

@KhafraDev
Copy link
Member

KhafraDev commented Sep 19, 2023

WPTs should be ran automatically, the results can be found here: https://wpt.fyi/results/fetch?label=master&label=experimental&product=chrome&product=firefox&product=safari&product=deno&product=node.js&aligned&q=fetch

I don't think there's a way for us to determine how spec compliant we are -

  1. we don't run tests in child processes & workers, so at minimum we run half the tests that Deno does (for example). You can see this in the content-encoding tests from the link above, we pass all 8 tests, and Deno passes all 16.
  2. we don't run *.window.js tests, even if they might pass, because *.window.js assumes that the environment is a browser (has access to Window, document, etc.)
  3. we don't run tests that only work in browsers (CORS, ORB, etc) for obvious reasons. This is basically 90% of the tests.

But if we wanted a "marketing" number (😄) we pass 1282 / 1453 tests we run, or 88.2% pass.

@Ethan-Arrowood
Copy link
Collaborator

Info like that @KhafraDev is amazing! We can use that in WinterCG in order to adjust the forked fetch spec regarding what is and isn't possible in non-browser environments. Nice!

@mcollina
Copy link
Member

Any opposed to mark it as stable?

@ronag
Copy link
Member Author

ronag commented Sep 21, 2023

SGTM

@KhafraDev
Copy link
Member

KhafraDev commented Sep 21, 2023

I am, but I haven't had time to write out my concerns. More or less I don't think that node's stability index is a good match for the fetch spec, which changes much more frequently than other specs. I also think it'll make it harder/impossible in some cases to bring bug fixes and new features to older versions of node.

I plan on making a PR to expose undici's WebSocket in node core once undici is updated. I'm assuming there will be a lot of bug reports. If fetch gets a breaking change, we can no longer land bug fixes in node v20 or v18, even if it's unrelated to fetch.

Hypothetically, if fetch was made stable in v18, we wouldn't be able to add WebSockets until v21/22 because undici had a breaking change in one of the v20 releases. Since fetch is "experimental", WebSocket will probably be in one of the v20 releases. edit: I made a PR that added WebSocket in a semver minor change that would not have been possible if fetch was stable.

@Ethan-Arrowood
Copy link
Collaborator

I'd be open to having a discussion about like marking Web APIs as "stable", but they are not enforced by SerVer since they are based on the specs. I.e. Fetch or WebStreams can received breaking changes within a Node.js minor. Just like how a web developer wouldn't necessarily be able to specify exactly which fetch/webstream implementation they want to use. They'd always be subjected to whatever browsers are shipping at that time

All that said, I do believe web spec authors rarely add breaking changes for this exact reason. Can you imagine what would happen if Fetch had a breaking change? The entire web would break overnight.

@GeoffreyBooth
Copy link
Member

what @KhafraDev is proposing is to keep it experimental forever.

I understand. But do we know where the future changes are likely to occur? As in are there parts of the overall fetch API that we feel confident won't change, that we can call stable while defining other parts as experimental?

@mcollina
Copy link
Member

I think it's impossible to predict.

@KhafraDev
Copy link
Member

KhafraDev commented Sep 26, 2023

Formdata (part of the xhr spec and not fetch) and Headers are unlikely to change. Of course it's possible, but they're relatively simple key value stores that would break everything if they had a major change.

@mcollina
Copy link
Member

I disagree to keep things experimental forever. This has lead to massive issues in the past, and we should avoid to repeat this mistake.

@KhafraDev
Copy link
Member

KhafraDev commented Sep 26, 2023

I'd be interested in some backstory about those issues. I don't think fetch should be experimental forever either, I am arguing in favor of a new stability index. However, as long as there isn't an alternative, experimental makes more sense for fetch than stable.

@mcollina
Copy link
Member

async_hooks being the most prominent, it has been experimental forever, it's relied as production-ready by businesses across the globe.

@spersico
Copy link

No-one asked me, but I'm putting my 2 cents here:

The options laid out by @mcollina are, IMO, the ones that should be considered:

  1. fetch() (and all other WHATWG specs) breaking changes should land only in Node.js and undici semver-majors releases?
  2. fetch() (and all other WHATWG specs) breaking changes are actually bugfix to rectify a spec compliance, and therefore can go into minor or patch releases?

An evolution of a spec does not mean that the current status of the spec is not respected. If there are no issues with the fetch API as is, and it's compliance with the current spec, I don't see why we should consider even inserting a new stability index, when it's the spec, the thing that's actually changing, and not the API.

Keeping this as experimental, is detrimental to Node as a whole because it pushes users to rely on user-space solutions that probably will not be maintained with the same care for stability, performance and security that this is being handled with (hello, Axios 0.x).

Lastly, what's stopping adding the stability index after marking this as stable? It's an index that would affect every WHATWG spec, isn't it?

@KhafraDev
Copy link
Member

KhafraDev commented Sep 26, 2023

the spec changes the api.

@styfle
Copy link
Member

styfle commented Sep 26, 2023

new changes can land about as frequently as they do in browsers, and there's no worry about .

I don’t understand this part.

Are you saying that browsers regularly ship breaking changes to fetch?

If so, then it sounds like no one noticed in browsers and therefore wouldn’t notice in Node.js either.

@spersico
Copy link

the spec changes the api.

When I say API, I mean the interface of the implementation, not the specification that the implementation should follow (the spec).
An implementation (in this case, undici/fetch) might choose to follow a specification (fetch), but that only means choosing to align the implementation's API, with the spec-defined API.

So, in short, you have the spec's defined API and the implementation's API.

My point is: changes in the living spec could hold this forever in the experimental category (because it's a living spec), when the stability of the implementation it's a different and independent thing, that can be handled with SemVer perfectly fine.

@KhafraDev
Copy link
Member

Are you saying that browsers regularly ship breaking changes to fetch?

Browsers release new major versions every few weeks/1-2 months. Chrome has 2 releases in October itself, according to this. I don't know how long it takes to ship in browsers though or if they follow semver, but you could probably expect breaking changes if they implemented the changes to the spec.

If so, then it sounds like no one noticed in browsers and therefore wouldn’t notice in Node.js either.

People have already noticed breaking changes we made here. If you search for duplex or duplex: 'half' github's search might be good enough to show you many such issues. I'm assuming it would be harder to find the issue someone reported when we started removing authorization headers from cross-origin redirects, but they are noticed.

that can be handled with SemVer perfectly fine.

The point about a living spec is great and I agree that currently fetch should remain experimental. It could follow semver, but it's already causing issues (read further up, there are already changes we cannot merge now that fetch is stable). Is it worth it to have a fetch implementation that isn't compliant with the spec?

async_hooks being the most prominent

Async_hooks doesn't follow a spec, I don't think it's a fair comparison with fetch.

@mcollina
Copy link
Member

Is it worth it to have a fetch implementation that isn't compliant with the spec?

If a user cannot port their code between browsers and servers... fetch is not useful. Ultimately users would need to keep their browsers up-to-date. Therefore we would need to keep fetch in sync with the status quo.

As a Node.js user, I don't want my application to break because I upgraded Node.js, especially a LTS release. I also think we should strive to have the most compliant implementation possible.

We need to reach a trade-off to the stability required by devs and businesses across the globe.

I'm personally leaning towards considering spec changes minor changes, and ship as usual.
The target use of fetch() is isomorphic JS after all, and if the Browsers change, so should we. We want to avoid the case where a code that run on modern browsers cannot run on Node.js LTS releases.

I'm also ok shipping new undici major for each fetch spec breaking change, and then follow the usual process to ask the TSC for approval for landing the breaking change on a case-by-case basis. We know these cases will happen.

@targos
Copy link
Member

targos commented Sep 27, 2023

Correct me if I'm wrong, but I think we already shipped "breaking" changes in LTS to the WHATWG URL implementation to follow spec changes.

@silverwind
Copy link
Contributor

silverwind commented Sep 29, 2023

If a user cannot port their code between browsers and servers... fetch is not useful

Only trivial cases will be portable anyways. Browsers have CORS and HTTP connection limits which are not active on server. Server has to worry about proxies (until #1650), which browsers do not have (because browser proxies are configured transparently from JS viewpoint).

@KhafraDev
Copy link
Member

KhafraDev commented Sep 30, 2023

If breaking changes can land then ultimately I have no issue, but it still makes little sense to call it stable if breaking changes can land, right? I think a stability index like "living standard" would be much better, stating mostly the same as stable:

Stability: 2 - Stable. Compatibility with the npm ecosystem is a high priority.
Stability: ? - Living Standard. Compatibility with the spec is a high priority.

If I'm being honest, I expect that eventually I will be screwed by semver if it's kept stable. Some change can't land because there's some disagreement and then we won't be able to land undici updates for multiple months to a year.

@jimmywarting

This comment was marked as off-topic.

@mcollina

This comment was marked as off-topic.

@ronag
Copy link
Member Author

ronag commented Oct 16, 2023

I see a problem here: people are overusing fetch for non-isomorphic cases instead of using e.g. undici.request. In this case the breaking changes can become quite problematic. Making nodejs seem like an "unstable" platform for developing back-end services.

I think it's important that we add more warnings and information to our documentation of the intended use for fetch, what pros/cons it has and what alternatives we recommend and why.

@Ethan-Arrowood
Copy link
Collaborator

Those same documentation changes need to also live in Node.js' official docs

@ljharb
Copy link
Member

ljharb commented Oct 16, 2023

@ronag this is the exact kind of problem i warned about when node chose to keep the name “fetch”, fwiw. Isomorphic/universal code is not the default, node-specific code is, and authors of that code are the only ones who can safely declare that their usage works universally. At this point, short of renaming fetch in node to something else, documentation updates are the best option remaining afaict.

@jaydenseric
Copy link

Node.js should support and promote web standard APIs, and Node.js should immediately publish updates for compliance with changes in web standards. If such changes are a breaking change, then Node.js should cut a semver major release. Node.js should not be “saving up” major changes for an arbitrary 6 month major release cycle. Browsers update all the time, and so should Node.js, Deno, Bun, or any other web standards aligned JavaScript runtime.

For many years Node.js had poor support for web standard APIs like fetch, File, Blob, etc. so the conservative and arbitrary release cycle made more sense then as Node.js had control over any changes to the API and could plan their release accordingly. Now that keeping up compliance with web standards is a goal, it doesn’t make sense to attempt to control the release cycle in the same way as before.

I’ve been working with Node.js professionaly for many years now, and never found any value in having “LTS” releases. That concept has only ever been annoying, because certain cloud providers would refuse to make available anything not labeled “LTS”, or internally people would procrastinate updating Node.js because of it and you miss out on new features, perf improvements, etc, or you would find yourself waiting months for a change you really look forward to. It reminds you of how back in the dark times “enterprises” would try to control and plan web browser releases, and employees would have to wait years to have their IE6 updated to IE10. Now we take for granted that browsers update all the time, and man, what a difference that has made to the world and our lives as developers.

@KhafraDev
Copy link
Member

I'm trying to avoid putting more work onto the releasers, and having a new release cycle similar to browsers would not be good.

@jimmywarting
Copy link
Contributor

jimmywarting commented Oct 18, 2023

Agree with jaydenseric on #1737 (comment).

If Node.js were a browser, people would quickly label it as Safari has become: "Safari is the new IE." Not because of a lack of new web/JavaScript features or coming up with its own ideas or having bugs in it, but due to the slow release cycle and the absence of auto-updates. Safari is tied to the macOS releases update, which is rarely updated.

The Node.js release cycle was fine in the beginning, but now that the goal has shifted more towards shipping more and more web standards and following a specification for compliance, the less current release cycles make less sense. Node.js is not automatically updated, so some library authors are frustrated that they still need to support very old NodeJS/V8 versions and has to resort to older solution and lots of polyfill & shims for not being able to use the newest and shiniest feature. It's like still having to support IE10 today.

@SimenB
Copy link
Member

SimenB commented Oct 18, 2023

some library authors are frustrated that they still need to support very old NodeJS/V8 versions and has to resort to older solution and lots of polyfill & shims for not being able to use the newest and shiniest feature. It's like still having to support IE10 today.

Do you have any examples of projects/authors that are stuck on IE10 level shims/polyfills due to Node's release cycle? fetch is already a global in Node, so not sure what's blocking them.

Auto-updating server runtimes seems weird to me - why would you want this?

@jimmywarting
Copy link
Contributor

jimmywarting commented Oct 18, 2023

Do you have any examples of projects/authors that are stuck on IE10 level shims/polyfills due to Node's release cycle? fetch is already a global in Node, so not sure what's blocking them.

I was just talking in general, i did not have any example in mind. and i don't know who is stuck with ie10. it was just a metaphor. i could replace ie10 with phantomjs...

I know some authors who follow/support the LTS release cycles and uses for instance rimraf instead of NodeJS newest rm recursive mode. so they can't remove rimraf b/c they still need to support other runtimes.
gulp / express is very old fashion and never switch to newer stuff - do you know why? cuz developer do not upgrade there NodeJS version and they support them too.

if you are building an application then you can pick and choose whatever nodejs version you want, then auto update dose not matter. But if you are building a library and publishing something on npm for others to use. then you can't exactly use new feature that exist in NodeJS v18+

so you can't reliable depend on fetch or fs.openAsBlob cuz you have to support older env. that's what's a #developer-pain


Both of us might use gulp (who have a dependency on rimraf), you might use NodeJS v6 and be fine by that. where as i use v16 and don't wish gulp to use rimraf or a polyfill for something like string.prototype.repeatAll. cuz i think that leads to just more javascript fatigue
by you not updating make is so that gulp wants to still maintain backwards compatibility.

So i have to indirectly pay the price for others who dose not update their nodejs version.

@SimenB
Copy link
Member

SimenB commented Oct 18, 2023

But if you are building a library and publishing something on npm for others to use. then you can't exactly use new feature that exist in NodeJS v18+

Of course you can, nothing is stopping you from saying what versions of Node your library supports. Node <18 is also EOL, so supporting older versions in new code shouldn't be an automatic choice, unless you think that maintenance burden is fine.


This became a bit off topic so I'll stop here, sorry folks 🙏

@mcollina
Copy link
Member

mcollina commented Dec 3, 2024

This is done, closing.

@mcollina mcollina closed this as completed Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetch question [Use a Discussion instead]
Projects
None yet
Development

No branches or pull requests