-
Notifications
You must be signed in to change notification settings - Fork 60
Add Ts<T> wrapper type to avoid memory leaks
#71
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
base: main
Are you sure you want to change the base?
Conversation
|
Looks like there are some test failures, but it might not be due to this PR? Do the existing tests already cover the traits in this PR? |
|
There is absolutely nothing testing the new code yet. |
a8f617d to
04da1d2
Compare
|
Ok, new stuff. The most major changes are:
And most importantly
Review questions:
|
04da1d2 to
91f86de
Compare
|
I can also hold off on the deprecation commit until this has been tested a bit more in the wild. |
91f86de to
9dcb2c9
Compare
| #[wasm_bindgen] | ||
| pub fn from_js(point: Point) {} | ||
| pub fn from_js(point: Ts<Point>) -> Result<(), JsError> { | ||
| let point: Point = point.to_rust()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially called this .into_rust() but changed to .to_rust() when I made it take &self. But the other APIs (Tsify trait) do not follow this convention, so maybe this is more confusing and it would be better off called into_rust.
| use tsify::Tsify; | ||
| use tsify::Ts; | ||
| use wasm_bindgen::prelude::*; | ||
| use wasm_bindgen::JsError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note, I am actually the creator of JsError and the non-memory-leaking Result return-throw in wasm-bindgen :)
There seems to be no way to emit #[deprecated] warnings.
…asm_abi attributes
9dcb2c9 to
53acd1d
Compare
|
@cormacrelf Sorry, I lost track of this :-(. Is the PR ready to go? |
This is a solution for #65. Basically we can't do the conversion inside FromWasmAbi etc, so we just do it outside but make it as ergonomic as possible.
These produce exactly the same typescript outputs as before. But there are no memory leaks.
Still working on the extensions to the
Tsifytrait for ergonomic return values, int terms of naming. Happy to hear comments and concerns.