codec: allow user to override default JSON serializer (#157) #159
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
===================================================
+ Coverage 85.52917% 85.60010% +0.07093%
===================================================
Files 189 189
Lines 16129 16139 +10
===================================================
+ Hits 13795 13815 +20
+ Misses 1774 1762 -12
- Partials 560 562 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| jsoniter "github.com/json-iterator/go" | ||
| ) | ||
|
|
||
| // JSONAPI is json packing and unpacking object, users can change | ||
| // the internal parameter. | ||
| var JSONAPI = jsoniter.ConfigCompatibleWithStandardLibrary | ||
| var JSONAPI JSONSerializer = jsoniter.ConfigCompatibleWithStandardLibrary |
There was a problem hiding this comment.
The type of JSONAPI still changes.
Compatibility means the name and the type of an exported entity both cannot be changed.
There was a problem hiding this comment.
The scenario you are using is for RESTful? Since for the trpc protocol, the json implementation used is actually codec.JSONPBSerializer.
There was a problem hiding this comment.
The scenario you are using is for RESTful? Since for the trpc protocol, the json implementation used is actually
codec.JSONPBSerializer.
@WineChord I changed both JSONAPI simple because I saw them both defined as jsoniter typped variable. Should I just change one?
There was a problem hiding this comment.
The type of
JSONAPIstill changes.Compatibility means the name and the type of an exported entity both cannot be changed.
I modified JSONAPI just trying to follow the programming style of trpc. However I still feel confused of why JSONAPI, JSONSerialization is exportable.
There was a problem hiding this comment.
Comments of JSONAPI said: JSONAPI is json packing and unpacking object, users can change the internal parameter.
As JSONAPI is claimed to used to 'change', I am afraid it should be an interface instead of jsoniter-typped variable.
There was a problem hiding this comment.
I understand your perspective, and what I'm trying to highlight here is the conundrum of strict compatibility. Strict compatibility implies that nothing about the exported entities can be altered.
Indeed, JSONAPI should not have been exported initially. The PR was submitted 4 years ago (July 16th, 2020 1:59 PM) to allow users to modify the codec.JSONAPI to a custom one (e.g., codec.JSONAPI = jsoniter.Config{EscapeHTML: false, SortMapKeys: true, ValidateJsonRawMessage: true}.Froze()). However, I fail to see why this should be incorporated into the framework rather than the user's code. Nonetheless, it was merged and has continued in this manner.
Regarding the type, it should not be the native type of jsoniter. It should be an interface, and I believe that the following would suffice for your needs:
| var JSONAPI JSONSerializer = jsoniter.ConfigCompatibleWithStandardLibrary | |
| var JSONAPI Serializer = jsoniter.ConfigCompatibleWithStandardLibrary |
Here, codec.Serializer is an already defined interface. I believe that the framework only requires plain Marshal/Unmarshal.
However, we're still discussing the matter of strict compatibility, which means this change would still break compatibility.
It might be decided that changing the type is acceptable since the framework only uses the Marshal/Unmarshal utility. However, users may directly use other methods provided by the exported JSONAPI variable. After changing its type, users' code would break because the JSONAPI variable no longer has those methods it previously defined.
It's a tough decision.
cc @tocrafty
|
Close due to discussion in #157 |
As an alternative of #157, not removing json-iterator, but allow user to override default JSON serializer.