Changed to using unwrap()

[Billy-Sheppard]

Changed usage of unwrap_throw() to use unwrap(). 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.

[Pauan]

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.

[Billy-Sheppard]

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.

[esheppa]

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

[Pauan]

@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.

[Pauan]

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.

[esheppa]

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

[Pauan]

@esheppa Yup, I'm replacing some of the unwrap_throw with more useful error messages, so that should fix this issue.

[Billy-Sheppard]

I'll close this PR now, will investigate potentially allowing MultiStr/class() etc, to handle empty strings separately.

[Pauan]

I changed the way that errors are handled, now the error messages are much better and more intelligent:

• In release mode it will either throw the JS Error directly, or it will use unwrap_throw (this has basically no performance or code size cost).
• In debug mode it will use Rust's panic! with proper filename, line number, and column. This isn't perfect, because #[track_caller] isn't currently supported in closures.
• I added in more specific and helpful error messages for some things.

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.