-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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.
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
FF102 User define byte stream interface - not experimental #16818
FF102 User define byte stream interface - not experimental #16818
Conversation
@wbamberg If you're happy with this please approve but don't merge. |
Hey @domenic and @mathiasbynens: Hamish has been working up the MDN documentation for user defined byte streams, based partially on my explanations at the bottom of this bug. I would love it if either of you had the time, if you could take a peek as well for accuracy. |
Thanks @mgaudet . FYI @domenic and @mathiasbynens This PR is now worth looking at. The work continues ... |
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
@mgaudet Howdy, FYI, the first draft of the guide is now done in https://pr16818.content.dev.mdn.mozit.cloud/en-US/docs/Web/API/Streams_API/Using_readable_byte_streams The examples clarified a few things for me, as they always do, including that the auto buffer allocation is ignored if you have a byte reader. Now that I have the examples I plan to flesh out all the pages tomorrow. That means this should be ready for proper remove in about 24 hours. I'll ping the spec authors then too. |
I am on vacation this week, so won’t be able to take a look till the 11th. |
@domenic and @mathiasbynens . First draft complete. There might be tweaks but the shape of it is done. Matthew is hoping to review next week. If you have comments, in particular about https://pr16818.content.dev.mdn.mozit.cloud/en-US/docs/Web/API/Streams_API/Using_readable_byte_streams they would be much appreciated. EDIT: Added request for review in whatwg/streams#1237 |
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed about half of the files, will try to get through the rest later this week. Overall it's looking very good, nice work! 👏
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/readablebytestreamcontroller/desiredsize/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/readablebytestreamcontroller/enqueue/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/readablestreambyobrequest/respondwithnewview/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/streams_api/using_readable_byte_streams/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/readablestreambyobrequest/respondwithnewview/index.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Mattias Buelens <649348+MattiasBuelens@users.noreply.github.com>
Co-authored-by: Mattias Buelens <649348+MattiasBuelens@users.noreply.github.com>
Also update code review to use "instance" variants Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
84107e9
to
0b1de15
Compare
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Thanks @teoli2003 YOu raised a few questions about cases where a reject is not used. I can remove these if you like. I left them in because originally this was a generally purpose mock and I wasn't sure whether I'd add error fail cases etc. They aren't needed, but nor do they do much harm. So unless you say "must be commented or removed" then I lean to leaving them along - in particular since most are hidden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed an alternative, your call. I'm approving, and let you merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final nits, other than that this LGTM. Thank you so much for this excellent write-up! 😄
I did a cursory review, but I'm very happy that the spec authors found the time to review this, and I will defer to their expertise. LGTM as well :) |
Co-authored-by: Mattias Buelens <649348+MattiasBuelens@users.noreply.github.com> Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
Thanks @teoli2003 - I accepted all your nits and will merge. Thanks @MattiasBuelens for your reviews. This took longer for me to get my head around than necessary. FYI only - these will seem stupid to you:
At some point it might be worth me/someone coming back and adding a mock backpressure. IMO the ability to try stuff in live examples adds a lot of value. @mgaudet You've gone above and beyond. Really appreciate it. Note the comment from @MattiasBuelens that the spec does not require that if a byobRequest is provided that it is used - you can enqueue if you like and the controller should handle that. If FF does something different it might be off spec. I'll leave you guys to chat about that elsewhere. |
FF102 brings support for the following interfaces:
This work includes updates to the above classes (and their properties and methods), along with a new guide Using_readable_byte_streams.
There are lots of other minor changes to related docs to cross link the new docs and also minor fixes to things like TransformStreams and streaming API doc while trying to get my head around the above.
Other docs work can be tracked in #16816