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 streams #39093

Open
ronag opened this issue Jun 19, 2021 · 19 comments
Open

Future of streams #39093

ronag opened this issue Jun 19, 2021 · 19 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Jun 19, 2021

We are now making progress with WHATWG (web) streams #39062. Which brings up the question, what is our long term strategy in terms of streams?

What is the preferred implementation for core modules? Do we always implement as node streams and then wrap to web streams? Or the other way around? If the other way around should we look into re-implementing existing apis?

Basically when do we use which streams type?

I haven’t personally looked at web streams so I don’t really have any opinions yet in regards to pros and cons relative node streams.

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Jun 19, 2021
@mscdex
Copy link
Contributor

mscdex commented Jun 19, 2021

We can't wrap node streams in web streams because of the enormous breakage that would cause (code expecting node stream APIs and even code utilizing private stream state).

@jasnell
Copy link
Member

jasnell commented Jun 19, 2021

fwiw, I have no intention of replacing node.js streams nor replacing or re-implementing any of the existing code that uses node.js streams. This is a separate additional API that is best treated separately.

To provide some background on the intent here...

For the QUIC API, I plan on supporting both WHATWG streams and node.js streams.

For instance, to open a QUIC stream and send data, there will be several options:

session.open({ body: 'hello' });
session.open({ body: new Uint8Array(10) });
session.open({ body: getNodeJsStreamReadableSomeHow() });
session.open({ body: getWhatWGReadableStreamSomeHow() });
session.open({ body: blob });
session.open({ body: fsPromises.open('...') });
// ...

respondWith({ body: 'hello' });
respondWith({ body: new Uint8Array(10) });
respondWith({ body: getNodeJsStreamReadableSomeHow() });
respondWith({ body: getWhatWGReadableStreamSomeHow() });
respondWith({ body: blob });
respondWith({ body: fsPromises.open('...') });

For receiving data, my intent is to largely implement the Body mixin

// "stream.*" here refers to the QUIC Stream object, not node or web streams...
stream.readable   // WHATWG ReadableStream
await stream.text()  // Promise<string>
await stream.arrayBuffer() // Promise<ArrayBuffer>
await stream.json()  // Promise<Object>

// With an additional option for reading as a Node.js stream
await stream.stream()  // Node.js stream.Readable()

// Async Iterator will also be possible
for await (const chunk of stream) {}

What is the preferred implementation for core modules? Do we always implement as node streams and then wrap to web streams? Or the other way around? If the other way around should we look into re-implementing existing apis?
Basically when do we use which streams type?

I don't think there's a clear answer. Any and all existing uses of Node.js streams remain untouched or extended at some point to also accept WHATWG streams. New API may support both if appropriate and possible. We should no be looking at re-implementing anything at all.

Specifically, introducing web streams does not need to have any impact whatsoever on any existing node.js streams. There's no intention of replacing node.js streams. No intention of deprecating those or even marking them legacy. There's no intention of making any breaking changes to Node.js streams and no reason to fear that any existing code will stop working.

@ronag
Copy link
Member Author

ronag commented Jun 19, 2021

So new APIs should preferably support both types? Old APIs should hopefully be updated likewise?

Usually the one that gets implemented “natively” in an API will get best performance while the other one gets wrapped. Which should we prioritize for new modules?

@julienw
Copy link

julienw commented Jun 22, 2021

Thanks for working on this!

My 2 cents as a user of the current stream API: if there's 2 non-deprecated competing stream APIs in node, we'll need clear explanations about which one a new project should use, and why (clear advantages / drawbacks for each of them).

My expectations about the Web implementation is that because it's well spec-ed (hopefully) I won't have to look at the source code as much as I had to for node's stream API, and I'll be less worried of breakage on major node updates.

@domenic
Copy link
Contributor

domenic commented Jun 23, 2021

For receiving data, my intent is to largely implement the Body mixin

Minor note: there are basically two styles we've seen in the web platform for this, and I don't think your proposal matches either.

  • The Body mixin, which has body (a WHATWG ReadableStream), plus methods like arrayBuffer(), blob(), text(), etc. Those methods have the side effect of draining the body stream, and can each only be called once.

  • The Blob class, which has separate methods stream(), text(), and arrayBuffer(). Those methods generate a new thing from the underlying Blob pointer, and can be called many times.

