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

Deprecate JsValue::from_serde and JsValue::into_serde #3031

Merged
merged 6 commits into from Aug 30, 2022

Conversation

Liamolucko
Copy link
Collaborator

I've listed serde-wasm-bindgen as the replacement, and changed the section of the guide that talks about Serde to talk about serde-wasm-bindgen instead of the deprecated methods.

I didn't remove it entirely because I can imagine someone remembering it and trying to look it back up, only to find that it no longer exists, which would quite frustrating. I also added a footnote about the deprecated methods in case someone remembers the old way and wants to know what happened.

There were several examples using from_serde/into_serde, which I updated to use serde-wasm-bindgen or not use serde altogether.

The fetch example was a bit weird, in that it took a JS value, parsed it into a Rust value, only to serialize it back into a JS value. I removed that entirely in favour of just passing the original JS value directly. I suppose it behaves slightly differently in that it loses the extra validation, but a panic isn't all that much better than a JS runtime error.

Fixes #2770
Fixes tkaitchuck/aHash#95

This also indirectly 'fixes' issues about the deprecated functions:

Resolves #1258
Resolves #2017
Resolves #2539

I've listed `serde-wasm-bindgen` as the replacement, and changed the section of the guide that talks about Serde to talk about `serde-wasm-bindgen` instead of the deprecated methods.

I didn't remove it entirely because I can imagine someone remembering it and trying to look it back up, only to find that it no longer exists, which would quite frustrating. I also added a footnote about the deprecated methods in case someone remembers the old way and wants to know what happened.

There were several examples using `from_serde`/`into_serde`, which I updated to use `serde-wasm-bindgen` or not use `serde` altogether.

The `fetch` example was a bit weird, in that it took a JS value, parsed it into a Rust value, only to serialize it back into a JS value. I removed that entirely in favour of just passing the original JS value directly. I suppose it behaves slightly differently in that it loses the extra validation, but a panic isn't all that much better than a JS runtime error.
@gthb
Copy link
Contributor

gthb commented Aug 16, 2022

Whether serde-wasm-bindgen is preferable probably depends a lot on the use case, so “replacement” and “deprecation” seems a bit off to me.

As a data point, in our use case (a parser module compiled to WebAssembly, passing an AST to the JavaScript side) we went the opposite way, dropping serde-wasm-bindgen in favor of JsValue::from_serde ... effectively simply this change:

 pub fn parse(formula: &str) -> JsValue {
-    let serializer = serde_wasm_bindgen::Serializer::new().serialize_maps_as_objects(true);
     match native_parse(formula) {
         Ok(result) => {
-            result.serialize(&serializer).unwrap_or_else(|err| {
+            JsValue::from_serde(&result).unwrap_or_else(|err| {

and this significantly reduced per-parse call overhead; we saw our measured parse times on real-world data (measured in Node.js 16.x) change from:

  • average: 92.6 μs,
  • 99.9th percentile : 1.44 ms

to:

  • average 38.6 μs
  • 99.9th percentile : 390 μs

This is presumably because a structure of potentially many JS objects is returned, which gets chatty over the JS/WebAssembly boundary in the serde-wasm-bindgen implementation, so from_serde is preferable in such use cases.

So the comment “This approach [serde-wasm-bindgen] has both advantages and disadvantages” being removed in this PR remains true and I don't think it should be removed.

Plus serde-wasm-bindgen (or at least the use of it in some cases, wherever the root cause may lie) has this rather serious issue RReverser/serde-wasm-bindgen#28 ... it is still unaddressed, and completely breaks module loading on Safari 14 and earlier. That's 4% of current browsers, according to https://browserslist.dev/?q=aW9zIDwgMTUsIHNhZmFyaSA8IDE1

So, you know, not thrilled about this idea. :-)

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree that serde-wasm-bindgen is a bit of a different use case than the methods deprecated here so I don't think it's a direct replacement. Instead though could the __wbindgen_json_{parse,serialize} methods be exposed as first-class methods perhaps? That would give convenient access to JSON and additionally be able to lift the serde dependency up in to callers instead of baking it in wasm-bindgen itself.

@andoriyu
Copy link

@alexcrichton I've started this PR rustwasm/gloo#242 to extract this functionality. If __wbindgen_json_{parse,serialize} were publicly exposed, then behavior would match exactly. I've documented the difference in my PR.

@hamza1311
Copy link
Collaborator

As far as I understand it, __wbindgen_json_{parse,serialize} is equivalent to js_sys::JSON::{parse,stringify} so those are already exposed. Or maybe I'm misunderstanding something?

@alexcrichton
Copy link
Contributor

Indeed! In that case I think it would be good to migrate the examples to that instead and to mention that as an alternative in addition to serde-wasm-bindgen

@andoriyu
Copy link

@hamza1311 I think they slightly different: in my PR for gloo, when I passed undefined to js_sys::JSON::stringify it panicked while __wbindgen_json_serialize didn't.

@Liamolucko
Copy link
Collaborator Author

@hamza1311 I think they slightly different: in my PR for gloo, when I passed undefined to js_sys::JSON::stringify it panicked while __wbindgen_json_serialize didn't.

Looks like the intrinsic has a special case for undefined:

Intrinsic::JsonSerialize => {
assert_eq!(args.len(), 1);
// Turns out `JSON.stringify(undefined) === undefined`, so if
// we're passed `undefined` reinterpret it as `null` for JSON
// purposes.
prelude.push_str(&format!("const obj = {};\n", args[0]));
"JSON.stringify(obj === undefined ? null : obj)".to_string()
}

That should be able to be replicated in gloo pretty easily.

@andoriyu
Copy link

@hamza1311 Oh, it's using a replaces function It's just replacing the entire object with null. That's similar to what I did - instead of calling JSON.stringify I return String::new() if the object is undefinied

@alexcrichton
Copy link
Contributor

@Liamolucko would you be up for rewording to indicate that js_sys::JSON is an alternative and leaving the examples as-is but updating them to use js_sys::JSON?

@Liamolucko
Copy link
Collaborator Author

Okay, I've added a section to the guide page which talks about using JSON as an alternative to serde-wasm-bindgen. Currently it uses js-sys + serde_json directly, but it should probably be updated to use gloo-utils when rustwasm/gloo#242 is merged.

In my opinion, serde-wasm-bindgen is the better default, since the performance can swing either way and serde-wasm-bindgen has smaller code size. I agree that JSON should be mentioned though, since it can be much faster in some cases like @gthb's.

Also, I ran serde-wasm-bindgen's benchmark suite on a couple different engines and uploaded the results here, if you're curious exactly how it stacks up against JSON.

I was considering leaving the examples using `JSON` directly and mentioning `gloo-utils` as an aside, but that has the major footgun that `JSON.stringify(undefined) === undefined`, causing a panic when deserializing `undefined` since the return type of `JSON::stringify` isn't optional. `gloo-utils` works around this, so I recommended it instead.
`serde_json`. This caused problems when certain features of `serde_json` and
other crates were enabled that caused it to depend on `wasm-bindgen`, creating
a circular dependency, which is illegal in Rust and caused people's code to
fail to compile. So, they were deprecated.
Copy link
Contributor

@gthb gthb Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions in this crate were deprecated, but the usage isn't really deprecated (just no longer the recommended default, and requires a trait from a different crate), so maybe

Suggested change
fail to compile. So, they were deprecated.
fail to compile. So, they were moved out of `wasm-bindgen` into `gloo_utils` to
resolve the circular dependency and replaced by `serde-wasm-bindgen` as the
default approach to use.

... or something like that.

src/lib.rs Outdated
@@ -214,6 +221,7 @@ impl JsValue {
///
/// Returns any error encountered when serializing `T` into JSON.
#[cfg(feature = "serde-serialize")]
#[deprecated = "causes dependency cycles, use `serde-wasm-bindgen` instead"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[deprecated = "causes dependency cycles, use `serde-wasm-bindgen` instead"]
#[deprecated = "causes dependency cycles, use `serde-wasm-bindgen` instead, or `gloo_utils::format::JsValueSerdeExt`"]

src/lib.rs Outdated
@@ -201,6 +201,13 @@ impl JsValue {
/// Creates a new `JsValue` from the JSON serialization of the object `t`
/// provided.
///
/// **This function is deprecated**, due to [creating a dependency cycle in
/// some circumstances][dep-cycle-issue]. Use [`serde-wasm-bindgen`]
/// instead, or manually call `serde_json::to_string` + `JSON.parse`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// instead, or manually call `serde_json::to_string` + `JSON.parse`.
/// instead, or `gloo_utils::format::JsValueSerdeExt`, or manually call
/// `serde_json::to_string` + `JSON.parse`.

src/lib.rs Outdated
@@ -225,6 +233,13 @@ impl JsValue {
/// Invokes `JSON.stringify` on this value and then parses the resulting
/// JSON into an arbitrary Rust value.
///
/// **This function is deprecated**, due to [creating a dependency cycle in
/// some circumstances][dep-cycle-issue]. Use [`serde-wasm-bindgen`]
/// instead, or manually call `JSON.stringify` + `serde_json::from_str`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// instead, or manually call `JSON.stringify` + `serde_json::from_str`.
/// instead, or `gloo_utils::format::JsValueSerdeExt`, or manually call
/// `JSON.stringify` + `serde_json::from_str`.

src/lib.rs Outdated
@@ -236,6 +251,7 @@ impl JsValue {
///
/// Returns any error encountered when parsing the JSON into a `T`.
#[cfg(feature = "serde-serialize")]
#[deprecated = "causes dependency cycles, use `serde-wasm-bindgen` instead"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[deprecated = "causes dependency cycles, use `serde-wasm-bindgen` instead"]
#[deprecated = "causes dependency cycles, use `serde-wasm-bindgen` instead, or `gloo_utils::format::JsValueSerdeExt`"]

@RReverser
Copy link
Member

RReverser commented Aug 25, 2022

Also, I ran serde-wasm-bindgen's benchmark suite on a couple different engines and uploaded the results here, if you're curious exactly how it stacks up against JSON.

Just to add some clarification not reflected in graph titles - after various added optimisations over time, serde-wasm-bindgen should be faster than serde-json based approach for almost all cases, except when your data has a lot of strings.

Not much more we can do there, as having all strings serialized into one large string (JSON) on one side, passing that string once, and parsing it back on another side will be always faster than passing tons of small strings one-by-one (same overhead x N).

For other data types we can do a lot better than JSON though, and that's why twitter benchmark (lots of strings) looks so different than others.

UPD / side-note: I see the original comment mentions lots of small objects and yeah, I can see how that case would have similar overhead issues to having lots of strings.

trevor-crypto added a commit to trevor-crypto/cardano-serialization-lib that referenced this pull request Aug 26, 2022
1. Reorganize dependencies and remove requirements ("=") on wasm-bindgen
   and js-sys, due to conflicts that can occur when using this crate.
2. Replace wasm-bindgen's "serde-serialize" feature with
   serde-wasm-bindgen due to cyclical dependency issue.

Relates to:
tkaitchuck/aHash#95
rustwasm/wasm-bindgen#3031
trevor-crypto added a commit to trevor-crypto/cardano-serialization-lib that referenced this pull request Aug 26, 2022
1. Reorganize dependencies and remove requirements ("=") on wasm-bindgen
   and js-sys, due to conflicts that can occur when using this crate.
2. Replace wasm-bindgen's "serde-serialize" feature with
   serde-wasm-bindgen due to cyclical dependency issue.

Relates to:
tkaitchuck/aHash#95
rustwasm/wasm-bindgen#3031

`serde-wasm-bindgen` works by directly manipulating JavaScript values. This
requires a lot of calls back and forth between Rust and JavaScript, which can
sometimes be slow. An alternative way of doing this is to serialize values to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's important to still call out the limitations in terms of type support in JSON-based approach here too (like the document did in the original version).

The new version of this section makes it sound like both approaches are equivalent and the only difference is performance. However, the support for various JavaScript types that are not representable by JSON is an even more important advantage of serde-wasm-bindgen than just runtime perf or code size IMO.

sometimes be slow. An alternative way of doing this is to serialize values to
JSON, and then parse them on the other end. Browsers' JSON implementations are
usually quite fast, and so this approach can outstrip `serde-wasm-bindgen`'s
performance in some cases.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
performance in some cases.
performance in some cases. But this approach supports only types that can be
serialized as JSON, leaving out some important types that `serde-wasm-bindgen`
supports such as `Map`, `Set`, and array buffers.

... addressing @RReverser's point about type support

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so this edit didn't get in? Can you make a separate PR please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voilà! #3047

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thanks again! I think further iteration is safe to happen in-tree

relrelb pushed a commit to Aaron1011/ruffle that referenced this pull request Sep 24, 2022
The `serde-serialize` feature is deprecated
(rustwasm/wasm-bindgen#3031).

This solves a cyclic dependency error with aHash
relrelb pushed a commit to ruffle-rs/ruffle that referenced this pull request Sep 24, 2022
…8080)

The `serde-serialize` feature is deprecated
(rustwasm/wasm-bindgen#3031).

This solves a cyclic dependency error with aHash
facebook-github-bot pushed a commit to facebook/hhvm that referenced this pull request Oct 5, 2022
…alize feature

Summary:
`wasm-bindgen`s `serde-serialize` feature is the cause of:

> cyclic package dependency: package indexmap v1.8.0 depends on itself

See https://fb.workplace.com/groups/rust.language/posts/9594592340589295/

This diff updates `wasm-bindgen` to 0.2.83, which deprecates the
`serde-serialize` feature in favor of the `serde-wasm-bindgen` create
(see rustwasm/wasm-bindgen#3031)

Reviewed By: diliop

Differential Revision: D40104867

fbshipit-source-id: 9396923fba95ff7919c9a751b6a76c407934b9e4
@tomasro27
Copy link

tomasro27 commented Feb 24, 2023

I'm still seeing this issue after updating wasm-bindgen to latest 0.2.84.
I also upgraded getrandom on a private branch to use wasm-bindgen 0.2.84.
wasm-bindgen has deprecated JsValue::from_serde and JsValue::into_serde, but it still has serde-json as optional dependency. Is this the issue if I have other crates depending on serde_json?
Any help is appreciated.

On the error below I can see wasm-bindgen on latest version 0.2.84. Also js-sys on latest 0.3.61 which consumes wasm-bindgen 0.2.84.

error: cyclic package dependency: package `ahash v0.7.6` depends on itself. Cycle:
package `ahash v0.7.6`
    ... which satisfies dependency `ahash = "^0.7.0"` of package `hashbrown v0.12.3`
    ... which satisfies dependency `hashbrown = "^0.12"` of package `indexmap v1.9.2`
    ... which satisfies dependency `indexmap = "^1.5.2"` of package `serde_json v1.0.93`
    ... which satisfies dependency `serde_json = "^1.0"` of package `wasm-bindgen v0.2.84`
    ... which satisfies dependency `wasm-bindgen = "^0.2.84"` of package `js-sys v0.3.61`
    ... which satisfies dependency `js-sys = "^0.3"` of package `getrandom v0.2.81 (https://github.com/tomasro27/getrandom?rev=ddcf36ef7ee401af102debb9c758703b1e574461#ddcf36ef)`
    ... which satisfies dependency `getrandom = "^0.2.3"` of package `ahash v0.7.6`

@RReverser
Copy link
Member

Did you remove the serde-serialize feature from wasm-bindgen in your Cargo.toml after the update?

@tomasro27
Copy link

@RReverser I did not have the serde-serialize feature, but from reading more on the ahash thread, the issue is probably that one of my dependencies directly or indirectly defines wasm-bindgen with the serde-serialize feature enabled.
Is that a fair assumption? meaning that I would need to track down any dependencies that use wasm-bindgen with serde-serialize in my dependency tree so that my project does not enable that feature indirectly.

Is there a way to force indirect dependencies not using serde-serialize feature on wasm-bindgen? or is the only fix to upgrade those dependencies?

Thanks!

@tomasro27
Copy link

For anyone that might find this helpful, you can run cargo metadata > metadata.json to generate a file with all the crates/dependencies on your project. Then find all usages of wasm-bindgen with the feature flag serde-serialize (make sure it is not a dev dependency, as that might be okay). After finding which crates depend on wasm-bindgen with the feature flag enabled, you will need to update those packages to the latest version that does not use the serde-serialize feature in their dependencies.

@RReverser
Copy link
Member

RReverser commented Feb 24, 2023

@tomasro27 FWIW for slightly simpler way you could also do cargo tree -p wasm-bindgen which will show you a tree of deps showing where wasm-bindgen is used. Or you could even just look at Cargo.lock contents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants