stream: expose web streams globals, remove runtime experimental warning

[aduh95]

The web streams implementation seems to be quite stable, this PR propose to mark it as stable and to expose its class on the global object.

/cc @jasnell

Fixes: #40950

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[benjamingr]

I think it's probably too soon and web-streams still aren't used enough "in the wild" to graduate. I think there are also a few open bugs like. #39756

[benjamingr]

@aduh95 @jasnell that issue was just an example of an open web-streams issue. I just don't think they've seen enough adoption to be "stable" yet.

That said it's a spec and any violation is a bug fix anyway so I guess the "API" is stable (if probably still buggy?)

[aduh95]

@benjamingr yes, that was my understanding :)
I'm opening this PR now so it can land in v18.0.0, but it doesn't have to, we could wait for v19.0.0 (or even later) if you think v18.x is too soon. I have a personal preference for landing it earlier because v18.x will be LTS and not having them as globals might slow down their adoption, but I'm happy to defer to @nodejs/streams judgement.

[targos]

@benjamingr do you think it's too soon for stable status, for exposing as globals, or both?

[benjamingr]

do you think it's too soon for stable status, for exposing as globals, or both?

The stable status though that's just "we are giving users an API and telling them it's stable even though it isn't battle-tested and probably still has more bugs than other stable APIs". If we're OK with that then the API surface itself won't change (other than the spec itself changing).

[mcollina]

I'm +1 in marking them as stable. I'm -1 as exposing them as globals. I guess is that these adds quite a bit of work to our bootstrap without significant win for our users.

I might be ok with some form of lazy-load but I'm unsure if it's possible.

I'd also add that our implementation is likely quite slow, so I'm unsure if we should promote their usage. Ref. nodejs/undici#1203.

[mcollina]

I was supposed to hit the request change button.

[vweevers]

[globals don't have a] significant win for our users.

It hurts adoption. As an example, level-web-stream needs a conditional import and also in turn has to explain this to consumers. Globals would be a clear win here.

I'd also add that our implementation is likely quite slow

I can confirm (from a quick benchmark so I won't share numbers). That can be added to documentation though. Web streams have immense value even if they are slower. Let users make the choice.

[mcollina]

@vweevers you quoted only half of the paragraph. In this PR, there are quite a number of files that would be loaded during the bootstrap of Node.js.

Most users (currently) do not use those APIs so this loading cost is not well spent.

[vweevers]

If the lack of globals hurts adoption (that's debatable ofc) then there will be less users to justify the cost of globals. To be fair, there are other ways to increase adoption - for me it was @jasnell's talk at Node Congress about a standard library for JS. That talk pushed me to try out web streams, and the lack of globals was just a minor snag.

Is there a plan to reduce the bootstrap cost (with snapshots?) or is it an indefinite blocker?

[aduh95]

I guess is that these adds quite a bit of work to our bootstrap without significant win for our users.

Is it still true with snapshot? IIRC lazy loading has no upside (and has the downside of having more overhead), except to avoid circular dependencies.
Is there a way to measure the difference? Looking at the time it takes for make test to complete, it seems that PR doesn't have any noticeable impact on bootstrap performance (from 19'43" for acc11c9 to 19'28" for fc80fac).

In this PR, there are quite a number of files that would be loaded during the bootstrap of Node.js.

It adds three, based on https://github.com/nodejs/node/pull/42225/files#diff-eeda4d549f43051fb9ffbc8286e838ebfd607bae681dfc55a76679e6705124f9.

[lin72h]

+1 for expose Stream as global.

Reason 1:
Many future API like WebTransport, based on it, and I believe there'll be more to come

Reason 2:
Stream based programming model have been increasing popular inside or outside JS community, make it global, ease transition from community to Nodejs, examples are
Apple's Combine framework
IBM's red-node
React Flow
Video community's Touchdesigner
Audio community's bespoke

[aduh95]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1103/

No significant perf regression detected

                                                                                  confidence improvement accuracy (*)   (**)  (***)
misc/startup.js mode='process' script='benchmark/fixtures/require-builtins' dur=1                 0.25 %       ±1.67% ±2.22% ±2.89%
misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1                 0.82 %       ±1.57% ±2.09% ±2.73%
misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                     **      1.68 %       ±1.25% ±1.67% ±2.18%
misc/startup.js mode='worker' script='benchmark/fixtures/require-builtins' dur=1                 -0.46 %       ±2.66% ±3.55% ±4.64%
misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1           *      2.60 %       ±2.14% ±2.85% ±3.71%
misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                              0.35 %       ±1.70% ±2.27% ±2.95%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 6 comparisons, you can thus
expect the following amount of false-positive results:
  0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.06 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

[Jamesernator]

It would seem a bit odd to have Blob and fetch/Response to graduate to globals but not streams given that both blob.stream() and response.body both expose ReadableStream instances.

[GeoffreyBooth]

+1 to improved compatibility with browsers, and enabling more code that can run without modification in both environments. Especially since this doesn’t seem to entail any performance costs per #42225 (comment), I don’t see why we shouldn’t land this?

[jasnell]

@mcollina:

I'm +1 in marking them as stable. I'm -1 as exposing them as globals. I guess is that these adds quite a bit of work to our bootstrap without significant win for our users... I might be ok with some form of lazy-load but I'm unsure if it's possible.

If these are loaded as part of the startup snapshot then there really shouldn't be much cost at all during bootstrap.

I'd also add that our implementation is likely quite slow, so I'm unsure if we should promote their usage. Ref. nodejs/undici#1203.

That's a separate issue that should not block them graduating from experimental. Yes, the performance can be improved and node.js streams are definitely still faster, but graduation from experimental should not be gated on that factor at all.

[aduh95]

@mcollina Can you clarify your position on this given the last comments? Adding the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label to make sure we don't forget to discuss it further before the v18.0.0 semver cutoff.

[mcollina]

Yeah, I changed a bit my mind given the statement.
I'm +1 in exposing them as globals but not marking them stable as I would not recommend folks to use them right now (nodejs/undici#1203).
I would also remove the experimental warning.

[ronag]

I think we are also missing some fixes that might be semver-majory.

• Web Streams: reject pending reads when releasing reader #41506
• Locked stream should be cancellable #41159

[apapirovski]

@mcollina in terms of performance, I think we should have some sort of criteria for what qualifies as good enough to no longer be labeled as experimental in the docs. (If that's the only reason it's marked as experimental for now.)

Overall, I'm -0 on removing the experimental warning if we actually don't believe the performance is good enough for production usage. Especially if we're making this global and this is an API that isn't Node specific, folks might just use it without even checking our documentation. It feels like we're relinquishing our responsibility for producing an easy-to-use runtime and pushing off that responsibility to the user.

[mcollina]

Overall, I'm -0 on removing the experimental warning if we actually don't believe the performance is good enough for production usage. Especially if we're making this global and this is an API that isn't Node specific, folks might just use it without even checking our documentation. It feels like we're relinquishing our responsibility for producing an easy-to-use runtime and pushing off that responsibility to the user.

That's exactly my intent. Most developers won't care about performance but just about Web compatibility. As for Web compatibility goes, I think our implementation is solid, possibly it needs some work (see #42225 (comment)).

My take is that we need more feedback from people. Folks will not use it with the big warning in the console.

[jasnell]

+1 on making it global. -1 on graduating it. It's complex enough that it should remain experimental for a while, just in case

[apapirovski]

Most developers won't care about performance but just about Web compatibility.

I think that somewhat depends on the use-case. I would consider streams to be one of the main performance-sensitive pieces in Node.js but perhaps I'm mistaken. That said, IMO your undici-fetch story kinda highlights a similar concern.

[aduh95]

@mcollina does this look good to you now?

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43295/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43315/

[nodejs-github-bot]

Landed in 560cbc5