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

Skip serializing object entries w/ undefined value #639

Merged

Conversation

jbourassa
Copy link
Contributor

@jbourassa jbourassa commented May 3, 2024

Description of the change

Improve Javy's Value serializer compatibility with JSON.stringify:

  • For Arrays, nullify non-JSONable values
  • For Objects, skip entries with non-JSONable values

Why am I making this change?

Encoding a QuickJS value to JSON using the transcoder offers a nice performance gain over QuickJS's JSON.stringify, but differs in behaviour. This PR should partially close the gap.

$ echo "console.log(JSON.stringify({foo: undefined}))" | node
{}

Without this change, transcoding the above {foo: undefined} to a JSON string would result in {"foo":null}.

This PR fixes that by skipping skipping the deserialization of entries with undefined. This is a lossy conversion given serde data model doesn't know about undefined, and both the serde Unit and None data types are serialized as null in JSON.

Similar logic is applied to Array values, but in that case the values are nullified instead of skipped.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • [ ] I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • [ ] I've updated documentation including crate documentation if necessary.

@jbourassa jbourassa force-pushed the skip-undefined-obj-values branch from 4c0df91 to e156914 Compare May 6, 2024 14:31
@jbourassa jbourassa marked this pull request as ready for review May 6, 2024 14:32
@saulecabrera saulecabrera self-requested a review May 6, 2024 20:38
Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

This change looks good to me, but it also got me thinking about other things:

  • I think we should also check if values are functions and values are symbols? That's what the official JS JSON spec states:

undefined, Function, and Symbol values are not valid JSON values. If any such values are encountered during conversion, they are either omitted (when found in an object) or changed to null (when found in an array). JSON.stringify() can return undefined when passing in "pure" values like JSON.stringify(() => {}) or JSON.stringify(undefined).

  • I'm wondering how this affects the messagepack implementation, if anything at all, given that this deserializer is used there too. I don't think that the messagepack crate is used at all in general, maybe another sensible option is to deprecate it.

@jbourassa
Copy link
Contributor Author

I think we should also check if values are functions and values are symbols?

Good point, I can throw those in too. Hopefully it has limited impact on performance.

I'm wondering how this affects the messagepack implementation, if anything at all, given that this deserializer is used there too.

It will, but arguably that's a feature? I'd expect the msgpack implementation to be exactly like JSON.stringify, but msgpack encoded.

@jbourassa jbourassa force-pushed the skip-undefined-obj-values branch 2 times, most recently from b9ad184 to 985499c Compare May 15, 2024 00:01
@jbourassa jbourassa force-pushed the skip-undefined-obj-values branch from 985499c to 8b85c96 Compare May 15, 2024 00:03
@jbourassa
Copy link
Contributor Author

@saulecabrera I've now applied your suggestion of checking for more types. Sorry for the delay!

@jbourassa jbourassa requested a review from saulecabrera May 15, 2024 00:17
Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@saulecabrera saulecabrera merged commit 162b3da into bytecodealliance:main May 15, 2024
14 checks passed
@jbourassa jbourassa deleted the skip-undefined-obj-values branch May 16, 2024 17:03
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