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

Why not use ReadableStream/WritableStream/WritableStream*Writer #19

Closed
kenchris opened this issue Aug 2, 2018 · 16 comments
Closed

Why not use ReadableStream/WritableStream/WritableStream*Writer #19

kenchris opened this issue Aug 2, 2018 · 16 comments
Assignees
Milestone

Comments

@kenchris
Copy link

kenchris commented Aug 2, 2018

Instead of the FileReader and FileWriter.

It could potentially be specialized versions, or at least the new objects could share API

If there are obvious reasons why these cannot be used, I think the explainer should say so.

Of course FileReader already exists, but FileWriter doesn't to my knowledge, so it would be good to figure out what is the best API going forward.

Currently you have createWriter and WritableStream has getWriter returning a WritableStreamDefaultWriter:

https://streams.spec.whatwg.org/#default-writer-class

The FileReader also doesn't support streaming of content, unless you read manually by using .slice()

@kenchris kenchris changed the title Why not use ReadableStream/WritableStream Why not use ReadableStream/WritableStream/WritableStream*Writer Aug 2, 2018
@mkruisselbrink
Copy link
Contributor

For reading, see #2 and w3c/FileAPI#40; i.e. I'm not against adding something to File/Blob to make it easier to use a stream to read from a file, but it's already pretty easy to create a ReadableStream out of a Blob (and no, that doesn't require using slice).

For writing, this can't just use WritableStream at least, as that wouldn't support seeking/random access. But perhaps it would indeed make sense to optionally let you treat a file reference as a WritableStream. Not quite sure what that would look like. Also the explainer currently (somewhat on purpose) completely glosses over things like locking files, if multiple writers should be able to write to the same file at the same time, etc.

@kenchris
Copy link
Author

kenchris commented Aug 2, 2018

Yes, a WritableStream would lock by default. At least the FileWriter could attempt to use the same method names for the same things.

Would it be possible to inherit from a WritableStream instead and add the needed functionality (seek?)

@mkruisselbrink
Copy link
Contributor

Thinking about this a bit more, being able to pass in a ReadableStream to write(), like the current explainer mentions, also means you can use an identity transform stream to get a WritableStream to then write to the file (i.e. like how https://streams.spec.whatwg.org/#example-transform-identity describes the same for providing a request body). So I'm really not sure about the need for more API surface here...

@domenic
Copy link
Contributor

domenic commented May 8, 2019

I'd like to bump this a bit. The current shape of the spec is pretty nice: you get a FileSystemWriter which has a write(position, data), asWritableStream(), truncate(), and close() method. But I think it could be even nicer.

What if instead you had a WritableFileStream subclass of WritableStream, which added the seek and truncate operations? Then we'd stay with a single paradigm for writing, instead of introducing a new one with a converter method to get to the standard one. You'd probably want to also create a WritableFileStreamDefaultWriter subclass of WritableStreamDefaultWriter.

Then the code would look something like

const stream = await fileHandle.createWritable();

// All I want to do is truncate:
await stream.truncate(1000);

// Or I want to do more complicated stuff:
const writer = stream.getWriter();
writer.seek(10);
writer.write(uint8Array);
writer.truncate(100);

// Or I want to compose with readablestream:
await readable.pipeTo(stream);

What do you think?

@mkruisselbrink
Copy link
Contributor

hmm, yeah... that does seem potentially nice. The main reason I hadn't done anything like that was that I wasn't sure if/how actually deriving from WritableStream and or WritableStreamDefaultWriter would work, both in spec and implementation. You don't think it would be weird to have thinks like seek and truncate on a writable stream like object?

With such an API would it be crazy for write to not just accept uint8arrays, but also accept strings (that are utf-8 encoded) and (more importantly) blobs as well?

cc @oyiptong

@domenic
Copy link
Contributor

domenic commented May 8, 2019

wasn't sure if/how actually deriving from WritableStream and or WritableStreamDefaultWriter would work, both in spec and implementation.

In spec, we've been hand-waving a bit, treating them as IDL types. After whatwg/streams#963 it should be on solid ground, but until then, I think the semantics are relatively clear, if you do something like interface WritableFileStream : WritableStream {.

In the Blink implementation, similarly, this will be much easier with @ricea and @yhirano's work to move to IDL-based types.

You don't think it would be weird to have thinks like seek and truncate on a writable stream like object?

Nah. Having specialized subtypes that give you higher-level or resource-specific operations has always been part of the plan. Not one we've stressed yet, so there might be some kinks, but conceptually it hangs together.

With such an API would it be crazy for write to not just accept uint8arrays, but also accept strings (that are utf-8 encoded) and (more importantly) blobs as well?

We've tried to keep streams to be single-type-chunked so far. Especially for string vs. Uint8Array, we've encouraged people to pipe through a TextEncoderStream or similar.

This isn't enforced by the API; it's just a convention so far. So you could allow them to accept more, and do the conversion for your users, but it makes me a bit uncomfortable... Let's compare with the alternative, of a Uint8Array-only version:

const encoder = new TextEncoderStream();
encoder.readable.pipeTo(writableFileStream);
const writableFileStreamAcceptingStrings = encoder.writable;

// Now use writableFileStreamAcceptingStrings as usual, e.g.
writableFileStreamAcceptingStrings.getWriter().write("a string");
blob.stream().pipeTo(writableFileStream);

Seems not so bad, but I guess not as nice. Hmm. I'm unsure.

@oyiptong
Copy link
Collaborator

oyiptong commented May 8, 2019

Had a chat with @mkruisselbrink, he suggested instead to have the write API accepting strings and other types to instead be at the top-level.

The reasoning is to have convenience methods be easily accessible.
e.g. Just wanting to truncate a file, or to write a string.

Conceptually, if one wants more control, they'd obtain the WritableFileStreamDefaultWriter instance and get more fine-grained control.

I.e. something akin to:

const stream = await fileHandle.createWritable();
// Just write something.
await stream.write(0, "foo");

// Need more control now.
const writer = stream.getWriter();
writer.seek(4);
await writer.write(new Uint8Array([0x21, 0x21]));
...

@domenic
Copy link
Contributor

domenic commented May 8, 2019

Ooh, I like that a lot!

Riffing off that further, I wonder if you could even move the seeking before the getWriter() call, so that way you can keep using WritableStreamDefaultWriter. Then you have a nice clean separation of high-level manipulation/preparation APIs on the stream, and lower-level simple-sequential-stream operations once you grab the writer.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 21, 2019
This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 22, 2019
This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 22, 2019
This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 22, 2019
This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 23, 2019
This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623534
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Auto-Submit: Olivier Yiptong <oyiptong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662620}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 23, 2019
This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623534
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Auto-Submit: Olivier Yiptong <oyiptong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662620}
@mkruisselbrink mkruisselbrink added this to the MVP milestone Jun 4, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 19, 2019
…m API, a=testonly

Automatic update from web-platform-tests
[Native File System] Writable File Stream API

This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623534
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Auto-Submit: Olivier Yiptong <oyiptong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662620}

--

wp5At-commits: 2263ea8f0429928725fb287e2ffd3d4f8bcc279e
wpt-pr: 16945
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jun 19, 2019
…m API, a=testonly

Automatic update from web-platform-tests
[Native File System] Writable File Stream API

This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623534
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Auto-Submit: Olivier Yiptong <oyiptong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662620}

--

wp5At-commits: 2263ea8f0429928725fb287e2ffd3d4f8bcc279e
wpt-pr: 16945
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
This change adds a writable stream output to the `FileSystemFileHandle`.

This follows some
[discussion](WICG/file-system-access#19 (comment)) on github.

While this writable stream does not yet feature stream-like behavior, it
carries over some of the interface from `NativeFileSystemWriter`, namely
`truncate` and `write`.

Bug: 955189
Change-Id: I8b8c9ff1ef36adfb30501251563699c6c7973889
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623534
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Auto-Submit: Olivier Yiptong <oyiptong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662620}
@kenchris
Copy link
Author

Ae you moving ahead with this new API suggestion?

@oyiptong
Copy link
Collaborator

Yes, the plan is to iterate on this API suggestion.

While we originally started implementation on this API shape, as we got deeper into implementing our security model, it turns out we might need to revise this API a bit.

For instance, for the non-stream writer, we have a well understood way to protect users from harm (#37) and at the same time enabling atomic writes (#48), for the use-case where developers do not want incomplete writes. This has materialized in the spec in #51.

This API suggestion as is currently does not have an explicit Close or Flush statement, which we need to implement the security model in the same way as we did for the non-stream writer.

As for the streams implementation, the next step is to figure out a way to protect users and at the same time have a decent API shape.

@mkruisselbrink
Copy link
Contributor

To clarify a bit on this, I don't think the concerns around safe browsing like behavior particularly impact the design of a streams based writing API. Writable Streams do support a "close" operation, so it should be no problem to match the current behavior with an API that exposes writing using writable streams (modulo fixing whatwg/streams#1007, as it would be a bit awkward to have to create a writer from the stream just to be able to close the stream).

We just put figuring out the best way to have a streams based API a bit on the backburner, because the streams implementation in Blink wasn't ready to support other classes deriving from WritableStream.

So yes, we definitely plan to pursue this, I don't see any major blockers, we just haven't prioritized working on the spec side of this since we were blocked on the implementation side of this.

@mkruisselbrink mkruisselbrink modified the milestones: MVP, V1 Aug 5, 2019
@lgrahl
Copy link

lgrahl commented Aug 5, 2019

Perhaps you can nudge the Blink team to progress on WritableStream since there is more and more interest in using it (another example being WebTransport). 🙂

@mkruisselbrink
Copy link
Contributor

Oh, progress is certainly being made. The streams implementation just has been going through a rewrite that made building other native features on top of it harder, but that is rolling out/should be rolling out soon, I believe.

@oyiptong oyiptong self-assigned this Nov 11, 2019
@oyiptong
Copy link
Collaborator

oyiptong commented Dec 4, 2019

@kenchris, I'm compiling some public documentation about this API change and I was wondering if you had a specific use-case in mind to use streams.

The pwrite-style API provides all capabilities necessary, and streams enables a higher level API with access to stream semantics.

However, in view of making a good case for this change in my document, perhaps you could share TAG's perspective on using Streams semantics for the writer?

@kenchris
Copy link
Author

kenchris commented Dec 9, 2019

^ maybe something for one of our upcoming calls @torgo @plinss @cynthia ?

@mkruisselbrink
Copy link
Contributor

The API currently does use WritableStream for writing. Closing this, but open to feedback for ways we should change the API.

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

No branches or pull requests

5 participants