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

Early draft of WritableStream "Design Philosophy" section #718

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ricea
Copy link
Collaborator

@ricea ricea commented Mar 29, 2017

This is supposed to help implementors and others understand the design
principles.

Markup isn't done properly yet, so please evaluate the text only.


Preview | Diff

This is supposed to help implementors and others understand the design
principles.

Markup isn't done properly yet, so please evaluate the text only.
@ricea
Copy link
Collaborator Author

ricea commented Mar 29, 2017

@domenic @tyoshino Suggestions, comments, radical changes all welcome.

I uploaded it as a whatwg/streams branch to make it easy to look at the preview, which should show up at
https://streams.spec.whatwg.org/branch-snapshots/writable-streams-design-philosophy/ eventually.

Also about developer predictability > implementation simplicity.
index.bs Outdated

<ul>
<li><p>Only one sink method can ever be executing at a time.
<li><p>Sink methods are treated as atomic.
Copy link
Member

Choose a reason for hiding this comment

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

IMO this can be one paragraph instead of sub-bullets

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I use paragraphs for everything instead of sub-bullets? I quite like the sub-bullets to be honest.

index.bs Outdated
<li><p>Sink methods are treated as atomic.
<ul>
<li><p>"Atomic" here means that the time between a sink method being called and the returned Promise resolving or
rejecting is treated as indivisible.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really clarify. What would divide them? I think the next bullet points actually state what atomic means.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just took this out.

index.bs Outdated
<li><p>"Atomic" here means that the time between a sink method being called and the returned Promise resolving or
rejecting is treated as indivisible.
<li><p>A new sink method will never be called until the Promise from the previous one has resolved.
<li><p>State changes do not take effect until any in-flight sink method has completed.
Copy link
Member

Choose a reason for hiding this comment

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

"State changes" isn't too clear here, especially given the next two bullet points worth of exceptions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworded a bit.

index.bs Outdated
now would be effective.
<ul>
<li>writer.ready and writer.desiredSize will change even in the middle of executing a sink method, as soon as we
know that calling writer.write() won't work any more.
Copy link
Member

Choose a reason for hiding this comment

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

They also change in cases besides "won't work" though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworded a bit.

index.bs Outdated
<div class=note>Because promises are dispatched asynchronously, the state can still change between
writer.ready becoming fulfilled and write() being called.</div>
<li>The value of writer.desiredSize decreases synchronously with every call to writer.write(). This implies that
strategy.size() is executed synchronously.
Copy link
Member

Choose a reason for hiding this comment

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

Given how we're not calling strategy.size as a method, I'd state this as "the queuing strategy's size() function is executed..."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
<li><p>The writer.closed promise and the promises returned by writer.close() and writer.abort() do not resolve or
reject until no sink methods are executing and no further sink methods will be executing.
<ul>
<li>If the user of the WritableStream wants to retry against the same underlying data item, it is important to
Copy link
Member

Choose a reason for hiding this comment

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

"data item" = chunk?

"Retry against" is a bit confusing to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to clarify by using the example of a file, rather than try to come up with a general term for "named or otherwise identifiable entities which can be retried independently of the lifetime of any particular stream".

index.bs Outdated
have confidence that all other operations have ceased.
<li>This principle also applies to the ReadableStream pipeTo() method.
</ul>
<li><p>Promises resolve or reject in consistent order. In particular, writer.ready always resolves before
Copy link
Member

Choose a reason for hiding this comment

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

I think "fulfill" is more informative here (although both are technically accurate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
<li>This principle also applies to the ReadableStream pipeTo() method.
</ul>
<li><p>Promises resolve or reject in consistent order. In particular, writer.ready always resolves before
writer.closed, even in cases where they both resolve "at the same time".
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 state this as "where both are fulfilling in reaction to the same occurrence"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great! Thanks.

index.bs Outdated
writer.closed, even in cases where they both resolve "at the same time".
</ul>

<div>Some of these design decisions improve predictability, ease-of-use and safety for developers at the expense of
Copy link
Member

Choose a reason for hiding this comment

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

Oxford comma please (after "ease-of-use")

I know it's not markup-critiquing time yet but this <div> seems pretty unnecessary :)

Copy link
Member

Choose a reason for hiding this comment

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

I think this could move up to merge with the intro actually.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved it up and made it a paragraph.

index.bs Outdated
<h3 id="ws-design-philosopy">Design Philosophy</h3>

<div>While sharing the principles for streams in general, a number of additional principles have informed the design of
the WritableStream class.</div>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can make clearer the "scope" of this section. It's got great info but it's of a very specific kind. It's not about the philosophy behind writable streams in general (of the type of stuff covered by the "Model" section). It's more about the details of its API surface and the interactions between the sink, controller, and writer APIs. Not sure on the exact phrasing, but that might be a start.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the name to "Design Of The State Machine" which may or may not be helpful.

@ricea
Copy link
Collaborator Author

ricea commented Mar 30, 2017

I added another item, "Queued calls to writer methods such as write() are not cancelled when writer.releaseLock() is called. This makes them easy to use in a "fire and forget" style." which raises the question of exactly how much detail belongs in this section.

Personally I'm happy with it being quite inclusive, but I don't think it should be an exhaustive list of every design decision we ever made.

Maybe I should rephrase it as "Fire-and-forget style usage is supported. Therefore..."

@@ -2568,6 +2568,45 @@ nothrow>ReadableByteStreamControllerShouldCallPull ( <var>controller</var> )</h4
writeRandomBytesForever(myWritableStream).catch(e => console.error("Something broke", e));
</code></pre>
</div>

<h3 id="ws-design-of-the-state-machine">Design Of The State Machine</h3>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Needs to be marked non-normative)

@ricea ricea added the do not merge yet Pull request must not be merged per rationale in comment label Mar 8, 2018
Base automatically changed from master to main January 15, 2021 07:25
Copy link

@Ansiic112 Ansiic112 left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment documentation writable streams
Development

Successfully merging this pull request may close these issues.

3 participants