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 simpler reading methods to Blob interface. #117

Merged
merged 5 commits into from
Apr 12, 2019
Merged

Conversation

mkruisselbrink
Copy link
Collaborator

@mkruisselbrink mkruisselbrink commented Mar 7, 2019

This fixes #40

For now this just more or less copies the hand-wavy text of "perform a read operation". We'll need to better specify what actually reading from a blob does. Separately in a follow-up I would also like to rephrase the existing FileReader/FileReaderSync spec text in terms of readable stream operations, being much more precise when and how state is updated and events are queued.


Preview | Diff

@mkruisselbrink
Copy link
Collaborator Author

@annevk would you mind taking a look at this?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
1. [=ReadableStream/Enqueue=] |bytes| into |stream|.

Issue: We need to specify more concretely what reading from a Blob actually does, and what
possible errors can happen.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, and how large is a chunk? Is there some POSIX operation we can hint at?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if size of chunks is ever going to really matter, as that shouldn't really be observable anyway... These new methods only return the whole stream in one go anyway, and while the existing FileReader methods do give you progress events those are more time-limited (with the same ~50ms as used by XHR) rather than directly tied to chunks that are pushed in the stream. And I expect this always to be somewhat hand-wavy, but at least I hope to have one place that describes how to get a ReadableStream out of a blob, and then the rest of the spec can just use that stream and not have any hand-waving or ambiguities... This "algorithm" at least doesn't seem worse than the current defintion of read operations.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the stream() method gives you incremental access, right? I was interested in that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, yeah. Not sure what we can helpfully say about chunk sizes though... What does fetch do for example? Especially when a response is read from the http cache it seems like that is sort of a similar situation as this one? Does it specify anything about how the body is streamed?

Copy link
Member

@annevk annevk Mar 13, 2019

Choose a reason for hiding this comment

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

It uses https://fetch.spec.whatwg.org/#concept-read-chunk-from-readablestream. As I mentioned in the other issue though there are some problems with how the construct works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, that is for reading chunks from the stream. The bit I can't find is how it is specified what chunks you're going to get out of a stream. I.e. if you call (await fetch('....')).body, and read from that. I don't think it is specified what those chunks are either? I can imagine if the fetch hit the network it would depend on what the server returns, but if it is loaded from the cache it is less clear. Fetch just seems to link to https://tools.ietf.org/html/rfc7234#section-4 for how the Response (and its body) are created when loaded from the cache, but also doesn't mention anything about chunk size etc.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, see step 13 of https://fetch.spec.whatwg.org/#http-network-fetch. It basically waits for a number of bytes to arrive and then enqueues a Uint8Array. That's probably what you want to mimic to some extent, until there's better infra.

index.bs Outdated Show resolved Hide resolved
@mkruisselbrink
Copy link
Collaborator Author

(of course this will need tests before landing as well)

@mkruisselbrink
Copy link
Collaborator Author

Okay, rebased this on top of the FileReader changes.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Looks good to me % nits. With tests and implementation bugs this'll be a great addition to the platform. Hopefully everyone picks it up quickly.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@Jamesernator
Copy link

Any possibility of including .dataURL() as well?

@annevk
Copy link
Member

annevk commented Mar 22, 2019

@Jamesernator let's track that separately.

@mkruisselbrink
Copy link
Collaborator Author

Tests are in progress in https://chromium-review.googlesource.com/c/chromium/src/+/1526796.

Implementation bugs: Chromium, WebKit, FireFox

@jarryd999
Copy link

Hey @annevk, do you think Mozilla will implement this soon? I'm filing the Intent to Implement/Ship for blink and I'd like to accurately represent Mozilla's stance.

@annevk
Copy link
Member

annevk commented Apr 5, 2019

I can't really comment on a timeline, but we're supportive of these methods being added to the web platform.

@jimmywarting
Copy link

Any possibility of including .dataURL() as well?

  • I would suggest using URL.createObjectURL(blob) instead of using a base64 string
  • if you want to upload/save a file you should send it as a binary

base64 is ~3x larger

@mkruisselbrink mkruisselbrink merged commit b0fa7ef into master Apr 12, 2019
@annevk annevk deleted the blob-to-stream branch April 13, 2019 07:39
foolip added a commit to foolip/FileAPI that referenced this pull request Apr 13, 2019
mkruisselbrink pushed a commit that referenced this pull request Apr 13, 2019
@jimmywarting
Copy link

jimmywarting commented Apr 16, 2019

Should the FileReader change how it gets the stream?

- Should Let stream be the result of calling get stream on blob .
+ Should Let stream be the result of calling blob.stream()

That would make fileReader a self contained module and it dosen't have to have any internal access to blobs private methods

@annevk
Copy link
Member

annevk commented Apr 16, 2019

@jimmywarting please file a new issue if you feel strongly, but generally we don't use that pattern to describe platform objects. If someone were to modify Blob.prototype.stream it shouldn't affect FileReader. It would actually add quite some complexity as we'd have to handle all the exceptional cases that might arise from such a setup.

@jimmywarting
Copy link

jimmywarting commented Apr 16, 2019

hmm okey, will stay on the fence. will trust that you know what is best

@annevk
Copy link
Member

annevk commented Apr 16, 2019

@jimmywarting happy to discuss this further somewhere somehow. It kinda falls out of IDL and how browsers implement platform objects. https://annevankesteren.nl/2015/01/javascript-web-platform explains some of this, but doesn't really go into why we try to avoid invoking JavaScript once we cross the IDL boundary.

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

Successfully merging this pull request may close these issues.

Streams are hot, FileReader.readAs____ is not
6 participants