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

stream: implement WHATWG streams #39062

Closed
wants to merge 2 commits into from
Closed

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Jun 17, 2021

This is an initial Work in progress Experimental implementation of the WHATWG streams standard.

The intent is for this to be a complete spec compliant implementation. This PR includes the web platform tests.

It is my intention to...

  1. Use this as part of the QUIC/HTTP3 implementation, and eventually as part of a larger revamped net subsystem API.
  2. Integrate this into the fs/promises FileHandle API.
  3. Integrate this into the Blob API
  4. Implement adapters and magic for existing node.js streams APIs

Once compiled, the module is available via require('stream/web'). This does not expose the globals, and I don't intend to expose the globals until after the implementation is ready to graduate from experimental.

For an example of how the QUIC API intends to use this... here's a snippet:

session.onstream = async ({stream, respondWith}) => {
  console.log(await stream.readable.getReader().read());  // reads a single chunk

  respondWith({ body: getReadableStreamFromSomewhere() });
};

TODOS:

  • Necessary to land initial implementation
    • Finish implementation
      • ReadableStream
      • WritableStream
      • TransformStream
      • Async iteration of ReadableStream
      • pipeTo and pipeThrough
      • postMessage() transfer of ReadableStream, WritableStream, and TransformStream
      • Validate passing WPT tests
    • Add appropriate WPT tests
    • Finish initial documentation
    • Split into multiple files
    • Move into lib/internal/webstreams
  • Necessary for coming out of experimental
    • Revisit error codes strategy
    • Performance profiling and optimization
    • Potentially split into separate files
    • Potentially refactor to simplify code
    • Implement Node.js streams adapters
      • stream.Readable <-> ReadableStream Adapters
      • stream.Writable <-> WritableStream Adapters
      • stream.Transform <-> TransformStream Adapters
      • In pipeTo / pipeThrough, if the destination and source are adapters of stream.Readable and stream.Writable, fallback to Node.js pipeline() instead if possible. If source and destination are both backed by native StreamBase, fallback to native pipeline.
      • stream.pipeline integration if possible.
      • Implement FileHandle.prototype.readable to get a ReadableStream for the FileHandle
  • Additional
    • Implement Blob.prototype.readable to get a ReadableStream for the Blob
    • Implement QUIC Stream.prototype.readable
@jasnell jasnell removed the needs-ci label Jun 17, 2021
@jasnell jasnell force-pushed the whatwg-streams branch 2 times, most recently from f58982f to 7829392 Jun 17, 2021
@XadillaX
Copy link
Member

@XadillaX XadillaX commented Jun 18, 2021

A deadly simple way is just using Deno's. (just joking

@legendecas
Copy link
Member

@legendecas legendecas commented Jun 18, 2021

A deadly simple way is just using Deno's.

IIUC, like said in the OP, it is planned to be "working with existing require('stream') API through low level adapters and magic". I don't believe it's a good idea to "adopt" Deno's implementation as it is not designed to be work with Node.js stream API in the first place.

lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/internal/streams/whatwg.js Outdated Show resolved Hide resolved
lib/stream.js Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the whatwg-streams branch 2 times, most recently from 5bfaf4e to 3d4c78a Jun 18, 2021
@ljharb
Copy link
Member

@ljharb ljharb commented Jun 18, 2021

It seems kind of odd to have the specifier include a standards body’s name.

@jasnell jasnell force-pushed the whatwg-streams branch from 3d4c78a to ba0b908 Jun 18, 2021
@jasnell
Copy link
Member Author

@jasnell jasnell commented Jun 18, 2021

It seems kind of odd to have the specifier include a standards body’s name.

That's something we can worry about later once the basic implementation is done.. but... Alternative suggestions?

@ljharb
Copy link
Member

@ljharb ljharb commented Jun 18, 2021

I agree the bikeshed can wait :-) just pointing it out.

As for suggestions: what are the long-term plans in node here? Are "node streams" going to eventually be deprecated/replaced by "whatwg streams"? Or will node likely have two streams implementations for awhile? What's notable about "whatwg streams" besides that they're what's in browsers? Would "web streams" make more sense?

@jasnell jasnell force-pushed the whatwg-streams branch from ba0b908 to 5e3052c Jun 18, 2021
@jasnell
Copy link
Member Author

@jasnell jasnell commented Jun 18, 2021

Existing streams aren't going anywhere. At least not for the foreseeable future. I have no intent here to impact existing streams at all.

Perhaps ... require('streams/standard')?

@jasnell jasnell force-pushed the whatwg-streams branch from 5e3052c to 5fce796 Jun 19, 2021
@ljharb
Copy link
Member

@ljharb ljharb commented Jun 19, 2021

Not stoked on implying that web specs are somehow “the” standard, but it’s probably better than “whatwg”, if “web” isn’t viable :-)

@jasnell jasnell force-pushed the whatwg-streams branch 5 times, most recently from c0d112b to ee39843 Jun 19, 2021
@ronag
Copy link
Member

@ronag ronag commented Jun 19, 2021

I’d very much like to have a discussion about our overall strategy and plan regarding streams in node before landing this. I’m not against it just want to make sure we have a reasonable plan we can communicate.

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jun 19, 2021

I’d very much like to have a discussion about our overall strategy and plan regarding streams in node before landing this

Ok, but I would ask that that discussion not happen in this PR. Can I ask you to open either an issue or discussion topic seeded with your thoughts?

@jasnell jasnell force-pushed the whatwg-streams branch from ee39843 to 65cd482 Jun 19, 2021
@ronag ronag mentioned this pull request Jun 19, 2021
@jasnell jasnell force-pushed the whatwg-streams branch from 65cd482 to 324783c Jun 19, 2021
@ronag
Copy link
Member

@ronag ronag commented Jun 19, 2021

Any preliminary performance figures relative to node streams?

Is stream.pipeline support in scope for this PR or is that future work?

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jun 19, 2021

Any preliminary performance figures ...

No. Not yet. I won't be working on performance optimizations until I'm sure the implementation is correct per the spec.

Is stream.pipeline support in scope for this PR ...

No, the adapters to existing streams API will come in separate PRs

jasnell added 2 commits Jun 28, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>
Experimental implementation of the WHATWG streams standard.

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell force-pushed the whatwg-streams branch from 9b66e95 to 8ae35d2 Jun 28, 2021
@jasnell
Copy link
Member Author

@jasnell jasnell commented Jun 28, 2021

Squashed the commits, rebased from the main, and CI passing. This is ready for reviews and landing. While it has the minimum number of sign offs necessary to land, I'll give it until Wednesday before landing.

targos
targos approved these changes Jun 30, 2021
Copy link
Member

@targos targos left a comment

Rubberstamp LGTM as experimental.

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jun 30, 2021

Landed in a9f9d2b...fa0c688

@jasnell jasnell closed this Jun 30, 2021
jasnell added a commit that referenced this issue Jun 30, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39062
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
jasnell added a commit that referenced this issue Jun 30, 2021
Experimental implementation of the WHATWG streams standard.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39062
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jimmywarting jimmywarting mentioned this pull request Jun 30, 2021
35 tasks
@jimmywarting
Copy link

@jimmywarting jimmywarting commented Jun 30, 2021

  • Implement FileHandle.prototype.readable to get a ReadableStream for the FileHandle

I was like: Wait what? Dose node have something called a FileHandle? must be something i have missed. But turns out that was not the same as FileSystemFileHandle provided by Native file system access (which i totally think we should implement btw, Deno have considered this too)

I don't know how i feel about FileHandle.prototype.readable i think it should perhaps mimic more how the file system access works (you get a FileHandle, call FileHandle.prototype.getFile() and receive a Blob/File backed up by the filesystem (#37340)

const file = await fileHandle.getFile()
const stream = file.stream()
console.assert(file instancesof Blob) // true
console.assert(stream instancesof ReadableStream) // true

  • Implement Blob.prototype.readable to get a ReadableStream for the Blob

I hope you meant Blob.prototype.stream()?

@espoal espoal mentioned this pull request Jul 1, 2021
targos added a commit that referenced this issue Jul 11, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39062
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit that referenced this issue Jul 11, 2021
Experimental implementation of the WHATWG streams standard.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39062
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos added a commit that referenced this issue Jul 13, 2021
Notable changes:

deps:
  * upgrade npm to 7.19.1 (npm-robot) #39225
fs:
  * (SEMVER-MINOR) allow empty string for temp directory prefix (Voltrex) #39028
stream:
  * (SEMVER-MINOR) implement Web Streams API (James M Snell) #39062

PR-URL: #39373
targos added a commit that referenced this issue Jul 13, 2021
Notable changes:

deps:
  * upgrade npm to 7.19.1 (npm-robot) #39225
fs:
  * (SEMVER-MINOR) allow empty string for temp directory prefix (Voltrex) #39028
stream:
  * (SEMVER-MINOR) implement Web Streams API (James M Snell) #39062

PR-URL: #39373
targos added a commit that referenced this issue Jul 14, 2021
Notable changes:

deps:
  * upgrade npm to 7.19.1 (npm team) #39225
fs:
  * (SEMVER-MINOR) allow empty string for temp directory prefix (Voltrex) #39028
stream:
  * (SEMVER-MINOR) implement Web Streams API (James M Snell) #39062

PR-URL: #39373
targos added a commit that referenced this issue Jul 14, 2021
Notable changes:

deps:
  * upgrade npm to 7.19.1 (npm team) #39225
fs:
  * (SEMVER-MINOR) allow empty string for temp directory prefix (Voltrex) #39028
stream:
  * (SEMVER-MINOR) implement Web Streams API (James M Snell) #39062

PR-URL: #39373
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jul 16, 2021
…estonly

Automatic update from web-platform-tests
Streams: read with fixed endianness

In some tests for readable byte streams, we pass a Uint16Array to
reader.read(view), then write into it as a Uint8Array and finally read
the results back as a Uint16Array. However, Uint16Array uses the
platform byte order, whereas these tests assume that it's always in
little-endian order.

Node.js has also started implementing the Streams API
(nodejs/node#39062), and they also run on big-endian platforms. To
support this, the tests must be independent of the platform byte
order. Use a DataView to achieve this.
--

wpt-commits: afcacf21caaf9d0efd6601833077e04af9b4dee1
wpt-pr: 29488
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet