Skip to content

Conversation

@noamr
Copy link
Contributor

@noamr noamr commented Dec 12, 2022

This makes it easier for non-JS consumers of streams, like the Fetch specs, to use streams without a JS
execution context.

See whatwg/fetch#1568

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno: …
    • Node.js: …
  • MDN issue is filed: …

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

This makes it easier for non-JS consumers of streams,
like the Fetch specs, to use streams without a JS
execution context.

See whatwg/fetch#1568
@noamr noamr force-pushed the consume-no-promise branch from a4a65b9 to f3ca53f Compare December 12, 2022 08:34
@noamr noamr requested a review from MattiasBuelens December 12, 2022 08:44
Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm but please wait for other editors to look at it.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This works surprisingly well! I think we should just be a bit more aggressive.

index.bs Outdated
1. Return |promise|.
1. Let |success steps| given a [=byte sequence=] |data| be to [=resolve=] |promise| with |data|.
1. Let |failure steps| given a JavaScript value |error| be to [=reject=] |promise| with |error|.
1. [=Consume all bytes=] from |reader| given |success steps| and |failure steps|.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to get rid of "read all bytes as a promise". Nobody should use it, for various reasons (e.g. having to deal with then tampering). https://dontcallmedom.github.io/webdex/r.html#read%20all%20bytes%40%40ReadableStreamDefaultReader%40dfn implies it wouldn't be so bad to break some consumers; I think we could fix Fetch ourselves, file issues on the rest, then move on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, once this is reviewed I'll post a Fetch patch and issues to the other specs.

@ricea
Copy link
Collaborator

ricea commented Dec 13, 2022

No problem for Chrome, we don't do "full reading" at the JavaScript layer anyway. I think you can call us "interested" because it clears up any possible difference between our implementation and the spec.

@noamr
Copy link
Contributor Author

noamr commented Dec 13, 2022

No problem for Chrome, we don't do "full reading" at the JavaScript layer anyway. I think you can call us "interested" because it clears up any possible difference between our implementation and the spec.

I think this is purely editorial anyway..

noamr and others added 5 commits December 13, 2022 09:21
Co-authored-by: Domenic Denicola <[email protected]>
Co-authored-by: Domenic Denicola <[email protected]>
Co-authored-by: Domenic Denicola <[email protected]>
@domenic domenic merged commit f894acd into whatwg:main Dec 13, 2022
noamr added a commit to noamr/fetch that referenced this pull request Dec 13, 2022
noamr added a commit to noamr/fetch that referenced this pull request Dec 21, 2022
annevk added a commit to whatwg/fetch that referenced this pull request Dec 22, 2022
They are not designed for that and don't work well (e.g., resolve expects a JS value).

Follows whatwg/streams#1250 and addresses the Fetch part of whatwg/streams#1178.

Closes #1568.

Co-authored-by: Domenic Denicola <[email protected]>
Co-authored-by: Anne van Kesteren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants