Replace TryFrom with custom TryFromJsValue trait

[biryukovmaxim]

This commit replaces the usage of the standard library's TryFrom trait with a custom TryFromJsValue trait added for handling the conversion from JavaScript values in WASM. This trait has been implemented for different types across multiple modules to ensure consistent error handling on failed conversions.

fixes #3685

[daxpedda]

Could you add an entry to the changelog as well?
LGTM otherwise.

[daxpedda]

@hamza1311 and @Liamolucko, the idea here is to yank the current version and release a new one with this change.
Myself I'm not entirely fond of the whole idea, but I believe if somebody is affected and willing to provide a fix as well, we should take that as something that needs to be addressed. So I would vote in favor.

Let me know what you think.

[Liamolucko]

Myself I'm not entirely fond of the whole idea, but I believe if somebody is affected and willing to provide a fix as well, we should take that as something that needs to be addressed. So I would vote in favor.

I feel the same way.

This does un-fix #2231 though, since wasm_bindgen::convert is an internal module and so TryFromJsValue is effectively private.

The easy solution is just to move it to wasm_bindgen::TryFromJsValue, but it's a bit weird to make people use a custom trait rather than the standard TryFrom<JsValue>. Alternatively we could make a TryFromJsValue derive macro, but that's a lot more work.

[biryukovmaxim]

This does un-fix #2231 though, since wasm_bindgen::convert is an internal module and so TryFromJsValue is effectively private.

convert module is public, it already exports From/IntoWasmAbi, and others. But these two look like custom From and Into which include additional constraints.

[Liamolucko]

convert module is public, it already exports From/IntoWasmAbi, and others. But these two looks like custom From and Into that include additional constraints.

Yes, it's technically public, but see its documentation:

This is mostly an internal module, no stability guarantees are provided. Use at your own risk.

It's only really public so that macros can access it, nobody should be using anything from in there directly.

[biryukovmaxim]

This is mostly an internal module, no stability guarantees are provided. Use at your own risk.

It's only really public so that macros can access it, nobody should be using anything from in there directly.

Oh. I see. Your suggestion is to move the trait to wasm_bindgen - root module?

[biryukovmaxim]

Could you add an entry to the changelog as well? LGTM otherwise.

done

[daxpedda]

Alternatively we could make a TryFromJsValue derive macro, but that's a lot more work.

Even if we want to do that one day, it would still require TryFromJsValue trait to be public.
I agree though this is weird, but I can't come up with a better solution.

[daxpedda]

I'm on the fence about moving TryFromJsValue just back to convert and just figure out what to do here another time.
Any input would be appreciated.

[daxpedda]

LGTM, just some nits about docs.

As mentioned before, this unfixes #2231, but we can figure out exactly what to do here at some later point.

@biryukovmaxim you should probably hold off on any major changes for now, we are still discussing how exactly to proceed, there's no need to move TryFromJsValue back and forth upon every whimsical comment I make! (don't move it back now 😄)

[daxpedda]

I'm happy to move forward with this, we can figure out how to fix #2231 again in the future.

[dani-garcia]

I've hit a problem with the TryFrom changes that is unrelated to #3685, but that would probably also occur with this PR, related to the ambiguity of using Self::Error when #[wasm_bindgen] is applied to an enum containing an Error variant.

#[wasm_bindgen]
pub enum LogLevel {
    Trace,
    Debug,
    Info,
    Warn,
    Error,
}

The way to solve that would be to use the fully qualified syntax for Self:

fn try_from_js_value(value: #wasm_bindgen::JsValue)
-                   -> #wasm_bindgen::__rt::std::result::Result<Self, Self::Error> {
+                   -> #wasm_bindgen::__rt::std::result::Result<Self, <Self as TryFromJsValue>::Error> {

I can open a PR myself after this one is merged if you prefer to merge this one as-is though.

[daxpedda]

Could you open a PR targeting main? We can merge that right now and let this PR rebase.
Thank you for bringing this up!

[Liamolucko]

I've hit a problem with the TryFrom changes that is unrelated to #3685, but that would probably also occur with this PR, related to the ambiguity of using Self::Error when #[wasm_bindgen] is applied to an enum containing an Error variant.

#[wasm_bindgen]
pub enum LogLevel {
    Trace,
    Debug,
    Info,
    Warn,
    Error,
}

The way to solve that would be to use the fully qualified syntax for Self:

fn try_from_js_value(value: #wasm_bindgen::JsValue)
-                   -> #wasm_bindgen::__rt::std::result::Result<Self, Self::Error> {
+                   -> #wasm_bindgen::__rt::std::result::Result<Self, <Self as TryFromJsValue>::Error> {

I can open a PR myself after this one is merged if you prefer to merge this one as-is though.

I think this was already fixed in #3678, can you check if it still happens using a git version of the wasm-bindgen macro?

[dani-garcia]

My bad, seems this was fixed on master a bit ago, and I was looking at the struct changes of this PR instead of the enum changes. I just tested both master and this PR and they both work as expected, sorry for the confusion!

[Liamolucko]

I don't like regressing on #2231, but I guess the breaking changes are a higher priority.

[Liamolucko]

Alternatively we could make a TryFromJsValue derive macro, but that's a lot more work.

Even if we want to do that one day, it would still require TryFromJsValue trait to be public. I agree though this is weird, but I can't come up with a better solution.

By the way, what I meant by a TryFromJsValue derive macro was a derive macro named TryFromJsValue that implements TryFrom<JsValue>. I guess that is a bit confusing when there's also an internal trait with the same name, but the internal one could be renamed to TryFromJsValueInternal or something to avoid that (not that that needs to happen in this PR).

[daxpedda]

I'm gonna go ahead and merge this now and get the new release going.