Add Streams

[MattiasBuelens]

This PR updates the type definitions for streams to the latest version of the spec (snapshot). This also introduces generic type parameters for the chunk argument, which enables type-safe reading, writing, transforming and piping of streams.

This is largely based on @types/whatwg-streams. Important differences:

• Default values for the type parameters are set to any (instead of ArrayBufferView). This ensures that developers can still use ReadableStream (and others) as before and read chunks as any. In places where we know the precise chunk type (e.g. BodyInit and Body.body), I've added an explicit type parameter.
• Callback interfaces are used instead of callback types. This is mostly a side effect from the WebIDL processing of callback definitions.

Since the streams spec does not provide an official WebIDL (see whatwg/streams#45), the input WebIDL file was hand-written just for type generation purposes. The generated types are then further adjusted to add generics.

Fixes #521
Fixes microsoft/TypeScript#25277

[MattiasBuelens]

Mainly looking for some feedback.

Is there a nicer way to introduce a type parameter on a callback-function? Right now, to generate this:

interface ReadableStreamDefaultControllerCallback<R> {
    (controller: ReadableStreamDefaultController<R>): any;
}

I have to do add this to overridingTypes.json:

"callback-functions": {
    "callback-function": {
        "ReadableStreamDefaultControllerCallback": {
            "name": "ReadableStreamDefaultControllerCallback<R>",
            "override-signatures": [
                "(controller: ReadableStreamDefaultController<R>): any"
            ]
        }
    }
}

but then I also have to add a line in knownTypes.json:

{
    "Window": [
        "ReadableStreamDefaultControllerCallback<R>"
    ]
}

I tried using "type-parameters" on the "callback-function", but that doesn't seem to work.

[mhegazy]

@saschanaz can you please review this change.

[saschanaz]

I tried using "type-parameters" on the "callback-function", but that doesn't seem to work.

You may fix emitCallBackFunction in emitter.ts to make it work. You'll want to take a look at processInterfaceType when you do.

[saschanaz]

name and override-exposed are redundant here. Yes, names in e.g. Worker are also redundant, but no need to fix them in this PR.

[saschanaz]

And here.

[saschanaz]

Here too.

[saschanaz]

Also here, because the IDL has [Exposed=(Window,Worker)].

[saschanaz]

Can you move this above Storage? Also, npm run fetch will fail because the spec page does not have any IDL. We may add "local": true and fix fetcher.ts so that it won't fetch IDL when marked as local.

[MattiasBuelens]

I believe you mean below storage, since "Storage" < "Streams"? Sure, I can move "Streams" into alphabetical order.

[MattiasBuelens]

Right now, TypeParameter only supports name and default. Therefore, I simply patched in the type parameters into directly the interface's name. Ideally, this would go into the "type-parameters" property of the interface.

I tried to fix this by adding an extends property to TypeParameter and modifying the emitter, but that immediately caused the emitter to throw a "Maximum call stack size exceeded" error in getParentsWithEventHandler and getParentEventHandler. I'll look into this again later, to see why this would cause infinite recursion.

[MattiasBuelens]

Found it! The overriding interface still needs a name property for this to work, which I had removed while testing. Putting the name back fixes it.

[saschanaz]

1. A dictionary should be in dictionary part of overridingTypes.json.
2. A dictionary doesn't need override-exposed.
3. A dictionary shouldn't use "no-interface-object": "1". (It's for interfaces marked as [NoInterfaceObject])

[saschanaz]

Fixing this may introduce other problems as currently some interfaces incorrectly extends from dictionaries. This is not a strict blocker, anyway.

[MattiasBuelens]

This is probably a leftover from my initial hack to make <R extends ReadableStream> work. I'll look into it.

[MattiasBuelens]

A more precise type for this would be:

type ReadableStreamReadResult<T> = {
    done: false;
    value: T;
} | {
    done: true;
    value: undefined;
};

However, I also want this to be compatible with IteratorResult<T>, which is still defined as a regular interface:

interface IteratorResult<T> {
    done: boolean;
    value: T;
}

So yeah... ¯\_(ツ)_/¯

[zenmumbler]

In addition, the IteratorResult type is only defined in es2015.lib as I found out the hard way.

[MattiasBuelens]

All right, I think I got all of the types sorted out now. I tried using the new types against the tests for @types/whatwg-streams, and they seem to work just fine. 😄

[MattiasBuelens]

Ah dangit, I missed one.

[MattiasBuelens]

@saschanaz If you find some time for another review, that'd be much appreciated! 😁 (If not, that's okay as well, I can wait.)

[saschanaz]

Looks okay for me, given that we have minimal changes in emitter.ts.

Not sure this is maintainable, as updating things should be completely manual.

[MattiasBuelens]

Not sure this is maintainable, as updating things should be completely manual.

