-
Notifications
You must be signed in to change notification settings - Fork 65
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
Changed to using unwrap()
#70
Conversation
I'm fine with the examples using Panics shouldn't be happening anyways, since they should only happen if there is a bug. Which panic are you getting in your code? P.S. Style changes should be done in a separate PR, otherwise it's too hard to review the changes. |
e505e9c
to
0877636
Compare
I've reverted the style changes. EDIT: Seems like some formatting changes still snuck in, I can fix this if required. Apologies. The error I was getting was I was passing an empty string to class, which caused an unwrap, a use case I knew caused an error but hadn't realised my code was producing an empty string. Potentially the function can be updated to ignore empty strings rather than fail, I'm happy to look into that as a separate PR. As far as I understand it, because you depend on |
Hi @Pauan and @Billy-Sheppard - so I think the logic here is that |
Or it can use
My understanding is that the bloat is per The panic infrastructure will always be there, because even something as simple as |
There's also some other interactions at play here: |
Thanks for this - that makes sense as to why libraries would still use edit: @Pauan - just saw your comment on the |
@esheppa Yup, I'm replacing some of the |
I'll close this PR now, will investigate potentially allowing |
I changed the way that errors are handled, now the error messages are much better and more intelligent:
The end result is that in release mode debugging is a bit better (at no extra cost), and in debug mode it's much better. This doesn't actually fix the "empty classname" problem, but that's really an error in the browser itself, not Rust. |
Changed usage of
unwrap_throw()
to useunwrap()
. Also fixed some small/minor clippy lints + cargo fmt.This allows for users to use
std::panic::set_hook(Box::new(console_error_panic_hook::hook));
, normally only in debug environments to see line numbers.wasm_bindgen
aren't keen to allow for unwrap_throw to reflect line/col/file references.After discussions had in the PR, it is clearer to me anyway, that downstream crates should use unwrap, especially if they depend on other crates that use unwrap.
futures-util
for instace: https://github.com/rust-lang/futures-rs/blob/dd019055ef5bf4309f15db934407e202caf52e14/futures-util/src/fns.rs#L340.