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.

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

Editorial: use dictionaries for { value, done } and eliminate "forAuthorCode"? #1036

Closed
domenic opened this issue May 19, 2020 · 3 comments
Closed
Assignees

Comments

@domenic
Copy link
Member

domenic commented May 19, 2020

After #1035 lands, I think it might be possible to go further.

In particular, I'm hoping to clean up the return type of read() to be a dictionary with value and done members.

But while looking into this, I'm noodling on ways to clean up the whole "forAuthorCode" business. Is there a way we can keep the internals of the stream agnostic to the JS object format, and only convert the chunks/end-of-stream signals into dictionaries (and then via Web IDL, JS objects) at the end?

The difficult part of this is that the internals of the streams seem bound up in the promise machinery, and promises are fundamentally resolved with JS values. Maybe we could resolve those promises with JS values for the chunks, and have a special JS value representing end-of-stream, instead of having to create prototype-pollution-prone { value, done } objects and then potentially null out their prototype?

I think it's also worth figuring out what the best interface is for other specifications. If we could avoid them having to deal with raw promises for { value, done } objects, that would be a plus.

@ricea
Copy link
Collaborator

ricea commented May 26, 2020

JS values for chunks would also be vulnerable to prototype pollution.

When I thought about this before, the only solution I came up with was to have something almost but not quite the same as a Promise to use internally.

@domenic
Copy link
Member Author

domenic commented May 26, 2020

Oh, you're totally right. Dang.

Streams is in a peculiar position where it is using promises for internal flow control (throughout the spec ecosystem) as well as externally exposed to web developers. Everything else in the ecosystem only uses promises at the developer boundary, and uses "async algorithms" of some sort internally. Those async algorithms are sometimes informal, and sometimes formal (using mechanisms like "in parallel", "queue a task", etc.). whatwg/infra#181 touches on this more.

So I think if we were to do anything here it would be best to make streams behave more like the rest of the ecosystem. In particular, I think

the only solution I came up with was to have something almost but not quite the same as a Promise to use internally

is right, but I wonder if we'd be able to do that surgically, in only the places that handle chunks. I.e., keep using promises for closed/ready/the return values of the underlying X methods/etc. But for ReadableStreamDefaultReaderRead / [[PullSteps]] / read(), do something a bit different, passing the raw chunk around (not in promise form), until finally wrapping it up right before returning from read().

If this is surgical enough, then perhaps we could just use "spec callbacks". Callbacks are gross and messy compared to promises, but if you only go through one or two algorithms using them, it might not be so bad.

@ricea
Copy link
Collaborator

ricea commented May 28, 2020

So, concretely, we'd change ReadableStreamDefaultReaderRead() and PullSteps() to take (resolve, reject) callbacks as arguments instead of returning a Promise. And [[readRequests]] would become a queue for (resolve, reject) pairs rather than promises.

Tricky part is: do we call the callbacks synchronously, and risk re-entrancy, or asynchronously, and have to post microtasks somehow.

domenic added a commit that referenced this issue Jun 11, 2020
Closes #963.

Normative changes to widely-implemented features, roughly in order of most disruptive to least-disruptive:

* For the queuing strategy classes, their size and highWaterMark properties are now getters on the prototype, instead of data properties on the prototype and instance (respectively). Closes #1005. In particular this means that attempts to set either of them post-creation will throw a TypeError. Chromium already ships these semantics.

* Functions which take a dictionary no longer accept non-objects.

* For the queuing strategy classes, their highWaterMark property will no longer return a non-number from their highWaterMark properties, if one was passed to the constructor. Instead, NaN will be returned.

* All methods and accessors are now enumerable, per Web IDL defaults, instead of non-enumerable, per ECMAScript defaults.

* All classes are now exposed globally. Formerly, ReadableStreamDefaultReader, ReadableStreamBYOBReader, ReadableStreamDefaultController, ReadableByteStreamController, WritableStreamDefaultWriter, WritableStreamDefaultController, and TransformStreamDefaultController were not exposed. Closes #586.

* All classes now have [Symbol.toStringTag] properties. Closes #952.

* Some functions have changed their length property value.

* Some exceptions are thrown earlier, at argument-conversion time.

* Property lookup in options arguments now happens earlier, at argument-conversion time, and in alphabetical order, per dictionary rules.

Normative changes to unimplemented features:

* ReadableStream's getIterator() method has been renamed to values() as part of adopting Web IDL's infrastructure for async iterators.

* The byobRequest property on ReadableByteStreamController now returns null when there is no BYOB request, instead of returning undefined.

* The view property on ReadableStreamBYOBRequest now returns null when the view cannot be written into, instead of returning undefined.

* Various byte-stream-related APIs that used to specifically prohibit detached buffers now check for zero-length views or buffers, which is a more general category.

* The async iterator's next() and return() methods now behave more like async generators, e.g. returning promises fulfilled with { value: undefined, done: true } after return()ing the iterator, instead of returning a rejected promise.

Editorial changes:

* All APIs are specified to using Web IDL now, instead of using a modified version of the ECMAScript specification conventions. We continue using abstract operations and completion records for now, and we have to drop down to the ECMAScript level in a couple places (notably for dealing with %ObjectPrototype% vs. null-prototype iteration result objects, and transferring array buffers). But overall this removes a lot of type-checking and conversion boilerplate from the specification. Closes #963. Closes #1017. See #1036 for further followup on the iteration result objects.

* Individual abstract operations, constructors, methods, and properties no longer have their own heading. They are instead lumped together in sections. Closes #885.

* The constructors, methods, and properties are now documented in a per-class block, using the usual WHATWG "domintro" style. Closes #907.

* Abstract operations are now consistently alphabetized within their section. Closes #684.

* By using Bikeshed's <div algorithm> feature, we now get automatic identifier highlighting. Closes #687.

* Switched to 100-character line limits, 1-space indents, and omitting end tags, per WHATWG conventions.

* Removed usage of emu-algify in favor of using some more of Bikeshed's built-in features, plus manually annotating a few things.

* Switched to concise Bikeshed linking syntax, e.g. [=term=] and [$AbstractOp$].

* Eliminated a number of utility abstract operations, especially around calling functions, by better using Web IDL.

Other bug fixes:

* Web IDL makes constructor behavior clear, so this closes #965.
@domenic domenic self-assigned this Jun 23, 2020
@domenic domenic closed this as completed in 63b4407 Jul 6, 2020
yutakahirano pushed a commit to yutakahirano/streams that referenced this issue Jun 3, 2021
Closes whatwg#963.

Normative changes to widely-implemented features, roughly in order of most disruptive to least-disruptive:

* For the queuing strategy classes, their size and highWaterMark properties are now getters on the prototype, instead of data properties on the prototype and instance (respectively). Closes whatwg#1005. In particular this means that attempts to set either of them post-creation will throw a TypeError. Chromium already ships these semantics.

* Functions which take a dictionary no longer accept non-objects.

* For the queuing strategy classes, their highWaterMark property will no longer return a non-number from their highWaterMark properties, if one was passed to the constructor. Instead, NaN will be returned.

* All methods and accessors are now enumerable, per Web IDL defaults, instead of non-enumerable, per ECMAScript defaults.

* All classes are now exposed globally. Formerly, ReadableStreamDefaultReader, ReadableStreamBYOBReader, ReadableStreamDefaultController, ReadableByteStreamController, WritableStreamDefaultWriter, WritableStreamDefaultController, and TransformStreamDefaultController were not exposed. Closes whatwg#586.

* All classes now have [Symbol.toStringTag] properties. Closes whatwg#952.

* Some functions have changed their length property value.

* Some exceptions are thrown earlier, at argument-conversion time.

* Property lookup in options arguments now happens earlier, at argument-conversion time, and in alphabetical order, per dictionary rules.

Normative changes to unimplemented features:

* ReadableStream's getIterator() method has been renamed to values() as part of adopting Web IDL's infrastructure for async iterators.

* The byobRequest property on ReadableByteStreamController now returns null when there is no BYOB request, instead of returning undefined.

* The view property on ReadableStreamBYOBRequest now returns null when the view cannot be written into, instead of returning undefined.

* Various byte-stream-related APIs that used to specifically prohibit detached buffers now check for zero-length views or buffers, which is a more general category.

* The async iterator's next() and return() methods now behave more like async generators, e.g. returning promises fulfilled with { value: undefined, done: true } after return()ing the iterator, instead of returning a rejected promise.

Editorial changes:

* All APIs are specified to using Web IDL now, instead of using a modified version of the ECMAScript specification conventions. We continue using abstract operations and completion records for now, and we have to drop down to the ECMAScript level in a couple places (notably for dealing with %ObjectPrototype% vs. null-prototype iteration result objects, and transferring array buffers). But overall this removes a lot of type-checking and conversion boilerplate from the specification. Closes whatwg#963. Closes whatwg#1017. See whatwg#1036 for further followup on the iteration result objects.

* Individual abstract operations, constructors, methods, and properties no longer have their own heading. They are instead lumped together in sections. Closes whatwg#885.

* The constructors, methods, and properties are now documented in a per-class block, using the usual WHATWG "domintro" style. Closes whatwg#907.

* Abstract operations are now consistently alphabetized within their section. Closes whatwg#684.

* By using Bikeshed's <div algorithm> feature, we now get automatic identifier highlighting. Closes whatwg#687.

* Switched to 100-character line limits, 1-space indents, and omitting end tags, per WHATWG conventions.

* Removed usage of emu-algify in favor of using some more of Bikeshed's built-in features, plus manually annotating a few things.

* Switched to concise Bikeshed linking syntax, e.g. [=term=] and [$AbstractOp$].

* Eliminated a number of utility abstract operations, especially around calling functions, by better using Web IDL.

Other bug fixes:

* Web IDL makes constructor behavior clear, so this closes whatwg#965.
yutakahirano pushed a commit to yutakahirano/streams that referenced this issue Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants