Skip to content
This repository has been archived by the owner on Apr 6, 2024. It is now read-only.

Fixed cyclic reference in cargo. #9

Closed

Conversation

CMoore-Darwinium
Copy link
Contributor

@CMoore-Darwinium CMoore-Darwinium commented Sep 22, 2022

This changes worker to use serde-wasm-bindgen package, rather than relying on the deprecated serde option in wasm-bindgen.

Description of the cyclic reference issue that I was hitting is here:
tkaitchuck/aHash#95

wasm_bindgen::JsValue::from_serde and wasm_bindgen::JsValue::into_serde are deprecated here (citing the cyclic reference issue that I encountered).
rustwasm/wasm-bindgen#3031

Also, additional description of serde-wasm-bindgen is found here (turns out that it was written by Cloudflare).
rustwasm/wasm-bindgen#1258

This patch replaces the usage of wasm_bindgen::JsValue::from_serde and wasm_bindgen::JsValue::into_serde with serde_wasm_bindgen::from_value() and serde_wasm_bindgen::to_value() equivalents..

…en::JsValue::into_serde that have been deprecated. Added the new serde_wasm_bindgen::from_value() and serde_wasm_bindgen::to_value() equivalents instead.
@CMoore-Darwinium CMoore-Darwinium changed the title Fixed potential cyclic reference in cargo. Fixed cyclic reference in cargo. Sep 22, 2022
Copy link
Owner

@zebp zebp 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 apprehensive to use serde-wasm-bindgen, which would add another error variant and expand what can and can't be deserialized, both of which I'm a bit uncomfortable with. As noted in the documentation of JsValue::from_serde, gloo-utils' JsValueSerdeExt has serde-based json serialization while using serde_json's error type and is still strictly JSON-able types. I think this PR should switch to gloo-utils to solve the dependency cycle.

@CMoore-Darwinium
Copy link
Contributor Author

serde-wasm-bindgen in theory will allow a superset of what gloo-utils can serialize, as well as leading to a smaller binary size.
serde-wasm-bindgen's error is just a wrapper around JsValue, we can change it to the KvError::JavaScript variant to maintain the same number of varients.

@zebp
Copy link
Owner

zebp commented Sep 23, 2022

serde-wasm-bindgen in theory will allow a superset of what gloo-utils can serialize

I'm aware, but I'd prefer if we kept the set of items that could be serialized the same as what we have currently.

serde-wasm-bindgen's error is just a wrapper around JsValue, we can change it to the KvError::JavaScript variant to maintain the same number of varients.

This would break anyone's code that was relying on getting a serialization error if the input could not be serialized, which I'd don't want to do.

Also as for binary size, I don't think it'll make much of a difference as anything unrelated to serialization will get removed when linking as it's unused when we use the extension trait.

@CMoore-Darwinium
Copy link
Contributor Author

There is now a gloo_utils based pull request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants