Closed Billy-Sheppard closed 1 year ago
I'm fine with the examples using unwrap
, but the actual library code should use unwrap_throw
, because that reduces file size for consumers.
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.
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 futures-util
, which uses unwrap, so there is no extra file size cost for consumers, but perhaps I don't quite understand it right.
Hi @Pauan and @Billy-Sheppard - so I think the logic here is that unwrap_throw()
is primarily about reducing code size, however this can only have an effect if the application and all of its dependencies don't use regular unwrap
or equivalent on any code paths anywhere. I think this is a pretty tricky ask for any application using dependencies however it could be quite valuable for std-only or no-std work. From what I can see, dominator
depends on futures-util
which uses unwrap
or equivalent at least here: https://github.com/rust-lang/futures-rs/blob/dd019055ef5bf4309f15db934407e202caf52e14/futures-util/src/lock/mutex.rs#L215 but also in a number of other places. This means that any code that depends on futures-util
will bring through all the panic infrastructure anyway and hence switching dominator
to use unwrap
instead of unwrap_throw
gives an ergonomics improvement without causing any extra code to be included
@Billy-Sheppard Potentially the function can be updated to ignore empty strings rather than fail, I'm happy to look into that as a separate PR.
Or it can use expect
to give a nicer error message. I think that's reasonable for user-facing APIs.
@esheppa so I think the logic here is that unwrap_throw() is primarily about reducing code size, however this can only have an effect if the application and all of its dependencies don't use regular unwrap or equivalent on any code paths anywhere.
My understanding is that the bloat is per unwrap
. So less unwrap
will mean smaller file size. Of course ideally there wouldn't be any unwrap
at all, however less unwrap
should still be smaller file size.
The panic infrastructure will always be there, because even something as simple as some_slice[0]
can panic. The point of using unwrap_throw
is to avoid the bloated strings. Every unwrap
call will call Debug
and then create a run-time string, that extra Debug
code (and runtime string) is avoided with unwrap_throw
.
There's also some other interactions at play here: unwrap_throw
is specifically designed for JS, so it uses the JS-exception mechanism, whereas unwrap
is pure-Rust, so it uses Wasm unreachable instead. I would prefer to use the JS-specific unwrap_throw
as much as possible.
My understanding is that the bloat is per unwrap. So less unwrap will mean smaller file size. Of course ideally there wouldn't be any unwrap at all, however less unwrap should still be smaller file size.
Thanks for this - that makes sense as to why libraries would still use unwrap_throw
where possible. In this case it looks like the best option might be some sort of feature that turns unwrap_throw
(or equivalent) into a regular unwrap
with the format strings (eg cfg(debug_assertions)
or feature = "fancy-debugging"
). This means that when compiling in release or without the feature, users can get a smaller wasm package, but while trying to debug an error it is possible to get a more ergonomic message. This would probably be in wasm-bindgen
rather than here
edit: @Pauan - just saw your comment on the wasm-bindgen
PR - full unwrap strings on debug makes the most intuitive sense as you've suggested
@esheppa Yup, I'm replacing some of the unwrap_throw
with more useful error messages, so that should fix this issue.
I'll close this PR now, will investigate potentially allowing MultiStr
/class() etc, to handle empty strings separately.
I changed the way that errors are handled, now the error messages are much better and more intelligent:
unwrap_throw
(this has basically no performance or code size cost).panic!
with proper filename, line number, and column. This isn't perfect, because #[track_caller]
isn't currently supported in closures.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.