bytecodealliance / javy

JS to WebAssembly toolchain
Apache License 2.0
2.1k stars 101 forks source link

Skip serializing object entries w/ undefined value #639

Closed jbourassa closed 2 months ago

jbourassa commented 3 months ago

Description of the change

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

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

jbourassa commented 2 months ago

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 commented 2 months ago

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