Understandable. It'd be nicer if the streams spec had its own WebIDL definition, but there's still the whole "JSIDL vs WebIDL" discussion going on there. Even better would be if Microsoft Edge updated its implementation to the latest version of the spec so we could use its generated types, but it doesn't seem like that's going to happen any time soon.

I can help keep these types up-to-date. Feel free to @ me if you see any streams-related issues popping up. 😉

[MattiasBuelens]

Any idea when this might land? I can wait, but it'd be nice to know when to expect this in a future TypeScript release.

I noticed that the last time a PR got merged was on August 2, and all newer PRs are still open. Are the maintainers taking a (well deserved) break, or are you working on something else?

[saschanaz]

@mhegazy is (probably) taking a break 😁

[HolgerJeromin]

His twitter status says:

Formally engineering manager for
typescriptlang

[zenmumbler]

Oh man, why did I not check this before I started my own version of adding all the types to TSJS, I only checked another issue in the main TS repo Aargh!… Anyway…, since the maintainer is MIA and this is still open for edits, can I make some? Mostly for @MattiasBuelens

• Can you change the order of the R and W parameters for Transformer and TransformStream? Reason is that usage of the type then reads as TransformStream<Input, Output>. The signature of the TransformStream constructor is also setup this way with the writable strategy first.
• Ah, I see you changed the type from the deftyped version that I don't have to do something likeTransformStream<WritableStream<string>, ReadableStream<Uint8Array>> but can just do the natural TransformStream<string, Uint8Array> Again, if the order of these params would be from, to instead of to, from

Great work, I will also gladly help out with updating these if necessary.

[zenmumbler]

BTW, this will also close microsoft/TypeScript#17247

[MattiasBuelens]

@zenmumbler Ah dangit, I didn't see that issue, otherwise I would have definitely put a link to this PR. Hope you didn't spend too much time on your own PR. 😅

Anyway, I definitely see the benefit of swapping the type arguments on TransformStream. I think TransformStream<I, O> would be most appropriate. Originally I wanted to keep this PR somewhat close to the DefinitelyTyped type definitions, but it's probably more important to "get it right" the first time for the TypeScript DOM types.

[MattiasBuelens]

Rebased on master, and swapped the order of the type parameters.

@zenmumbler Could you have a look? 😃

[zenmumbler]

Hi @MattiasBuelens, something I found quickly, will do another round of checks in a bit:

The following types all need "no-interface-object": 1 added so that they don't define a global constructor:

ReadableByteStreamController
ReadableStreamBYOBReader
ReadableStreamBYOBRequest
ReadableStreamDefaultReader
TransformStreamDefaultController
WritableStreamDefaultWriter

This is because these objects, according to the spec, cannot be directly constructed and have hidden/private constructor types. The spec even adds checks in the constructors themselves to throw if invoked from outside the Streams internals if the user acquired a constructor function through sneaky means.

[zenmumbler]

Regarding several locations where | null is specified on an optional field in a dictionary: except for the body field in the Body/Response type and the various desiredSize fields, where the spec explicitly sets these fields to null in specific situations, these are all, if not incorrect, at least confusing.

E.g. the preventClose, etc fields in QueuingStrategy These are optional fields that will be interpreted as booleans. While the spec does a reinterpret-as-boolean operation on the field, it does not specifically mention null to have special meaning. In that case since it's either truthy or falsy, the type could as well be any.

I would remove these | null annotations as their explicit addition implies some kind of meaning, and that isn't there. Again, keep the ones for desiredSize as that gets explicitly set to null by the spec.

It concerns all fields in the following interfaces that have an | null type union.

PipeOptions
QueuingStrategy
Transformer
UnderlyingByteSource
UnderlyingSink
UnderlyingSource

[zenmumbler]

OK, I compared all the resulting types with what I had manually created for my streams implementation and with the Streams Spec on hand I found the following final points:

