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

Version 0.4.0 makes JS code that breaks at load time on Safari 14 #28

Closed
gthb opened this issue Jan 5, 2022 · 5 comments · Fixed by rustwasm/wasm-bindgen#3037
Closed

Comments

@gthb
Copy link

gthb commented Jan 5, 2022

We just had to swiftly revert our dependency on serde-wasm-bindgen back to 0.3.1, as the introduction of BigInt support makes BigUint64Array and BigInt64Array be referenced at module scope in the *_bg.js wrapper, causing the module to fail to load in Safari 14 and earlier. Safari does not have these types until version 15.0 (released last Sep 20th, so there's still lots of Safari 14 out there).

This can break the whole web application (depending on one's bundling, I guess), even one that makes no use of this new BigInt support, in those browser versions.

So version 0.4 (and the README, if this stays as-is) should really carry a nice big warning about this effect on browser support.

@RReverser
Copy link
Owner

cc @mfish33

This is an interesting one, and a bit unfortunate. I'm surprised those are referenced at top-level scope, I guess we need some way to retrieve them lazily instead.

@mfish33
Copy link
Contributor

mfish33 commented Jan 12, 2022

@RReverser I think this is upstream from us. In the generated code by wasm bindgen these three lines are the culprit.

const u32CvtShim = new Uint32Array(2);
const int64CvtShim = new BigInt64Array(u32CvtShim.buffer);
const uint64CvtShim = new BigUint64Array(u32CvtShim.buffer);

Defined in:
https://github.com/rustwasm/wasm-bindgen/blob/ac87c8215bdd28d6aa0e12705996238a78227f8c/crates/cli-support/src/js/mod.rs#L1905
and
https://github.com/rustwasm/wasm-bindgen/blob/ac87c8215bdd28d6aa0e12705996238a78227f8c/crates/cli-support/src/js/binding.rs#L685

Further, it seems that this issue does not exactly have to do with Bigint types rather having rust bindings that receive u64/i64's as arguments or JS bindings that return u64/i64's

@RReverser
Copy link
Owner

Further, it seems that this issue does not exactly have to do with Bigint types rather having rust bindings that receive u64/i64's as arguments or JS bindings that return u64/i64's

Right I see, and we introduced those for our BigInt conversion bindings... I guess we need to either report this to wasm-bindgen and ask for those variables to be defined only lazily when u64/i64 is actually used, or we need to add a static feature in this crate as a way to opt-out.

Liamolucko added a commit to Liamolucko/wasm-bindgen that referenced this issue Aug 18, 2022
Previously, they were passed as a pair of `i32`s; this changes them to be passed as a single `i64`, which is directly converted to a bigint on the JS end.

I think they were previously passed as a pair because `wasm-bindgen`'s support for 64-bit integers predated the WebAssembly bigint integration that translates `i64`s to bigints. However, that feature has been around for quite a while now, so I think it should be fine to use.

I also bumped the schema version - although the schema itself hasn't changed, the ABI has, so using an older version of the CLI with this version of the `wasm-bindgen` crate or vice versa won't work and I want the version-mismatch warning to show up.

Fixes RReverser/serde-wasm-bindgen#28. This doesn't rely on a global `BigInt64Array` like the previous `i32`-pair version, which means that code using 64-bit integers will no longer fail at load time in browsers without bigint support.
alexcrichton pushed a commit to rustwasm/wasm-bindgen that referenced this issue Aug 18, 2022
Previously, they were passed as a pair of `i32`s; this changes them to be passed as a single `i64`, which is directly converted to a bigint on the JS end.

I think they were previously passed as a pair because `wasm-bindgen`'s support for 64-bit integers predated the WebAssembly bigint integration that translates `i64`s to bigints. However, that feature has been around for quite a while now, so I think it should be fine to use.

I also bumped the schema version - although the schema itself hasn't changed, the ABI has, so using an older version of the CLI with this version of the `wasm-bindgen` crate or vice versa won't work and I want the version-mismatch warning to show up.

Fixes RReverser/serde-wasm-bindgen#28. This doesn't rely on a global `BigInt64Array` like the previous `i32`-pair version, which means that code using 64-bit integers will no longer fail at load time in browsers without bigint support.
@Liamolucko
Copy link
Contributor

This should be fixed as of rustwasm/wasm-bindgen#3037.

@RReverser
Copy link
Owner

Great!

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.

4 participants