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

Support for RefFromWasmAbi and RefMutFromWasmAbi #3

Open
Alfred-Mountfield opened this issue Aug 18, 2022 · 7 comments · May be fixed by #30
Open

Support for RefFromWasmAbi and RefMutFromWasmAbi #3

Alfred-Mountfield opened this issue Aug 18, 2022 · 7 comments · May be fixed by #30

Comments

@Alfred-Mountfield
Copy link

I've just tried out this crate and the typescript interfaces it's creating are awesome. Unfortunately I don't seem to be able to use it in methods like

#[wasm_bindgen]
modify_some_object(my_wasm_obj: &mut MyWasmObj); 

as tsify(into_wasm_abi, from_wasm_abi) doesn't implement support for RefFromWasmAbi and RefMutFromWasmAbi

Is there any plan to support deriving impl's for these?

@madonoharu
Copy link
Owner

It is possible to implement RefFromWasmAbi and RefMutFromWasmAbi, but I think this expression is misleading.
Currently, tsify(into_wasm_abi, from_wasm_abi) merely converts to JSON at a cost.

What do you think?

@Alfred-Mountfield
Copy link
Author

I could see the concern about hiding the cost, and I think you're probably right that this isn't worth it. I was still learning a fair bit about wasm_bindgen as I opened this issue, and I was running into some strange behaviour where passing by value would cause null-pointer errors in JavaScript due to move mechanics (I assume). As such I thought it was pretty necessary to get the Ref* traits as passing by reference avoided that issue.

I also was reading something about how passing by value can cause issues with garbage collection if things are build with --weak-refs. I'm not sure if that would apply here though because as you say, the impl wraps with a JSON conversion.

I could still see a use for the RefMut impl, but I'm not entirely sure what the actual implementation would look like. The main problem I've found is that you can't create a function (from what I can tell) that modifies a JS object using this library. I think this should be possible with the js_sys methods while still leaving the memory inside JavaScript and not needing to take it into wasm-land.

@madonoharu
Copy link
Owner

As a premise, I do not have a good understanding of wasm_bindgen.
And my English skill is poor.
Therefore, please forgive me if I am wrong.

The Tsify trait implemented by derive(Tsify) holds the type Tsify::JsType.
This type is just a duck-type of wasm_bindgen.
This duck-type generated by wasm_bindgen implements IntoWasmAbi, FromWasmAbi and RefFromWasmAbi.

Therefore, this description is possible.

#[derive(Tsify)]
pub struct MyWasmObj;

#[wasm_bindgen]
pub fn modify_some_object(my_wasm_obj: &<MyWasmObj as Tsify>::JsType) {
    js_sys::Reflect::set(my_wasm_obj, &"property_key".into(), &"value".into()).unwrap();
}

In this case, of course, no json conversion occurs and not even MyWasmObj is used.

It may not be impossible to combine this behavior with RefFromWasmAbi for advanced abstraction, but I think #[wasm_bindgen] pub struct Foo is much easier than that.

@Alfred-Mountfield
Copy link
Author

You make a good point, I'm not sure it'd be worth the attempt to create that abstraction, it might hide a lot of complexity.

#[wasm_bindgen] 
pub struct Foo {
...

means that the memory of the object lives within WASM and you can access it via methods. This is mostly fine for things like classes or things with functionality, but I'm actually using tsify because I specifically want plain JS objects (no methods or anything). wasm_bindgen generated classes don't play particularly nicely (to my knowledge) with things like normal JSON.stringify methods (although there are some work arounds).

So it would have been nice if there was an easy way to get plain (but typed) JS objects, that you can also easily mutate from Rust. Tsify seems to have gotten me most of the way there :)

@madonoharu
Copy link
Owner

madonoharu commented Aug 30, 2022

Come to think of it, implementing RefFromWasmAbi may not be so problematic because of the lifetime limitation.

It seems to me that RefMutFromWasmAbi can be implemented by using Object.assign only for Object.
However, I do not know if this is the correct and safe way to do this...

struct TsifyRefMut<T: Serialize + DeserializeOwned> {
    js: ManuallyDrop<JsValue>,
    owner: T,
}

impl<T: Serialize + DeserializeOwned> TsifyRefMut<T> {
    unsafe fn new(js: <JsValue as RefFromWasmAbi>::Abi) -> Self {
        let js = JsValue::ref_from_abi(js);
        let owner: T = js.into_serde().unwrap();
        Self { js, owner }
    }
}

impl<T: Serialize + DeserializeOwned> Drop for TsifyRefMut<T> {
    fn drop(&mut self) {
        let target = js_sys::Object::try_from(&self.js).unwrap();
        let output = JsValue::from_serde(&self.owner).unwrap();
        let source = js_sys::Object::try_from(&output).unwrap();
        js_sys::Object::assign(target, source);
    }
}

impl RefMutFromWasmAbi for Foo {
    type Abi = <JsValue as RefFromWasmAbi>::Abi;
    type Anchor = TsifyRefMut<Self>;
    unsafe fn ref_mut_from_abi(js: Self::Abi) -> Self::Anchor {
        TsifyRefMut::new(js)
    }
}

impl<T: Serialize + DeserializeOwned> Deref for TsifyRefMut<T> {
    ...
}

impl<T: Serialize + DeserializeOwned> DerefMut for TsifyRefMut<T> {
    ...
}

@Alfred-Mountfield
Copy link
Author

I'm not sure if I have the appropriate knowledge here to comment on if it's safe or not :(

Reading through the code does make sense to me though.

@cwfitzgerald
Copy link

This definitely should be possible to implement soundly because of that lifetime limitation.

While yes there wouldn't be any benefit to calling the & version on wasm, there basically never is. However if you're calling the same function from rust in any situation, being able to declare them with a reference would be very useful.

@cwfitzgerald cwfitzgerald linked a pull request Jul 20, 2023 that will close this issue
carlsverre pushed a commit to carlsverre/tsify that referenced this issue Jan 23, 2024
update is_js_ident to cover most cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants