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

Create a JsError type #1017

Closed
fitzgen opened this issue Nov 8, 2018 · 11 comments
Closed

Create a JsError type #1017

fitzgen opened this issue Nov 8, 2018 · 11 comments
Labels
docs Issues related to documentation js-sys Issues related to the `js-sys` crate question

Comments

@fitzgen
Copy link
Member

fitzgen commented Nov 8, 2018

Something analogous to failure::Error maybe.

Easy conversion into a JS value (probably creates a js_sys::Error?)

Can be the E type in a Result<T, E> that is returned from an exported rust function.

Thoughts?

@fitzgen fitzgen added question js-sys Issues related to the `js-sys` crate docs Issues related to documentation labels Nov 8, 2018
@fitzgen
Copy link
Member Author

fitzgen commented Nov 8, 2018

And we should also add a reference page for "Dealing with fallible operations and errors" or some such.

@RReverser
Copy link
Member

I don't think it would be very efficient to create a js_sys::Error on Rust side, since it will end up adding a newly constructed object to the JS side, creating a reference to it, returning it to Rust side, and then Rust function will likely immediately return to the JS with that error reference, and JS will have to remove it back.

IMO it's easier to just allow arbitrary E: std::error::Error as a result, convert it using to_string() by default, return that in such JsError newtype and then construct new Error in the JS wrapper part of wasm-bindgen.

Then custom conversion will be still possible by providing explicit methods on JsError, but by default everything will be just returning human-readable string messages and constructing regular JS errors.

@alexcrichton
Copy link
Contributor

@RReverser hm a good point! I wonder though if perf matters too much in the failure case though? In theory it's rare-ish and it'll already involve a throw in JS as well as at least a copy of the message onto the JS heap, all of which are likely more expensive than a call or two between JS and wasm?

(would be good to measure though!)

@RReverser
Copy link
Member

Performance maybe not so much, but given that most exported functions commonly will (hopefully) be returning such Result, moving this interaction to the general wrapper might be a good saving in terms of size of repetitive code.

@Pauan
Copy link
Contributor

Pauan commented Dec 6, 2018

@RReverser There is a difference in behavior between the two approaches:

  • With JsError the stack trace is created at the time when the JsError is created.

  • Without JsError the stack trace is created at the time when the Result is passed into JS.

This causes a difference in the stack traces.

I think both of those approaches are useful, but I don't have an opinion about which one should be the default.

@RReverser
Copy link
Member

@Pauan I wasn't suggesting to remove JsError, so there shouldn't be a difference. My approach is merely an optimisation.

@RReverser
Copy link
Member

Oh... I see what you are saying I think. You mean that the JS stacktrace inside of WASM itself will look different. I agree, that would be unfortunate.

Let's just implement this naïvely first, and then try to optimise.

@Pauan
Copy link
Contributor

Pauan commented Dec 6, 2018

@RReverser Yeah, sorry I wasn't more clear. The stack trace is always generated at the time when new Error(...) is called (and the stack trace contains wasm functions).

So if you create a JsError in Rust, new Error(...) will be called immediately.

But if you instead don't use JsError in Rust, it will call new Error(...) when actually passing the Result to JS, so the stack trace will be at that point in time.

Like I said, both behaviors are useful, so I think both should be supported.

@lrlna
Copy link

lrlna commented Jan 8, 2019

Would love to have something to be able to work with errors properly in wasm! I currently have methods that returns a result with boxed error where i can't add the #[wasm-bindgen] tag. Instead i am currently wrapping my methods and convert the error to string and then to JsValue:

#[wasm_bindgen]
impl SchemaParser {
  #[wasm_bindgen(js_name = "write")]
  pub fn wasm_write(&mut self, json: &str) -> Result<(), JsValue> {

    // self.write is the function i would like to tag wasm_bindgen, but instead doing this:
    match self.write(json) {
      Err(e) => Err(JsValue::from_str(&format!("{}", e))),
      _ => Ok(()),
    }
  }
}

what would be have real nice to do is just to be able to tag something like this:

pub fn write(&mut self, json: &str) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {   
  unimplemented!()
}

@tonyhb
Copy link

tonyhb commented Jun 13, 2020

I'm also changing function signatures to -> Result<T, JsValue>. The problem here is that I'm also trying to cross-compile two targets: x86 and wasm-unknown-unknown.

All std:: functionality that can't be exposed to the web is hidden. But shared methods still need to return a boxed error. JsValue panics on non-wasm targets, so error handling is - frankly - really annoying.

Would be great to change this with something that's supported on both platforms, using the wasm ABI to transmit errors on the heap?

Sayan751 added a commit to Sayan751/email-address-parser that referenced this issue Sep 1, 2020
The local part and the domain are validated on EmailAddress::new.
The return type is changed to Option<EmailAddress>. Ideally it
should have been `Result<EmailAddress, std::error::Error>`. However,
unfortunately it does not seems to be working with wasm_bindgen;
refer: rustwasm/wasm-bindgen#1017.
@Liamolucko
Copy link
Collaborator

I think this was fixed by #2710.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues related to documentation js-sys Issues related to the `js-sys` crate question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants