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

Fixed cyclic reference in cargo. #227

Merged
merged 2 commits into from
Jan 2, 2023
Merged

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.

Together with this patch in workers-kv here, the observed cyclic reference issues are gone.

…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 Fixed cyclic reference in cargo. Sep 22, 2022
@jakubadamw
Copy link
Contributor

It would be great to see this change merged. 🙂

@itsbalamurali
Copy link

@zebp 🤓

Copy link
Collaborator

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

Sorry this review took so long, I've been really preoccupied with other things. LGTM

@zebp zebp merged commit 676d399 into cloudflare:main Jan 2, 2023
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