Add an API to choose the underlying JSON encoder.

matrix-org / python-canonicaljson

Canonical JSON

Apache License 2.0

31 stars 15 forks source link

Add an API to choose the underlying JSON encoder. #25

Closed clokep closed 4 years ago

clokep commented 4 years ago

This is an alternative to #24, which allows for better backwards compatibility for a period of time. It lets libraries depending on canonicaljson opt into using a different JSON implementation.

clokep commented 4 years ago

See #26 for the failing builds.

clokep commented 4 years ago

This is probably ready to go, minus some CI issues.

clokep commented 4 years ago

See matrix-org/synapse#7674 for how this would be used.

clokep commented 4 years ago

@anoadragon453 There's a bunch of conversation about this in https://github.com/matrix-org/synapse/issues/7674#issuecomment-659623838, but the gist is:

Synapse doesn't put a max version on the canonicaljson dependency, so any change must be backwards compatible to not break a new install of an old Synapse.
The goal of this is to allow Synapse to NOT use simplejson, in a backwards compatible fashion. The approach taken here is that it is backwards compatible unless you opt into new behavior.

as we seem to use simplejson in this library because we think it better performant than stdlib json

We used to think this, the latest profiling done shows this to no longer be true.

Let me know if that clears things up or not!

anoadragon453 commented 4 years ago

Ahh, I see now. We're not only worrying about other users of this library, but also old versions of Synapse. Makes sense :slightly_smiling_face:

as we seem to use simplejson in this library because we think it better performant than stdlib json

We used to think this, the latest profiling done shows this to no longer be true.

Yes, I was thinking we should just change the default in this library to always be stdlib json based on the new benchmark data, but I'm guessing that would be a backwards incompatible change.

clokep commented 4 years ago

but I'm guessing that would be a backwards incompatible change.

The standard library JSON on Python 3.5 doesn't handle bytes well, so...yeah it isn't backwards compatible. Hopefully we can do that at some point in the future!