feat(gloo-utils): Lift serde-serialization from wasm-bindgen by andoriyu · Pull Request #242 · ranile/gloo

andoriyu · 2022-08-15T01:45:00Z

Per discussion in #239 adding support for from_serde and into_serde to gloo-utils.

I couldn't replicate functionality 1:1 because wasm-bindgen uses private API methods (__wbindgen_json_serialize and __wbindgen_json_parse), so I've reimplemented it with publicly available APIs.

from_serde stayed nearly the same, but throws an error in case serde_json gives an invalid JSON. It's probably safe to do unwrap_unchecked, but I wasn't sure which MSRV should I target.

into_json is more complicated. If you pass undefined to JSON.stringify it throws an error somewhere inside. Current behavior was to return an error, to keep current behavior, I've added a check if JsValue is undefined.

While at it, made changes to crates that were using serde_json (#239 PR) to use functions from gloo-utils. Test suite is lifted from wasm-bindgen nearly verbatim.

How JSON.stringify function behaves in a browser:

> JSON.stringify({w: undefined});
"{}"
> JSON.stringify(undefined);
undefined << causes panic in rust

andoriyu · 2022-08-16T17:06:10Z

Fixed the formatting error. I thought I ran it, but I guess I didn't.

Clippy error & browser test failure looks like unrelated issue?

ranile · 2022-08-17T15:35:55Z

Can you please merge master so CI is green?

ranile

We need to document the feature on docs.rs as well. This can done by:

lib.rs

#![cfg_attr(docsrs, feature(doc_cfg))

on functions

#[cfg_attr(docsrs, doc(cfg(feature = "_")))]

Cargo.toml

[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]

crates/utils/Cargo.toml

crates/utils/src/json.rs

crates/utils/src/lib.rs

crates/utils/tests/serde.rs

andoriyu · 2022-08-17T19:00:48Z

I think that covers all requested changes. I had to use UFCS in a few places because otherwise it confuses with JsValue methods since it's a trait now. Trait is sealed, so only gloo-utils can implement it.

Tests - the way I have written it, tests must be run in node environment rather than browser. I think otherwise, you can't use imports in modules?

I'm not sure why unrelated test failed.

andoriyu · 2022-08-17T22:22:26Z

@hamza1311 do you want to keep:

let s = if self.is_undefined() {
    String::new()
} else {
    js_sys::JSON::stringify(self)
        .map(String::from)
        .unwrap_throw()
};

or do what wasm-bindgen does - wrap JSON.stringify in a function that replaces object with null if it's undefined?

There is also an option to use https://docs.rs/js-sys/latest/js_sys/JSON/fn.stringify_with_replacer.html, but I think it will slow down serialization too much?

crates/utils/src/lib.rs

ranile

I'm not sure why unrelated test failed.

The tests are all green on master now (as of #244 thanks to @futursolo). If you merge master here, the only failing tests should be from this PR

crates/utils/src/format/json.rs

crates/utils/src/lib.rs

crates/utils/src/format/json.rs

crates/net/src/websocket/futures.rs

crates/utils/tests/serde.js

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

futursolo · 2022-08-19T07:11:18Z

crates/utils/src/format/json.rs

+// we're passed `undefined` reinterpret it as `null` for JSON
+// purposes.
+#[wasm_bindgen(
+    inline_js = "export function serialize(obj) { return JSON.stringify(obj === undefined ? null : obj) }"


Inline JS snippets could cause issues for non-ES module targets.
no-modules target is still the only target supported by gloo-worker.

See: https://rustwasm.github.io/docs/wasm-bindgen/examples/without-a-bundler.html#using-the-older---target-no-modules

Instead of going through JSON::stringify and serde_json, I would suggest using serde-wasm-bindgen. It supports non-serializable types (e.g.: ES2015 Map) and has a smaller bundle footprint than serde_json.

@futursolo oh well, back to what I had before.

So if you use serde_wasm_bindgen, then you don't really need this. The goal is to provide a drop-in replacement for JsValue::{from_serde/to_serde} and an easier way to use js_sys::JSON::.

Plus, in the deprecation thread, someone said they had a bad time with serde-wasm-bindgen. I think it makes sense: it calls to JS for every field which is slow.

ranile

Looks good to me. Thanks a lot for your work!

ranile · 2022-08-21T14:23:50Z

These changes have been released in:

gloo-utils: v0.1.5
gloo-net: v0.2.4
gloo-storage: v0.2.2
gloo-console: v0.2.3

andoriyu force-pushed the feat/add_serde_serialiation branch from 18e2d4a to a49af99 Compare August 16, 2022 17:01

andoriyu mentioned this pull request Aug 16, 2022

Deprecate JsValue::from_serde and JsValue::into_serde wasm-bindgen/wasm-bindgen#3031

Merged

ranile requested changes Aug 17, 2022

View reviewed changes

andoriyu force-pushed the feat/add_serde_serialiation branch from 87a006f to ff13de2 Compare August 17, 2022 18:51

andoriyu requested a review from ranile August 17, 2022 18:53

Liamolucko reviewed Aug 18, 2022

View reviewed changes

crates/utils/src/lib.rs Show resolved Hide resolved

ranile reviewed Aug 18, 2022

View reviewed changes

andoriyu and others added 7 commits August 18, 2022 21:02

feat(gloo-utils): Lift serde-serialization from wasm-bindgen

bd52da5

Update crates/utils/Cargo.toml

f3149b0

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

Update crates/utils/src/json.rs

75a9560

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

Update crates/utils/src/json.rs

91476dc

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

Address the feedback

ecd3e55

only re-export if feature is enable

3b06f25

address the feedback

f34da81

andoriyu force-pushed the feat/add_serde_serialiation branch from 7aa660a to f34da81 Compare August 18, 2022 21:02

andoriyu requested a review from ranile August 18, 2022 21:48

futursolo reviewed Aug 19, 2022

View reviewed changes

andoriyu and others added 4 commits August 19, 2022 11:49

Update json.rs

d06c888

Update json.rs

81791fa

Update json.rs

6f8067c

cargo fmt

8cac0bf

ranile approved these changes Aug 21, 2022

View reviewed changes

ranile merged commit b627d82 into ranile:master Aug 21, 2022

Conversation

andoriyu commented Aug 15, 2022

Uh oh!

andoriyu commented Aug 16, 2022

Uh oh!

ranile commented Aug 17, 2022

Uh oh!

ranile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andoriyu commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andoriyu commented Aug 17, 2022

Uh oh!

Uh oh!

ranile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

futursolo Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andoriyu Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

ranile left a comment

Choose a reason for hiding this comment

Uh oh!

ranile commented Aug 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andoriyu commented Aug 17, 2022 •

edited

Loading

futursolo Aug 19, 2022 •

edited

Loading