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.

Sign up for GitHub

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

Jump to bottom

Updating added types for ReadableStreamReadDoneResult #1676

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Updating added types for ReadableStreamReadDoneResult #1676

wants to merge 4 commits into from

Conversation

kraenhansen
Copy link

@kraenhansen kraenhansen commented Jan 21, 2024 •
edited

This fixes #1675.

@github-actions GitHub Actions
Copy link
Contributor

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
Copy link
Contributor

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

@saschanaz
Copy link
Contributor

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

@saschanaz
Copy link
Contributor

saschanaz commented Jan 21, 2024 •
edited

Hi @MattiasBuelens, would you be satisfied with this?

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

@kraenhansen
Copy link
Author

kraenhansen commented Jan 21, 2024 •
edited

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
MattiasBuelens suggested changes Jan 22, 2024
View reviewed changes
inputfiles/addedTypes.jsonc Outdated
@@ -1303,7 +1320,8 @@
},
"value": {
"name": "value",
"overrideType": "T"
"overrideType": "T",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

inputfiles/addedTypes.jsonc
@@ -1303,7 +1320,8 @@
},
"value": {
"name": "value",
"overrideType": "T"
"overrideType": "T",
"required": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }:

  1. 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Author

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

I found the issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattiasBuelens MattiasBuelens MattiasBuelens requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ReadableStreamReadDoneResult shouldn't be generic and should have value: undefined
3 participants
@kraenhansen @saschanaz @MattiasBuelens