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

Add WritableStream#write convenience method #1339

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

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Jan 23, 2025

Add a WriteableStream.write convenience method to write a single chunk. It locks, writes, then unlocks all in one call.

Towards #1072

  • At least two implementers are interested (and none opposed):
    • Deno
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno: …
    • Node.js: …
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

index.bs Show resolved Hide resolved
@jasnell
Copy link

jasnell commented Jan 23, 2025

The fact that parallel writes wouldn't be allowed (since each write locks the writable) is a bit of a challenge here. I'm not entirely convinced that this is the right thing to add. Can you expand on the motivating use cases a bit more?

index.bs Outdated Show resolved Hide resolved
<div algorithm>
The <dfn id="ws-write" method for="WritableStream">write(|chunk|)</dfn> method steps are:

1. Let |result| be ? [$AcquireWritableStreamDefaultWriter$]([=this=]).
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth introducing a new abstract op WritableStreamWrite(stream, chunk) for doing a "writer-less write", similar to WritableStreamAbort and WritableStreamClose. We could then rework WritableStreamDefaultWriterWrite to use the same abstract op, similar to how WritableStreamDefaultWriterAbort delegates to WritableStreamAbort.

(We may have to take chunkSize as an extra argument though, since step 5 of WritableStreamDefaultWriterWrite might be difficult to implement otherwise.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do that if I can get implementer interest :)

@lucacasonato
Copy link
Member Author

I'm looking for implementer interest for this change. How are Chromium, Gecko, WebKit, and Cloudflare folks feeling about this? @saschanaz @domenic @jasnell @annevk

@lucacasonato
Copy link
Member Author

@jasnell Yeah see the first thread on this PR. I changed it to do acquire(); write(); release(); waitForWrite(); now.

@lucacasonato lucacasonato marked this pull request as ready for review January 24, 2025 10:24
@annevk
Copy link
Member

annevk commented Jan 24, 2025

Nit: please change the commit/PR title to make this not look like it's adding a static method.

@lucacasonato lucacasonato changed the title Add WritableStream.write convenience method Add WritableStream#write convenience method Jan 24, 2025
@youennf
Copy link
Contributor

youennf commented Jan 24, 2025

@lucacasonato
Copy link
Member Author

lucacasonato commented Jan 24, 2025

@youennf Yes (but for all WritableStream), not just the FS subclass.

@saschanaz
Copy link
Member

I'm interested to have this to replace Bucket FS side equivalent. cc @janvarga

@janvarga
Copy link

delegating to @jjjalkanen

@jasnell
Copy link

jasnell commented Jan 24, 2025

@lucacasonato Cloudflare will implement if this moves forward. I'm not super convinced of the use cases yet as I haven't had anyone ask for this but I certainly don't have a reason not to support it.

@annevk
Copy link
Member

annevk commented Jan 24, 2025

If this is happening we should probably remove/refactor FileSystemWritableFileStream#write at the same time.

@domenic
Copy link
Member

domenic commented Jan 27, 2025

I'm in favor of this but for Chromium it's @ricea and @nidhijaju who would be doing the work, so please wait for one of them to confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants