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

Downcast doesn't play nicely with minification #51

Closed
kegsay opened this issue Oct 31, 2023 · 10 comments · Fixed by #82
Closed

Downcast doesn't play nicely with minification #51

kegsay opened this issue Oct 31, 2023 · 10 comments · Fixed by #82
Assignees
Labels
bug Something isn't working

Comments

@kegsay
Copy link
Member

kegsay commented Oct 31, 2023

This function looks at constructor names to try to determine the right types. Specifically:

let constructor_name = Object::get_prototype_of(value).constructor().name();

This may return an unexpected constructor name when minification has been used, as classes are renamed to shorter strings e.g UserId becomes NG. This can then cause confusing errors:

Error: Expect an `UserId` instance, received `Ng` instead
@kegsay kegsay added the bug Something isn't working label Oct 31, 2023
@Hywan
Copy link
Member

Hywan commented Nov 2, 2023

With rustwasm/wasm-bindgen#3554, it might no longer be necessary to play with downcast here, as it's primarily used for supporting Vec<T> as far as I know.

@Hywan
Copy link
Member

Hywan commented Nov 2, 2023

A new release of wasm-bindgen has been made yesterday, perfect (0.2.88).

@richvdh
Copy link
Member

richvdh commented Nov 25, 2023

A new release of wasm-bindgen has been made yesterday, perfect (0.2.88).

Yes, though given we have custom impl From<X> for JsValue implementations for various of our types, it seems to cause damage for us in much the same way as rustwasm/wasm-bindgen#3685.

@richvdh
Copy link
Member

richvdh commented Dec 7, 2023

This is going to be a blocker for full release of Element R, because normal builds use minified source.

@Hywan
Copy link
Member

Hywan commented Dec 13, 2023

Do you need my help here?

@richvdh
Copy link
Member

richvdh commented Dec 22, 2023

Do you need my help here?

Any help would be much appreciated.

At the moment, my ideas are:

  • Get rid of the check on the constructor name altogether, and just assume that the application side has passed an object of the right type. Obviously, this will break horribly if they do not.
  • If we could somehow get hold of a JsValue holding an instance of an object known to be of the right type, then rather than hard-coding the constructor name, we could compare the two constructors. But we wouldn't want to create an object on the JS side for every call, so I don't think this works.

@richvdh
Copy link
Member

richvdh commented Jan 8, 2024

Just for the record: We could have replaced downcast with wasm_bindgen::convert::TryFromJsValue (as introduced in rustwasm/wasm-bindgen#3709), but:

(a) that's supposedly internal;

(b) we don't need downcast at all since wasm-bindgen now supports properly-typed Vec<...> parameters that will check the values in the array at runtime (added in rustwasm/wasm-bindgen#3554, as modified by rustwasm/wasm-bindgen#3709)

@richvdh
Copy link
Member

richvdh commented Jan 8, 2024

Just for the record: We could have replaced downcast with wasm_bindgen::convert::TryFromJsValue (as introduced in rustwasm/wasm-bindgen#3709)

oh, in fact @Hywan's PR does exactly that in this instance: c556316#diff-dd97281cbaa3b13ed5a8dc4417797959632eed864b774977e013fc307dcc9f01L1002-R1012

@Hywan Hywan self-assigned this Jan 8, 2024
@MTRNord
Copy link
Contributor

MTRNord commented Jan 11, 2024

There is also matrix-org/matrix-rust-sdk@main...MTRNord:matrix-rust-sdk:MTRNord/fix-binding-reflection which is based on wasmer code. However, afaik the repo for wasm-bindgen-downcast is still not public but the source on crates has a FOSS license as seen here: https://docs.rs/crate/wasm-bindgen-downcast/0.1.1/source/README.md

The missing repo is at https://github.com/wasmerio/wasm-bindgen-downcast

Thats how https://github.com/MTRNord/cetirizine is deployed

(Also yes my fork is dated ^^ but I hope it gets the point across still)

@Hywan
Copy link
Member

Hywan commented Jan 11, 2024

Thank you but we don't need an extra dependency. wasm-bindgen provides all the features we need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants