Skip to content

Replace Blob::array_buffer with FileReader #471

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 7 commits into from
May 28, 2020

Conversation

tkubicz
Copy link
Contributor

@tkubicz tkubicz commented May 27, 2020

This PR replaces the Blob:array_buffer usage with the FileReader. This change fixes #470.

This is my first PR to seed, and the first time that I had anything to do with wasm_bindgen so I would really appreciate a review.

@MartinKavik
Copy link
Member

Thanks for the PR!

I was looking at it and it looks good, I would add only a few comments.
However I found this code that directed me to function read_as_bytes. Seed's WebSocket wrapper already uses gloo_file::Blob so I can naively imagine that we can just write read_as_byte(&blog).await and done - but I don't know if there are some methods that don't work on Safari, you have better knowledge about it than me now => in that case we would return back to your implementation.

Also we should add unit test or perhaps ideally extend WebSocket example in the Seed repo to use also methodbytes - would you like to do it? I think you are more experienced than me with byte WebSocket messages so I would be glad.

@MartinKavik
Copy link
Member

Note: The latest wasm-bindgen supports Blob.stream (see README), but it isn't supported by all browsers, too.

@tkubicz
Copy link
Contributor Author

tkubicz commented May 28, 2020

HI Martin,

You were right, using read_as_bytes from gloo is much better approach and works just fine on every browser tested by me (Chrome, Firefox, Safari and Opera). I have also extended the web socket example to show the usage of binary messaging.

I'm having trouble with implementing tests for this function, because I have no idea how to convert js_sys::Uint8Array to JsValue and create a blob directly in rust.

@MartinKavik
Copy link
Member

Nice, thanks! I'll check how it works on Windows and try to do some quick review.

I have no idea how to convert js_sys::Uint8Array to JsValue and create a blob directly in rust.

Does AsRef<JsValue> for Uint8Array or From<Uint8Array> for JsValue help?
gloo_file has Blob::new and Blob::new_with_option. I assume that gloo's File and native blobs and files has similar methods. Also there are From implementation for casting among all types.

Copy link
Member

@MartinKavik MartinKavik left a comment

Choose a reason for hiding this comment

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

Code looks good, works good :) I've added some short comments. And please update CHANGELOG.md.

@@ -50,6 +51,7 @@ pub enum WebSocketError {
SendError(JsValue),
SerdeError(serde_json::Error),
PromiseError(JsValue),
FileRedaerError(FileReadError),
Copy link
Member

Choose a reason for hiding this comment

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

Typo

@tkubicz
Copy link
Contributor Author

tkubicz commented May 28, 2020

Ok, I've corrected the typo, added changelog entry and unit test to check if obtaining bytes really work.

@MartinKavik MartinKavik merged commit 3134d21 into seed-rs:master May 28, 2020
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.

WebsocketMessage.bytes() doesn't work on Safari
2 participants