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

Display for Errors, return error from WASM #2

Merged
merged 3 commits into from
Feb 25, 2020

Conversation

ThomasdenH
Copy link
Contributor

This PR adds Display and Error impls for the errors, and returns a JsError with the formatted string to WASM. I have updated the documentation and demo to reflect this, and also fixed the warnings Clippy gave me.

@fpapado
Copy link
Owner

fpapado commented Feb 24, 2020

Neat, thank you @ThomasdenH!

I had a note to circle back for explicit errors on a branch, so this is very fitting! I really like thiserror
We might want to expose more information in the errors in the future, but I bet that can be in another PR.

I'll take a final look at this once I'm home in a few hours, and merge :)

(Aside: do you know, by any chance, if there was any resolution to rustwasm/wasm-bindgen#1017? It's what kept me from making the js_sys::Error conversion in the first place, but maybe that doesn't matter much.)

@ThomasdenH
Copy link
Contributor Author

I'm not sure there is a resolution to the issue. There is some additional discussion here: rustwasm/wasm-bindgen#1742, which gives the recommendation to use a JsError instead of throwing a string.

@fpapado
Copy link
Owner

fpapado commented Feb 25, 2020

Sounds good! I tried the demos and so on and they seemed to work.
Thank you again for this PR. I'll merge it now and get a release out later today.

@fpapado fpapado merged commit 2dfccd1 into fpapado:master Feb 25, 2020
@ThomasdenH
Copy link
Contributor Author

Great!

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.

2 participants