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

Bump wasm_bindgen + fixes for JsError #98

Merged
merged 1 commit into from
Aug 10, 2022
Merged

Conversation

rooooooooob
Copy link
Contributor

During #81 utils.rs was refactored into many places and some wouldn't
be using error::JsError which in wasm builds is a typedef for
js_sys::JsValue and is its own struct for rust builds (to avoid panics
in JsValue when used from non-wasm builds).

Howeve, some places couldn't see these types and would pull it in
directly from wasm_bindgen::prelude from the wildcare import in
lib.rs. This caused an issue when bumping wasm_bindgen as the
from_str() method is no longer in js_sys::JsError but still is in
JsValue. Between now and #81 this would have also led to panics if
many places when an error was returned when using CML from rust instead
of wasm.

In the future we should deprecate the direct usage of js_sys::JsValue
and remove our JsError type on rust in favor for real rust errors (not
just string wrappers).

See rustwasm/wasm-bindgen#2710

Expands the acceptable return-position types to Result<T, E> where T: IntoWasmAbi, E: Into, so you can easily throw your own custom error classes imported from JS

which will greatly improve error handling from the rust side but this
should be done after the rust/wasm split.

During #81 `utils.rs` was refactored into many places and some wouldn't
be using `error::JsError` which in wasm builds is a typedef for
`js_sys::JsValue` and is its own struct for rust builds (to avoid panics
in JsValue when used from non-wasm builds).

Howeve, some places couldn't see these types and would pull it in
directly from `wasm_bindgen::prelude` from the wildcare import in
`lib.rs`. This caused an issue when bumping `wasm_bindgen` as the
`from_str()` method is no longer in `js_sys::JsError` but still is in
`JsValue`. Between now and #81 this would have also led to panics if
many places when an error was returned when using CML from rust instead
of wasm.

In the future we should deprecate the direct usage of `js_sys::JsValue`
and remove our `JsError` type on rust in favor for real rust errors (not
just string wrappers).

See rustwasm/wasm-bindgen#2710

>Expands the acceptable return-position types to Result<T, E> where T: IntoWasmAbi, E: Into<JsValue>, so you can easily throw your own custom error classes imported from JS

which will greatly improve error handling from the rust side but this
should be done after the rust/wasm split.
@joacohoyos
Copy link
Contributor

@rooooooooob this seems to fix the memory leak issue. The only issue is with flowgen.

Removing it from the build scripts seems to make it work. If we remove that it seems to be working fine
image

@rooooooooob
Copy link
Contributor Author

Yeah I saw that flowgen thing but it seemed unrelated to any change I made so I assumed the issue existed in develop already although I didn't check. I just wanted to get this PR up first for testing and to see if it resolved the issue. I noticed that building also deleted the .flow types file (I didn't commit the removal since it seemed unrelated) but I am also not sure why it happened. That seems more like it'd be related to my installed wasm-pack version than anything here but I guess it's possible it's related to the version bump here too. I'll have to try and sort this out after I finish a different task I'm working on unless anyone else wants to look into it and needs it done in the next few days.

IIRC we might end up deprecating flow at some point anyway and just provide typescript bindings. CC: @SebastienGllmt

@SebastienGllmt SebastienGllmt merged commit 9e04c2d into develop Aug 10, 2022
@SebastienGllmt SebastienGllmt deleted the wasm-bump-2.82 branch August 10, 2022 20:15
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 this pull request may close these issues.

3 participants