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

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

Merged
merged 11 commits into from
Aug 21, 2022

Conversation

andoriyu
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

ranile commented Aug 17, 2022

Can you please merge master so CI is green?

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
crates/utils/src/json.rs Outdated Show resolved Hide resolved
crates/utils/src/json.rs Outdated Show resolved Hide resolved
crates/utils/src/json.rs Outdated Show resolved Hide resolved
crates/utils/src/json.rs Outdated Show resolved Hide resolved
crates/utils/src/lib.rs Outdated Show resolved Hide resolved
crates/utils/tests/serde.rs Outdated Show resolved Hide resolved
@andoriyu
Copy link
Contributor Author

andoriyu commented Aug 17, 2022

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
Copy link
Contributor Author

@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?

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
crates/utils/src/lib.rs Show resolved Hide resolved
crates/utils/src/format/json.rs Show resolved Hide resolved
crates/net/src/websocket/futures.rs Outdated Show resolved Hide resolved
crates/net/src/websocket/futures.rs Outdated Show resolved Hide resolved
crates/utils/tests/serde.js Outdated Show resolved Hide resolved
crates/utils/tests/serde.js Outdated Show resolved Hide resolved
andoriyu and others added 7 commits August 18, 2022 21:02
Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>
Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>
Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>
// 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) }"
Copy link
Collaborator

@futursolo futursolo Aug 19, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

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

@ranile ranile merged commit b627d82 into rustwasm:master Aug 21, 2022
@ranile
Copy link
Collaborator

ranile commented Aug 21, 2022

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

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.

4 participants