I'm not sure either model makes perfect sense for your case, so maybe your proposal is better than following either of those. But I'd caution you to try to avoid an uncanny valley where possible. So for example:

  • Having stream() return a Node.js stream.Readable might be a bit confusing for people who are used to Blob's behavior.
  • Having readable instead of body might be confusing for people who are used to the Body mixin's behavior.
  • Making the parent object (stream in your example) async iterable might be confusing for people who are used to having to do for await (const chunk of response.body) on the web.

But again, this is just a surface appraisal, and there could very well be deeper reasons for the API you propose!

@jasnell
Copy link
Member

jasnell commented Jun 23, 2021

@domenic:

But again, this is just a surface appraisal, and there could very well be deeper reasons for the API you propose!

Not all that deep :-) ... it's to be As Familiar As What Makes Sense to developers. And even then, the API here is still completely flexible so I'm open to suggestions on what would be better :-)

@zwhitchcox
Copy link

So, for me the answers is clear that Node.js should prefer web streams over node streams, as that is available in the browser, and the whole ethos of Node/JavaScript is centered around "write once, execute everywhere".

Ideally, there would be some type of efficient conversion method between the two, which could augment existing functionality like, e.g. fs.createReadStream with fs.createReadWebStream and fs.createWriteWebStream which take a web stream.

Or maybe there could be separate versions like fs/web and net/web based on Bob streams, but ultimately, I think first class support is necessary, with web streams being preferred.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

I very much disagree that's ever been the ethos of node; much code can only ever run in the browser and much code can only ever run on a server.

@zwhitchcox
Copy link

@ljharb What's your definition of "ethos"

From Google:

ethos: the characteristic spirit of a culture, era, or community as manifested in its beliefs and aspirations.

I'm not saying Node achieves perfect continuity of javascript between server/client, just that it's a goal. Node had to augment browser functionality that doesn't exist, but now that it does exist, it should adopt it as a standard, imho.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

@zwhitchcox your phrasing "whole ethos" is quite different from "it's a goal", the latter of which I agree with.

@zwhitchcox
Copy link

zwhitchcox commented Aug 24, 2021

It would be nice if it were supported by node natively. Me not being too familiar with the internals of node though, it seems like most of the work would be done here.

Could this be exported as a utility function as well maybe? cc @jasnell

@zwhitchcox
Copy link

Realized there's already an issue for this.

@jimmywarting
Copy link

jimmywarting commented Sep 8, 2021

I have no intention of replacing node.js streams nor replacing or re-implementing any of the existing code that uses node.js streams

Ditto, fs stuff can still use node:streams
what i would like to have is the new file system access so we can use the same kind of code everywhere and begin to follow an actual spec. even if it is worse then fs

@o-t-w
Copy link

o-t-w commented Sep 17, 2021

Usually the one that gets implemented “natively” in an API will get best performance while the other one gets wrapped. Which should we prioritize for new modules?

Seeing as people have been asking for fetch for years and fetch returns a web stream, I'd say give preference to web streams. Anecdotally, though obviously subjective, I've heard plenty of people say they find the web streams API simpler to understand than the Node one (I include myself in that).

https://twitter.com/sindresorhus/status/1407214304164814848?s=20

@jimmywarting
Copy link

I've heard plenty of people say they find the web streams API simpler to understand than the Node one (I include myself in that).

Same, I have worked with node for many years. i also think whatwg:streams are easier to understand and use.

  • it follows a spec
  • and it works the same way everywhere on all platform, so better cross friendly

@ronag
Copy link
Member Author

ronag commented Sep 17, 2021

Anyone done any performance comparisons?

@zwhitchcox
Copy link

So, for anyone reading this, I exported the code by @jasnell to its own module which contains the node internal code for stream adapters.

This makes it easy to convert to/from node/web-streams in a platform-independent environment.

I haven't tested it on the browser, but it should work...will test later. PRs welcome.

Note, that all node web-streams can be converted to/from web streams by calling toWeb or toNode on the stream.

cc @pimterry and @sam-stanford because I remember seeing them asking for this.

@zwhitchcox
Copy link

Forgot to mention the repo is https://github.com/zwhitchcox/stream-adapters

This should also work on previous versions of node without web stream support.

@jasnell
Copy link
Member

jasnell commented Sep 18, 2021

@ronag ... I will be doing some benchmarking but I'm not sure exactly when I'll be getting to it. I want to go back through the impl and find optimization points that don't sacrifice spec compliance.

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

No branches or pull requests

9 participants