Editorial: add cross-links to internal slots

[MattiasBuelens]

This is a quick experiment to see what would be needed to have cross-links on internal slots. For now, I've only done ReadableStream, but I'm pretty confident we can extend this to other classes. I also have not yet taken the line length into account, so some lines may still need wrapping.

The change is mostly mechanical: e.g. find all usages of \[[disturbed]] and replace them with [=ReadableStream/[[disturbed]]=]. For usages of [[state]] and [[storedError]], care must be taken to check whether the usage references the slot on ReadableStream or on WritableStream.

I'm not sure if this is the most compact way to do it? A link like [=ReadableStream/[[readableStreamController]]=] is quite long, but I don't know if there's a way to write only [=[[readableStreamController]]=] and have Bikeshed figure out that it needs to use the ReadableStream namespace. 🤷

See #1048.

---

Preview | Diff

[domenic]

This looks quite nice to me. I'd be happy to proceed with this.

Feel free to throw 154bfb4 into #1051 if you want to morph that into a general "editorial fixups" PR.

I'm not sure if this is the most compact way to do it? A link like [=ReadableStream/[[readableStreamController]]=] is quite long, but I don't know if there's a way to write only [=[[readableStreamController]]=] and have Bikeshed figure out that it needs to use the ReadableStream namespace. 🤷

The alternative I can think of is just assuming internal slot names are unique, and then removing the dfn-for="". However, we have a few collisions these days (since we are no longer using internal slots for brand checking), and I'd like to introduce more going forward, e.g. [[readableStreamController]] -> [[controller]]. So that would only work in some cases, and we'd need for="" for others. I think this is reasonable.

(As a side note, we might want to do any internal slot renames ahead of this change, since they'll start having anchors that could break after this. However, nobody should really be linking to these externally, so maybe it's fine.)

Finally, we should rewrap all the lines to 100 characters. Hopefully we can find an automated way to do that, although all the tools I've tried are a bit thrown off by the one-space markdown-ish indents. It might be a case of https://xkcd.com/1205/ (or https://xkcd.com/1319/, or https://xkcd.com/974/).

[MattiasBuelens]

Feel free to throw 154bfb4 into #1051 if you want to morph that into a general "editorial fixups" PR.

Looks like 154bfb4 has already been superseded by 63b4407. 😄 I'll get rid of that commit next time I rebase this PR.

The alternative I can think of is just assuming internal slot names are unique, and then removing the dfn-for="". However, we have a few collisions these days (since we are no longer using internal slots for brand checking), and I'd like to introduce more going forward, e.g. [[readableStreamController]] -> [[controller]]. So that would only work in some cases, and we'd need for="" for others. I think this is reasonable.

Yeah, there's no way to make all internal slots unique, I'm afraid. Both ReadableStreamDefaultReader and ReadableStreamBYOBReader have [[closedPromise]] and [[ownerReadableStream]] slots, and although they serve the same purpose, they're technically different slots. Oh, and of course there's also WritableStreamDefaultWriter with its own [[closedPromise]] slot.

I'm a big fan of renaming things like [[readableStreamController]] to just [[controller]]. And I agree, we should probably do those renames first. 👍

[MattiasBuelens]

Ran into an issue. ReadableStreamClose has the following step:

6. Resolve reader.[[closedPromise]] with undefined.

However, [[closedPromise]] may refer to the internal slot of either ReadableStreamDefaultReader or ReadableStreamBYOBReader. So there's no single link I can put here. 😕

Do we need to introduce an "interface mixin" (like TextDecoderCommon in the Encoding spec) where we can define the internal slots shared between the two reader classes (i.e. [[closedPromise]] and [[stream]])?

Or should we add a section "Interfacing with readers" just to define the common internal slots without any Web IDL, like we did with "Interfacing with controllers"?

[domenic]

Here are some options that I'd be happy with:

1. Don't link for this special case.
2. Branch the spec in a way that reads kinda strange: "If reader is a ReadableStreamDefaultReader, resolve reader.[[closedPromise]] with undefined. If reader is a ReadableStreamBYOBReader, resolve reader.[[closedPromise]] with undefined." With each sentence having a difference link.
3. Add an "Interfacing with readers" section.

Personally, I don't think we should change the IDL for this kind of internal factoring concern... although I can kind of see the appeal of formally expressing that this sub-part of the reader classes is shared, with its own identity. That is, this does feel a bit more like a shared sub-part, than like polymorphism.

Anyway, (3) is a bit weird to me. Do we move the [[stream]] slot there too? Is this all for a single [[closedPromise]] manipulation?

If such polymorphic access to [[closedPromise]] happens multiple times, then I'd feel better about (3). And if polymorphic access to [[stream]] happened, then I'd definitely say we should do (3). But if this is rare, then (1) or (2) seem more reasonable.

[domenic]

Hmm, I'll also note that (2) isn't as bad as I've written it, since the algorithm already contains a specific branch for ReadableStreamDefaultReader. So, if this is the only such case, then I'd lean toward (2).

[MattiasBuelens]

Unfortunately, it's not just this one case. All of the following abstract ops can be called on both types of readers. As such, (1) is not an option:

• ReadableStreamReaderGenericInitialize uses both reader.[[stream]] and reader.[[closedPromise]]
• ReadableStreamReaderGenericRelease uses both reader.[[stream]] and reader.[[closedPromise]]
• ReadableStreamClose uses reader.[[closedPromise]]
• ReadableStreamError uses reader.[[closedPromise]]

I don't like (2). There are too many places where we would need branching.

