Editorial: remove forAuthorCode and use dictionaries for read() return value

[domenic]

This is a start at tackling #1036. The intention is to have no normative behavior changes. However, note that I have not introduced artificial microtasks, so there are probably be some normative timing changes. If so then we should probably fix that, unless for some reason we think this would be an improvement.

Right now I've just done the reference implementation to give people an idea of what it looks like. If we can get all the tests passing and the shape of the solution hammered out, then I can port it to the spec text.

There are two kinds of failing tests currently:

• readable-streams/tee.any.html: "ReadableStream teeing: errors in the source should propagate to both branches" fails.

• readable-byte-streams/general.any.html craps out due to a timeout in "ReadableStream with byte source: A stream must be errored if close()-d before fulfilling read(view) with Uint16Array", and then the harness doesn't continue with the rest of the tests, so there are unknown failures there.

Both of these could be due to something stupid I missed, or due to the timing changes. I'll investigate tomorrow.

In the meantime, I'd appreciate thoughts on how this looks to folks, and whether it's a promising direction. For example, does the (chunkSteps, doneSteps, errorSteps) tuple seem like the right way to go? For byte streams especially, I had to thread them through more places than I'd hoped; is that OK, or does it make this less worth pursuing?

---

Preview | Diff

[ricea]

This is better than I expected. This pretty closely reflects what we'll need to do to optimise pipeTo() anyway, so having the timing match up with the standard is useful.

I need to think a bit more about the (chunkSteps, doneSteps, errorSteps) tuple.

I think this is worth pursuing, and I'm in favour of it provided we can make it work.

[domenic]

This now passes all tests. I inserted a manual microtask for teeing, which I think is reasonable. The tests were not able to detect any other instances of timing changes, and I believe that they're exhaustive. The potentially affected instances of ReadableStreamDefaultReaderRead are:

• Teeing, where the timing change matters (as seen here)
• Piping, where the timing change does not matter, since everything observable is intentionally hidden
• Public APIs, where the result gets wrapped into a promise so there are no timing changes

(ReadableStreamBYOBReaderRead only has public API callers, although in theory piping could use it too.)

I'm happy to port this over to the spec text, once folks have had a chance to make any editorial suggestions.

[MattiasBuelens]

Looks good to me! 👍

I especially like the separation between chunkSteps and doneSteps. I find the new version of asyncIteratorNext to be much more understandable than the old one.

Perhaps we could use a "struct" or a "tuple" in the spec text to represent all three steps, so we can thread them as a single argument through the various operations? Or would that make the reference implementation too different from the spec text?

[domenic]

A struct or tuple seems very doable, although it's a tradeoff. It makes passing them through algorithms easier, but requires a bit more work to call them (e.g., it'd be something like "Perform readRequest.[[Steps]]'s done steps" instead of "Perofrm readRequest.[[doneSteps]]").

[MattiasBuelens]

This might be a dumb question, but wouldn't [[readRequests]] and [[readIntoRequests]] just become a list of tuples? I don't understand why the steps would need to be nested inside an internal slot on a read request, instead of just being the read request. Something like "Perform readRequest's done steps" should be sufficient, no?

[domenic]

Oh, sure, that's true. Then we'd need to manually copy the three items over from the (chunkSteps, doneSteps, errorSteps) tuple into the read request tuple, which would slightly decrease the benefit of keeping them together, but we're already doing that anyway with them as separate parameters. So I think it would indeed be a strict improvement.

[ricea]

Hmmm... having read results available synchronously is a potential footgun. If code is written naively it can run into reentrancy issues. But since AFAIK Fetch is the only spec that actually reads from a stream, the potential damage is limited.

lgtm, please write spec text when you are ready.

[domenic]

Alright, this is good to go. I should probably write a Fetch patch too and hold off on merging this until that is ready.

(In general I want to work on making streams better for other specs to interface with, so that'll potentially be a bit of wasted work, but we'll see.)

[ricea]

lgtm. Very nice.