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

Add serde serialization to fj. #610

Merged
merged 4 commits into from
May 20, 2022

Conversation

freylint
Copy link
Contributor

Adds serde as an optional dependency to fj.

I ran into the need this for while marshaling Shapes between javascript and rust for the wasm32-unknown-unknown platform.

I believe implementing serde on crates like this is also considered idiomatic. I'm not sure if this implementation is the exact idiomatic way to accomplish that.

@freylint freylint requested a review from hannobraun as a code owner May 20, 2022 01:11
@hannobraun hannobraun force-pushed the feature/shape-serialization branch from 9b421b4 to 34f5e89 Compare May 20, 2022 10:17
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @freylint, look great!

This doesn't seem to have any adverse effect on the compile times, so I'm happy to have it. I've left one comment with a suggestion. Feel free to address that in a follow-up PR or not. I'll merge this either way, once the CI build is green.


[features]
default = []
serialization = ["serde"]
Copy link
Owner

Choose a reason for hiding this comment

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

I think we don't need an explicit feature here. Just having the implicit serde feature should be enough, and is what I've seen in other crates in the Rust ecosystem.

@hannobraun hannobraun enabled auto-merge May 20, 2022 10:19
@hannobraun hannobraun merged commit 25f464e into hannobraun:main May 20, 2022
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.

2 participants