Updating added types for ReadableStreamReadDoneResult

[kraenhansen]

This fixes #1675.

[github-actions]

Thanks for the PR!

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

[saschanaz]

Makes sense, bu you need to build and commit the result.

[saschanaz]

Actually byte streams indeed return the value, btw. Perhaps split the type? https://streams.spec.whatwg.org/#byob-reader-internal-slots

[saschanaz]

Hi @MattiasBuelens, would you be satisfied with this?

Also it would be good to have some unit tests here.

[kraenhansen]

I've updated the code to separate the ReadableStreamReadResult into two:

• ReadableStreamDefaultReadResult
• ReadableStreamBYOBReadResult

I've also added ReadableStreamReadResult which union both, to keep breaking changes to a minimum.

The two new types reuse ReadableStreamReadValueResult but have their separate "DoneResult" types:

• ReadableStreamDefaultReadDoneResult
• ReadableStreamBYOBReadDoneResult

The PR is currently failing tests:

Test failed: could not compile 'dom.generated.d.ts':
generated/dom.generated.d.ts(28201,75): error TS2552: Cannot find name 'ReadableStreamBYOBReadDoneResult'. Did you mean 'ReadableStreamBYOBReadResult'?
generated/dom.generated.d.ts(28203,78): error TS2552: Cannot find name 'ReadableStreamDefaultReadDoneResult'. Did you mean 'ReadableStreamDefaultReadResult'?

I can't seem to get the specialised *DoneResult types into the dom.generated.d.ts 🤔

[MattiasBuelens]

ReadableStreamBYOBReadDoneResult.value should be T | undefined, as per spec:

If the reader is canceled, the promise will be fulfilled with an object of the form { value: undefined, done: true }. In this case, the backing memory region of view is discarded and not returned to the caller.

See also: Deno and web-streams-polyfill.

[MattiasBuelens]

If we make value: T | undefined, should we also make it optional? Or does that not work well together with exactOptionalPropertyTypes?

[kraenhansen]

I don't think so, the spec is pretty clear that the value property would exist: https://streams.spec.whatwg.org/#default-reader-read

[MattiasBuelens]

That's for default readers. For BYOB readers, the spec is at https://streams.spec.whatwg.org/#byob-reader-read.

But still, that doesn't explain where chunk is coming from. When a stream is cancelled while it has a BYOB reader attached, ReadableStreamCancel resolves all pending read(view) promises with { done: true, value: undefined }:

6. If reader is not undefined and reader implements ReadableStreamBYOBReader,
   6.3. For each readIntoRequest of readIntoRequests,
   6.3.1. Perform readIntoRequest’s close steps, given undefined.

[kraenhansen]

You're right. Reading the spec you've linked to, it seems pretty explicit (list item 9) that the value property will be in the object returned from read, even from a ReadableStreamBYOBReader.

[MattiasBuelens]

The PR is currently failing tests:

Test failed: could not compile 'dom.generated.d.ts':
generated/dom.generated.d.ts(28201,75): error TS2552: Cannot find name 'ReadableStreamBYOBReadDoneResult'. Did you mean 'ReadableStreamBYOBReadResult'?
generated/dom.generated.d.ts(28203,78): error TS2552: Cannot find name 'ReadableStreamDefaultReadDoneResult'. Did you mean 'ReadableStreamDefaultReadResult'?

I can't seem to get the specialised *DoneResult types into the dom.generated.d.ts 🤔

The Streams types have [Exposed=*] in their IDL, so I think you need to add "exposed": "*" (or something equivalent) to your addedTypes.jsonc changes?

[kraenhansen]

I can't seem to get the specialised *DoneResult types into the dom.generated.d.ts 🤔

I found the issue 👍