0.2.88 implements TryFrom on all ABI-bound objects, breaking API bindings that have custom TryFrom implementations

[aspect]

0.2.88 automatically derives TryFrom trait for any struct that uses #[wasm_bindgen].

This prevents any developer from creating custom TryFrom<JsValue> conversions for the ABI-bound struct.

We have a rather large Rust/JS API that uses TryFrom to provide custom processing for JS objects. For example, JS code can pass a Rust struct, a buffer, or a hex string, which the TryFrom resolves. This is extremely useful because JS objects can be of different types. The object received from JS does not need to be an ABI-bound object in order to be converted to a native Rust struct.

This addition makes it no longer possible to create custom TryFrom handling between JsValue and a Rust struct that is also WASM ABI-bound.

I believe that such imposition should not be present in the attribute macro, or it should be possibly gated by a feature or a macro attribute. It does not seem quite right that an attribute macro creates functionality that is typically created by derive macros.

This is semantically not different than (for example) wasm_bindgen implementing a custom Clone trait, preventing the developer from being able to implement custom Clone functionality for his objects...

Perhaps there should be a FromJsValue derive macro available? Or such conversion should be moved into a custom try_from_js_abi() function implemented on each ABI-bound object? (implementing a function will create much less of a potential interference with underlying frameworks).

[biryukovmaxim]

+1 it should be gated by feature or attribute

[daxpedda]

I think the solution here is to create a new (internal?) trait or function instead of using TryFrom.
WDYT @Liamolucko?

But I agree this is an issue and we should fix that.

[Liamolucko]

0.2.88 is already out, so it'd be a breaking change to remove the automatic TryFrom impls now. It should be possible to add something like #[wasm_bindgen(manual_tryfrom)] to disable it though.

I think the solution here is to create a new (internal?) trait or function instead of using TryFrom.

Yeah, an internal trait is the way to go if we want Vecs of Rust structs to keep working when TryFrom is disabled. Although making it public might make sense, since otherwise there'll be no way to retrieve a Rust struct from a JsValue when TryFrom is disabled.

[aspect]

It should be an opt-in as opposed to opt-out. Or gated by a feature, disabling all related functionality.

The breaking changes were introduced by 0.2.88 to like 15+ published downstream crates (at least on our end across different projects).

For access you can impl a custom fn on the struct like ref_from_abi(JsValue) -> Option<&..>

[aspect]

Just for reference, we have things like this:

https://github.com/kaspanet/rusty-kaspa/blob/master/consensus/wasm/src/keypair.rs#L239

https://github.com/kaspanet/rusty-kaspa/blob/master/consensus/wasm/src/transaction.rs#L190

---

https://github.com/workflow-rs/workflow-rs/blob/master/wasm/src/channel.rs#L163

https://github.com/workflow-rs/workflow-rs/blob/master/wasm/src/abi.rs

[ranile]

0.2.88 is already out, so it'd be a breaking change to remove the automatic TryFrom impls now.

We could yank 0.2.88. As I understand it, the automatic TryFrom impls are an unintended breaking change so they should be fixed. I don't have permissions to yank anything though so I can't help here.

[bouk]

I think the nicest thing would be to create a TryFromJsValue derive macro.

And I don't think we should yank 0.2.88, but release it as a version 0.2.89

[daxpedda]

0.2.88 is already out, so it'd be a breaking change to remove the automatic TryFrom impls now. It should be possible to add something like #[wasm_bindgen(manual_tryfrom)] to disable it though.

I do believe that adding the TryFrom implementation was a breaking change already.

We could yank 0.2.88.

That would be the solution indeed.

That said, I'm not sure if all this is really necessary. I don't get the impression that this has caused a widespread issue, though considering the lack of data I might also be totally wrong on that.

---

If somebody pushes this to happen and makes a PR fixing this, by implementing a new trait like the TryFromJsValue proposed above, I would be in favor of merging that, releasing a new version and yanking the current one.

Let me know what everybody thinks about that.

[biryukovmaxim]

If somebody pushes this to happen and makes a PR fixing this, by implementing a new trait like the TryFromJsValue proposed above,

I will do it