Skip to content

Files implementation #110

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

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Conversation

richard-uk1
Copy link
Contributor

@richard-uk1 richard-uk1 commented Jan 18, 2020

This carries on from the work in #83 from @rylev. I've tried to address the issues in that thread.

Closes #47

@richard-uk1
Copy link
Contributor Author

Concerns:

  • Could do with more test coverage
  • Callback interface for reading blobs could use gloo::events.
  • The error handling might be broken (I'm not sure how to trigger the different kinds of errors in tests).

We could address these in followup work.

@richard-uk1 richard-uk1 requested a review from Pauan January 18, 2020 21:53
@richard-uk1
Copy link
Contributor Author

I've added a patch that uses a slightly different approach to the Blob -> File inheritance hierarchy, using the Deref trick used in web_sys. See #02c675c for the original implementation.

@richard-uk1 richard-uk1 force-pushed the files-implementation branch 2 times, most recently from 2255819 to 36d9c76 Compare January 18, 2020 23:28
@richard-uk1 richard-uk1 changed the title Files implementation [wip] Files implementation Jan 19, 2020
@richard-uk1 richard-uk1 changed the title [wip] Files implementation Files implementation Jan 19, 2020
@richard-uk1
Copy link
Contributor Author

richard-uk1 commented Jan 19, 2020

Just to show how ace gloo can be: here is how to read in a file and convert it into a data url:

async fn data_url_future() {
    let blob = Blob::new_with_options(PNG_FILE, Some("image/png"));
    assert_eq!(
        gloo_file::futures::read_as_data_url(&blob).await.unwrap(),
        PNG_FILE_DATA
    );
}

const PNG_FILE: &'static [u8] = &[
    0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52,
    0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x01, 0x03, 0x00, 0x00, 0x00, 0x25, 0xdb, 0x56,
    0xca, 0x00, 0x00, 0x00, 0x03, 0x50, 0x4c, 0x54, 0x45, 0xff, 0x4d, 0x00, 0x5c, 0x35, 0x38, 0x7f,
    0x00, 0x00, 0x00, 0x01, 0x74, 0x52, 0x4e, 0x53, 0xcc, 0xd2, 0x34, 0x56, 0xfd, 0x00, 0x00, 0x00,
    0x0a, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x63, 0x62, 0x00, 0x00, 0x00, 0x06, 0x00, 0x03, 0x36,
    0x37, 0x7c, 0xa8, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82,
];

const PNG_FILE_DATA: &'static str =
    "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAA\
     Al21bKAAAAA1BMVEX/TQBcNTh/AAAAAXRSTlPM0jRW/QAAAApJREFUeJxjYgAAAAYAAzY3fKgAAAAASUVORK5CYII=";

Now imagine that you got the image from input[type=file].files[0]. Assuming you are using async, you can grab an image the user has selected and display it on the page in less than 5 lines of code.

@richard-uk1
Copy link
Contributor Author

ping

@Pauan
Copy link
Contributor

Pauan commented Feb 1, 2020

Sorry for the delay on this, I'll get to it soon!

Copy link
Contributor

@Pauan Pauan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this, I've been busy with other things.

Overall it looks great, I'm impressed! There's just a bunch of small little nits to fix.

@richard-uk1
Copy link
Contributor Author

Ok I think I've addressed all the issues from the review, so it should be ready for review again.

Copy link
Contributor

@Pauan Pauan left a comment

Choose a reason for hiding this comment

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

Just a few more small things and this will be ready to land!

@richard-uk1
Copy link
Contributor Author

Ready for review again.

I've experimented with changing the last_modified type to SystemTime, since a) it seems more semantically correct, and b) it allows times before the epoch. Just for your consideration, happy to change it back if you prefer.

@Pauan
Copy link
Contributor

Pauan commented Feb 17, 2020

@derekdreery I'm torn. On the one hand, it does seem semantically correct. On the other hand, the last modified time must always be after the Unix Epoch, never before. I guess some runtime checking is acceptable.

@Pauan
Copy link
Contributor

Pauan commented Feb 17, 2020

Nevermind, I just checked and times before the epoch are valid, so SystemTime seems definitely correct.

Copy link
Contributor

@Pauan Pauan left a comment

Choose a reason for hiding this comment

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

Just one more small nit.

@richard-uk1
Copy link
Contributor Author

Would you like me to squash changes down to a single commit?

@Pauan
Copy link
Contributor

Pauan commented Feb 18, 2020

@derekdreery I don't care much either way, so it's up to you.

Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Richard Dodd <[email protected]>
@richard-uk1
Copy link
Contributor Author

I squashed :)

@Pauan Pauan merged commit ce79166 into rustwasm:master Feb 18, 2020
@Pauan
Copy link
Contributor

Pauan commented Feb 18, 2020

Thanks a lot! I'm really happy with how this turned out.

@richard-uk1
Copy link
Contributor Author

woop woop :)

@richard-uk1 richard-uk1 deleted the files-implementation branch February 18, 2020 16:35
@richard-uk1
Copy link
Contributor Author

I wonder if @alexcrichton would mind commenting on the use of SystemTime for the last_modified_time return type, since if I recall correctly he wrote the std::time RFC.

@alexcrichton
Copy link
Contributor

I haven't really reviewed this much, but I would agree that using libstd types where possible is the best path to take.

@richard-uk1
Copy link
Contributor Author

richard-uk1 commented Feb 18, 2020

@alexcrichton really I was just asking if it seems reasonable to use SystemTime in wasm32-unknown-unknown, or not. Sounds like you think it's fine. Thanks for commenting. :)

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.

File API Library
4 participants