gloo icon indicating copy to clipboard operation
gloo copied to clipboard

[RFC] Add FileReaderSync API to gloo-file

Open kartva opened this issue 4 years ago • 4 comments

Summary

Add FileReaderSync API to gloo-file

Motivation

To enable synchronous File reading from Web Workers.

Detailed Explanation

Unlike the callback-based FileReader, this API is not callback-driven, which means that we can make it simpler, as:

  • A RAII style struct for automatically cancelling the read is not required, which means we don't need a struct.
mod FileReaderSync {
    pub fn readAsArrayBuffer (blob: &Blob) -> Result<ArrayBuffer, FileReadError>
    // ... similar method signatures to FileReader, without the callbacks
}

Drawbacks, Rationale, and Alternatives

Can consider transforming FileReaderSync into a trait which is implemented on Blobs and such, which makes the API prettier, but less clear?

use gloo_file::FileReaderSync;

let file = get_file().readAsArrayBuffer();
let data = Uint8Array::new(file);

Prior art:

Unresolved Questions

  • Implement std::io::Read for Blob/File using FileReaderSync. However, the implementation involves using readAsArrayBuffer and such, which makes it look like a mid-level API.

  • Might look into adding a few more error-types to FileReadError, which are specific to particular types of reads, such as EncodingError present in readAsDataURL.

Don't know whether they are in the scope of this PR.

Closes #129 in favor of this issue.

kartva avatar Jun 25 '21 07:06 kartva

Ping @hamza1311

This RFC is supposed to get accepted before I start a PR for the changes, right? If yes, then what is the procedure?

kartva avatar Jun 29 '21 10:06 kartva

I personally would wait for anyone to point out any objections that they have then start working on PR, if they have none. I guess this Is fine to be implemented.

One thing of note: please keep the API similar to callback/future based one. So like how there's a futures module, add a sync module with the implementation. Ideally, switching between sync and future based one should be similar to switching between std and async_std (1:1 corespondence between the APIs)

ranile avatar Jun 29 '21 10:06 ranile

Blocked on implementing Web Workers first.

kartva avatar Oct 28 '21 06:10 kartva

Closing the implementation PR is fine but I'll open this issue. We can work on it once Web Workers are supported. It's not like we are not implementing this at all and I don't want anyone searching through issues to get the wrong idea,

ranile avatar Oct 29 '21 10:10 ranile