• The type of UnderlyingSink.close is incorrect. close takes no parameters and returns void | Promise<void>. In my version I renamed the writer callback and added a close callback (master...stardazed:addStreamsTypes#diff-33c56fff5ba0262bfabf1ca82cfab269R2792)
• The type of ReadableStreamBYOBRequest.view is too narrow. There is no guarantee in the spec that it is a Uint8Array, it should be an ArrayBufferView
• The return types of ReadableByteStreamControllerCallback, ReadableStreamDefaultControllerCallback, TransformStreamDefaultControllerCallback, TransformStreamDefaultControllerTransformCallback , WritableStreamDefaultControllerCallback and WritableStreamDefaultControllerWriteCallback are too broad, they are now any whereas the streams spec expects void | Promise<void>, which is also consistent with other function definitions elsewhere in your changes and communicates that you can optionally return a Promise.
• The return type of ReadableStreamErrorCallback and WritableStreamErrorCallback are too broad, it states any but should be void. I know that some specs define any on callbacks where the return value is ignored, I prefer void as it communicates intent.
• Nice catch on the different signature for the bytes version of the ReadableStream ctor!
• Given that you haven't added the readableType and writableType future fields to Transformer, it may be best to remove the placeholder type field on UnderlyingSink, or to add these to Transformer as well.
• Q: what was the motivation to define WritableReadableStreamPair as

interface WritableReadableStreamPair<W extends WritableStream, R extends ReadableStream> {
    readable: R;
    writable: W;
}

instead of

interface WritableReadableStreamPair<I, O> {
    writable: WritableStream<I>;
    readable: ReadableStream<O>;
}

?
To add to that, when the encoding standard added the text decoding built-in streams, they defined this interface as GenericTransformStream, which is also what I adopted, probably best to use that name as it is used in a production standard. That would also necessitate the 2nd form of the interface, parameterized by chunk types instead of stream interfaces.
See encoding spec bit here: https://encoding.spec.whatwg.org/#generictransformstream

That would also make the pipeThrough signature of ReadableStream more consistent, from currently:
pipeThrough<T extends ReadableStream>(pair: WritableReadableStreamPair<WritableStream<R>, T>, options?: PipeOptions): T;
to
pipeThrough<W>(transform: GenericTransformStream<R, W>, options?: PipeOptions): WritableStream<W>;
Note how the type params are <R, W> here as it defines the inverse passthrough streams. I had to start using type names InputType and OutputType in my code to help my brain, it gets confusing fast.

OK, unless I missed something that is all there is. Again, thanks for the great work and if you have no time to work on these changes, I can do that as well. Cheers.

[sandersn]

Thanks @MattiasBuelens @zenmumbler and @saschanaz for all your work on this one. I have taken over this repo since @mhegazy has moved on, so I'll merge it when you all judge that it's done.

[zenmumbler]

@MattiasBuelens Do you have time to look into this this week, I'm not actually sure how I would help out if you don't have time, would I clone your branch? Anyway, 3.2 is slated for November and I'd really like to get this in for that release to avoid having to do more workarounds for my streams code. Thanks!

[MattiasBuelens]

@zenmumbler Thanks for your extensive review! I'll go over it today. 😃

[MattiasBuelens]

Sorry it took so long to respond. Anyway, here we go:

The following types all need "no-interface-object": 1 added so that they don't define a global constructor:

Good catch!

The reader and writer classes are a bit weird though. Although they aren't globally accessible, these classes do have a non-throwing constructor. There's even a test for this:

const ReadableStreamDefaultReader = (new ReadableStream()).getReader().constructor;
const reader = new ReadableStreamDefaultReader(new ReadableStream());

We'd need to write very tricky types just to give this constructor a correct signature without also defining a global variable. And since these constructors shouldn't really be used, I agree that it's better to simply remove them entirely.

Regarding several locations where | null is specified on an optional field in a dictionary

This seems to be an artifact of how WebIDL dictionaries are converted. Apparently, they're marked both nullable: 1 and required: 0. I'll just add nullable: 0 to every optional property.

The type of UnderlyingSink.close is incorrect. close takes no parameters and returns void | Promise<void>

Good catch, there's indeed no controller argument.

As for the return type, see below.

The type of ReadableStreamBYOBRequest.view is too narrow. There is no guarantee in the spec that it is a Uint8Array, it should be an ArrayBufferView

I guess I tried to be too clever for this one. Indeed, SetUpReadableStreamBYOBRequest requires view to be an ArrayBufferView. However, I noticed that ReadableByteStreamController.byobRequest always creates a Uint8Array when calling SetUpReadableStreamBYOBRequest, so in practice you won't see a different typed array. That's why I changed the type to Uint8Array in my PR for DefinitelyTyped.

However, the streams spec seems to leave room for a future update to create a BYOB request with a different typed array. Therefore, I agree that it's probably safer to change it back to ArrayBufferView.

The return types of ReadableByteStreamControllerCallback, ReadableStreamDefaultControllerCallback, TransformStreamDefaultControllerCallback, TransformStreamDefaultControllerTransformCallback , WritableStreamDefaultControllerCallback and WritableStreamDefaultControllerWriteCallback are too broad

There was some discussion about this in whatwg/streams#45, see comment. Basically, the streams spec allows you to return any, since it first passes the return value through Promise.resolve. That's why these return types were initially changed to any.

However, I agree that we should change this to void | PromiseLike<void> in order to catch more type errors. This seems to be common practice with similar types as well, e.g. EventListener also has a void return type. The handbook also recommends avoiding any on callback return types.

The return type of ReadableStreamErrorCallback and WritableStreamErrorCallback are too broad, it states any but should be void.

Agreed, we should use void or any. However, these error callbacks are also allowed to return a Promise, see the spec on UnderlyingSource.cancel and UnderlyingSink.abort. I'll change them to void | PromiseLike<void>, similar to the other calbacks.

Nice catch on the different signature for the bytes version of the ReadableStream ctor!

Thanks! 😄

Given that you haven't added the readableType and writableType future fields to Transformer, it may be best to remove the placeholder type field on UnderlyingSink, or to add these to Transformer as well.

I don't think I added an UnderlyingSink.type property just yet? I only added UnderlyingSource.type to ensure it's undefined.

Q: what was the motivation to define WritableReadableStreamPair as (...) instead of (...)?

This was taken from the DefinitelyTyped definitions. It allows ReadableStream.pipeThrough to have a more precise return type:

pipeThrough<T extends ReadableStream>(pair: WritableReadableStreamPair<WritableStream<R>, T>, options?: PipeOptions): T;

This has the advantage that if pair.readable is known to be a subclass of ReadableStream, TypeScript will use that subclass' type as the return value of pipeThrough.

However, I agree that this shouldn't complicate the other types. I think I'll just inline and remove WritableReadableStreamPair altogether.

To add to that, when the encoding standard added the text decoding built-in streams, they defined this interface as GenericTransformStream, which is also what I adopted, probably best to use that name as it is used in a production standard.

I feel like GenericTransformStream is merely a construct to avoid repetition in the Encoding standard. It doesn't seem like web developers should know about it, so I wouldn't use it in the type definitions. The GenericTransformStream definition also talks about "delegating" to a TransformStream, which isn't required for pipeThrough.

As mentioned before, I think it's cleaner to simply inline WritableReadableStreamPair everywhere.

(Also, you have a small typo in your proposed type definition: pipeThrough should return ReadableStream<W>, not WritableStream<W>. 😉)

[zenmumbler]

Agreed, we should use void or any. However, these error callbacks are also allowed to return a Promise, see the spec on UnderlyingSource.cancel and UnderlyingSink.abort. I'll change them to void | PromiseLike, similar to the other callbacks.

Oh wow, I actually had that wrong in my own types. Good catch again.

I don't think I added an UnderlyingSink.type property just yet? I only added UnderlyingSource.type to ensure it's undefined.

Sorry, probably made a typo there, my reason to comment was to indicate that the type, readableType and writableType fields are all sort of future field placeholders and in the current version only type is defined and not the other two. So I figured we should either do all or none of them. The case for none is that they have no utility currently and are only there almost informationally and that they need to be updated once the fields get accepted values, the case to include them all is to avoid current-spec runtime errors if included by people today. Could be a minor issue with polyfills (that we both maintain) if we add support ahead of browsers implementing them, but at that point we can just send a PR here as well I guess. The number of polyfills of the streams API is rather limited, 2? :)

(Also, you have a small typo in your proposed type definition: pipeThrough should return ReadableStream, not WritableStream. 😉)

Yes! I saw that and wanted to fix it but probably saw something shiny and forgot. I mean, I left that in to keep you on your toes.

[MattiasBuelens]

Sorry, probably made a typo there, my reason to comment was to indicate that the type, readableType and writableType fields are all sort of future field placeholders and in the current version only type is defined and not the other two.

Ah, I see. I think we can do the type?: undefined trick to enforce forward-compatibility.

[MattiasBuelens]

@zenmumbler Do you think this is good to go, or are there still some changes needed?

(Pro tip: go to the "Files changed" tab and click the fancy "Review changes" button to indicate whether you approve, or whether this needs more changes. Then it shows up nicely in this thread. 😄 )

[zenmumbler]

Just one final thing to discuss with the concept-crossing pipeThrough method. Sorry I didn't do this earlier, due to GH's layout I completely missed you had already made the changes with your earlier comment.

[zenmumbler]

Great, I think we've covered everything. Thanks again for all your efforts! cc: @sandersn

[zenmumbler]

Hi @sandersn, thanks for merging. This is the first time I've contributed to the lib generator and I was wondering how often these definitions are merged back in the main TS repo. Is it on the next track or only updated with beta/rc releases? Thank you.

[MattiasBuelens]

@zenmumbler From the history of src/lib/dom.generated.d.ts, the team seems to pull in updates around 'big' releases, so not for every nightly release. I would think they'll do an update somewhere during the preparation for a 3.2 release?

[zenmumbler]

Thanks @MattiasBuelens, I kind of need to use this today so I just resorted to copying over the contents of the TSJS generated file into my local TS installs… Not exactly scalable, but workable until 3.2(rc) comes out 😅. Only thing I can't do is publish updates to my packages until then but I have pnpm and verdaccio to take care of that.

[MattiasBuelens]

@zenmumbler microsoft/TypeScript#28343 has landed, so the new stream types are now available on typescript@next! 😄