I think the existence of ReadableStreamReaderGenericInitialize and ReadableStreamReaderGenericRelease already suggest that there should be some concept of a "generic reader". I'm not sure whether this should be expressed in the Web IDL or not. For now, I'm leaning more towards (3).

[domenic]

The fact that there are so many cases makes (3) feel pretty reasonable to me. My main concern was creating a whole new section and indirection for only 1-2 call sites, but with 6, I'm happy.

[MattiasBuelens]

Well, I tried something. It's very rough, so I'd very much like to hear your suggestions! 😄

Honestly, I think we'd be better off with an actual interface mixin ReadableStreamGenericReader. That way, we could move the shared internal slots and the shared closed getter and cancel(reason) method to that mixin. But I suppose such a change is no longer merely editorial, and would need a separate PR?

(I know the lines are too long, I'll fix those later.)

[ricea]

The fact that there are so many cases makes (3) feel pretty reasonable to me. My main concern was creating a whole new section and indirection for only 1-2 call sites, but with 6, I'm happy.

(3) is useful from an implementation perspective, because we need to do that anyway.

[ricea]

Honestly, I think we'd be better off with an actual interface mixin ReadableStreamGenericReader. That way, we could move the shared internal slots and the shared closed getter and cancel(reason) method to that mixin. But I suppose such a change is no longer merely editorial, and would need a separate PR?

My non-binding opinion is that interface mixins count as editorial, since you can't test for them and they don't necessarily change the implementation.

[domenic]

Interesting!

Moving to an interface mixin is indeed editorial. I can see the appeal. I've also run into the desire to link to a single public API for things like https://whatpr.org/streams/1050.html#dom-underlyingsource-cancel; as you can see I currently use text like "defaultReader.cancel() or byobReader.cancel()".

However, I think we should game this out a bit more. If we start using mixins for something like this... will we also want to use them for controllers? A lot of internal slots and public APIs are shared between the two readable stream controllers. And heck, maybe more abstract ops would be shared, if we had a common place to share them. (Not sure about that one.)

I also worry a bit about the impact on spec readability of splitting readers out so that you have to read at least 2 sections per reader, instead of 1. This is most stark for the domintro sections, aimed at web developers; would we split those up?

I'll proceed to respond to your questions and do a review based on sticking with (3), but I'm more open to discussing interface mixins now.

[MattiasBuelens]

Well, I gave the interface mixin thingy a go, just to see what it'd look like. 😁

It's probably too big of a change, so I understand if we want to revert this for now.

[MattiasBuelens]

If we start using mixins for something like this... will we also want to use them for controllers? A lot of internal slots and public APIs are shared between the two readable stream controllers. And heck, maybe more abstract ops would be shared, if we had a common place to share them. (Not sure about that one.)

I suppose we could share some code between SetUpReadableStreamDefaultController() and SetUpReadableByteStreamController(). The properties and methods (desiredSize, close, enqueue, and error) all use different abstract ops, and while they do share a similar structure, they call into different ops like ReadableStreamDefaultControllerShouldCallPull and ReadableByteStreamControllerShouldCallPull. We could try using polymorphism for that, but I'm worried it could get weird... 😕

I also worry a bit about the impact on spec readability of splitting readers out so that you have to read at least 2 sections per reader, instead of 1. This is most stark for the domintro sections, aimed at web developers; would we split those up?

In the Encoding spec, the domintro section about the encoding getter is repeated for TextDecoder and TextDecoderStream. The links inside those sections do link back to TextDecoderCommon.encoding, though.

I think we could do the same: keep the domintro sections for ReadableStreamDefaultReader and ReadableStreamBYOBReader, but link to the definitions in ReadableStreamGenericReader.

[domenic]

This is looking good to me; I'm OK with the interface mixin approach. We gotta delete the double-definitions, and it'd also be good to update https://whatpr.org/streams/1050.html#dom-underlyingsource-cancel to link to the new, consolidated cancel() definition.

And then we need to update the reference implementation too. You can leave the .js files as-is and just update the .webidl as a first pass, but ideally we should match the spec and have some mixed in implementations like the spec does. See https://github.com/jsdom/jsdom/blob/88e72ef028913e78266b8105493fd6d973c68e38/lib/jsdom/living/nodes/Slotable-impl.js and references to that in jsdom for an example.

[MattiasBuelens]

This is now feature complete. I still need to do one big pass to fix all the line wrapping, but you can already review the changes. (If anyone has tooling suggestions for the line wrapping, let me know. 😅)

And then we need to update the reference implementation too. You can leave the .js files as-is and just update the .webidl as a first pass, but ideally we should match the spec and have some mixed in implementations like the spec does. See https://github.com/jsdom/jsdom/blob/88e72ef028913e78266b8105493fd6d973c68e38/lib/jsdom/living/nodes/Slotable-impl.js and references to that in jsdom for an example.

Done! 😁

[MattiasBuelens]

Shamelessly stolen from jsdom. 😁

[domenic]

(If anyone has tooling suggestions for the line wrapping, let me know. 😅)

https://domenic.github.io/rewrapper/ works on pure HTML, but I haven't tested it with the Markdown style we're doing here... maybe too much yak-shaving, but patches welcome.

[domenic]

I pushed a few more line-wraps that I found by skimming, including some preexisting issues in sections of the spec you didn't touch. This LGTM!! Amazing work; thank you so much for your continued contributions.

I'll give this a day or so in case @ricea wants to take a look (especially at the mixin change), but I'm happy to squash and merge.

[ricea]

Really elegant. lgtm.

Sorry for the delay in reviewing this. Feel free to merge once you've attended to the minor issues I raised.