Make chunk param of ReadableStreamDefaultController.enqueue non-optional

[ikeyan]

Fixes #1682

[github-actions]

Thanks for the PR!

This section of the codebase is owned by Kagami Sascha Rosylight (@saschanaz) - if they write a comment saying "LGTM" then it will be merged.

[ikeyan]

@microsoft-github-policy-service agree

[Kagami Sascha Rosylight (saschanaz)]

Mattias Buelens (@MattiasBuelens) thoughts? Why is enqueue() accepting optional parameter at all? Who's calling it without any argument?

[Mattias Buelens (MattiasBuelens)]

Hmm, this wasn't optional when I made #541. This looks like a regression from f817b6e, see diff. Not sure how that happened?

Anyway, chunk should definitely be a required argument. 😉

[Kagami Sascha Rosylight (saschanaz)]

But it's optional in the spec, why?

[Kagami Sascha Rosylight (saschanaz)]

For now let's merge this. whatever the spec intended to do probably is for a very edge case.

LGTM

[github-actions]

Merging because Kagami Sascha Rosylight (@saschanaz) is a code-owner of all the changes - thanks!

[Kagami Sascha Rosylight (saschanaz)]

(And thank you all!)

[Mattias Buelens (MattiasBuelens)]

But it's optional in the spec, why?

I think the spec wants you to be able to write controller.enqueue(undefined) as just controller.enqueue(). Without the optional, that second call would throw a "not enough arguments" TypeError.

From a type checking perspective, this only make senses if the R in ReadableStreamDefaultController<R> is allowed to be undefined (i.e. it is of the form T | undefined). I don't think there's a way to cleanly express that in TypeScript right now, unless maybe with some weird conditional types?

I think it's still better to deviate from the Web IDL in this case and ignore the optional. The R generic type parameter is unlikely to allow undefined in real-world code, so it's better to catch more type errors by making it required. 🙂

[Kagami Sascha Rosylight (saschanaz)]

But any should accept undefined in Web IDL, no?

[Kagami Sascha Rosylight (saschanaz)]

Oh, maybe it predates undefined being a separate type.

[Mattias Buelens (MattiasBuelens)]

But any should accept undefined in Web IDL, no?

Yes, any includes undefined. (I don't think any or undefined or any? is allowed in Web IDL.)

There's no difference in what is accepted by any and optional any, it only matters whether you have to explicitly pass that parameter or whether you can leave it out.

I had a look at other APIs that accept a single any parameter. I found structuredClone, which is defined in Web IDL as:

interface mixin WindowOrWorkerGlobalScope {
  any structuredClone(any value, optional StructuredSerializeOptions options = {});
}

Note that undefined is a valid value:

structuredClone(undefined)
// -> undefined

But since value is not optional, this doesn't work:

structuredClone()
// -> TypeError: Failed to execute 'structuredClone' on 'Window': 1 argument required, but only 0 present.

Now I'm not so sure if controller.enqueue() should be allowed... 🤔

[ikeyan]

Mattias Buelens (@MattiasBuelens)
We can let it "enqueue with no argument" by allowing void in R (e.g. R = T | void). In that case, enqueue() is just "enqueue a void chunk":

new ReadableStream<Uint8Array | void>({
  start(controller) {
    controller.enqueue();
    controller.enqueue(new Uint8Array([1, 2, 3]));
    controller.close();
  },
});

This is the same pattern commonly used with Promise<void>, where resolve() can be called without an argument.

For the typical R that doesn’t include void/undefined, requiring chunk is the safer typing.