Replace Result<T, JsValue> with Result<T, Error>

[Pauan]

Motivation

As discussed in #1735, it's not idiomatic to throw JS values, like throw "foo", because that loses stack traces.

Instead it's highly recommended to always use throw new Error("foo"). In wasm-bindgen, this corresponds to Err(js_sys::Error::new("foo"))

Internally, wasm-bindgen itself follows this recommendation, however, because the wasm-bindgen (and js-sys and web-sys) APIs return Result<T, JsValue>, this encourages user functions to return Result<T, JsValue> as well, which makes it easy for users to return non-Error values:

fn foo() -> Result<(), JsValue> {
    call_some_wasm_bindgen_api()?;
    Err(JsValue::from("hi!"))
}

Proposed Solution

All wasm-bindgen APIs which return Result<T, JsValue> should instead return Result<T, Error>.

This includes wasm_bindgen(catch) (which should have a debug_assert to verify that it actually is an Error object)

This uses the static type system to make it impossible to accidentally throw non-Error types. It also is more "Rust-like", which makes the APIs easier to understand.

This is obviously a very large breaking change.

Alternatives

• Do nothing. This is a lot less work, and isn't a breaking change, but it does mean that wasm-bindgen is encouraging bad behavior.

• Keep Result<T, JsValue>, but put in runtime checks which verify that the Err is a js_sys::Error. This is also a lot less work, but it does have a small runtime cost, and it's surprising behavior for the user.

[alexcrichton]

This is somewhat similar to #1017 and #1004 as well I think. I'd love to start immediately making progress on this if we can instead of delaying it to a batch of breaking changes though, that'd help us get our feet wet and experiment with with possible ideas before we set it all in stone.

One thing we might want to do to start out is to support returning custom error types, converting them with Display to a new Error(rust_error.to_string()) (basically). Next it might be prudent to go ahead and start warning about Err(JsValue::from("hi")) in some capacity, or maybe even moving Error into wasm-bindgen the crate itself to make it more easily available

[Pauan]

One thing we might want to do to start out is to support returning custom error types, converting them with Display to a new Error(rust_error.to_string()) (basically).

That will have bad stack traces though. Because of the way errors work in JS, you basically always want to create the Error object at the specific place where the error occurred.

However, a custom error type which contains a js_sys::Error is probably a good idea. We can have macros to make it easy to generate them.

Stdweb already does this extensively. It has attributes and macros to generate the error types, and it uses them in the APIs.

---

For web-sys, we should add in a From<web_sys::DomException> for js_sys::Error impl (exact implementation to be decided), and all of the web-sys APIs would error with DomException instead of JsValue.

Ideally we would want more fine-grained errors for web-sys, like this:

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(
        extends = DomException,
        extends = Error,
        instanceof = InvalidCharacterError::instanceof
    )]
    type InvalidCharacterError;
}

impl InvalidCharacterError {
    fn instanceof(val: &JsValue) -> bool {
        if let Some(exception) = val.dyn_ref::<DomException>() {
            exception.name() == "InvalidCharacterError"

        } else {
            false
        }
    }
}

But the problem is that WebIDL doesn't specify which errors each API throws, so we would need to manually add those in. And it may not even be worth it, since DomException is probably good enough.

[Pauan]

Oh, I think I understand what you mean about custom error types. You mean something like this:

impl<E> From<E> for js_sys::Error where E: std::error::Error {
    #[inline]
    fn from(value: E) -> js_sys::Error {
        js_sys::Error::new(&value.to_string())
    }
}

And then it would be possible to use ? inside of a function and it would auto-convert the custom error into a js_sys::Error.

I think that's a good idea. The stack traces might not always be 100% ideal, but they'll be pretty good.

[alexcrichton]

Yes I don't think that capturing stack traces with the higest of fidelity is that important. Additionally while changing all of web-sys might be nice I think it's fine to leave that for later. For now I'd just like to explore avenues to unlock more functionality today rather than waiting on a bunch of changes to happen "eventually"

[cormacrelf]

Not sure if it's just me, but it doesn't seem like it's currently possible to construct a js_sys::Error without the glue code catching the error you're trying to create. (Bit weird, catch shouldn't catch an Error when it's just a return value from a new Error() call.) When I return Err(Error::new("...")), I just get this in the console:

wasm-bindgen: imported JS function that was not marked as `catch` threw an error: my custom error message here

Stack:
__wbg_new_2e9dc2ca6bd84218@webpack-internal:///../pkg/citeproc_wasm.js:680:17
__wbg_new_2e9dc2ca6bd84218@http://localhost:8081/index.js:104:75
js_sys::Error::new::h2f12b4de882df30f@http://localhost:8081/b0a7cc0755979b50ad35.module.wasm:wasm-function[11137]:0x2075a3
my_module::...

[alexcrichton]

@cormacrelf I think you may be running into a separate issue that's too eagerly attempting to print out that a function threw an error, mind opening up a new issue for that?

[That3Percent]

I like the idea of having some sort of impl<E> From<E> for js_sys::Error where E: std::error::Error {

Right now it's a papercut to try to use an error crate like Anyhow. Because of the orphan rule, a user crate can't implement From for JsValue as would be required for ergonomic error handling.