ref: refactored events data parsing implementation #231
Conversation
…ion implementation
mohitpubnub
requested review from
Xavrax,
marcin-cebo and
jakub-grzesiowski
as code owners
mohitpubnub
added
the
priority: medium
| @@ -7,12 +7,16 @@ internal class ResponseBuilder | |||
| private readonly PNConfiguration config; | |||
| private readonly IJsonPluggableLibrary jsonLib; | |||
| private readonly IPubnubLog pubnubLog; | |||
| private readonly NewtonsoftJsonDotNet newtonsoftJsonDotNet; | |||
There was a problem hiding this comment.
There's already a IJsonPluggable reference here, we shouldn't use the concrete implementation because NewtonsoftJsonDotNet breaks in certain Unity build targets
There was a problem hiding this comment.
It's safe with that aspect, Since this code is not changing configuration related to serialisation/deserialisation.
NewtonsoftJsonDotNet is existing custom serialiser library Which is already being used in other parts of the sdk (e.g. PubnubCore class) and got tested already with unity since old versions.
The reason for not using pluggable one, There are some default settings in serialiser(with netwonsoft) and type declarations to make sdk's NewtonsoftJsonDotNet class compatible with unity and other supported targets.
Using pluggable one might leave a chance to break existing behaviour while deserialising the custom user messages.
There was a problem hiding this comment.
We currently have NewtonsoftJsonUnity in Unity SDK, which is very similar but uses the Unity version of Newtonsoft library to avoid crashing in IL2CPP build mode (it was a client reported issue a while back), and from what I see we use the DotNet version directly as the default value if no other pluggabel was provided - we definitely can't use it instead of pluggable
There was a problem hiding this comment.
updated the implementation, not EventSerializer is using only json pluggable.
for unity compatibility, I saw updates at v6.20.1
also this will make this implementation a bit more unified.
good catch!
| public class EventDeserializer | ||
| { | ||
| private readonly IJsonPluggableLibrary jsonLibrary; | ||
| private readonly NewtonsoftJsonDotNet newtonSoftJsonLibrary; // the default serializer of sdk |
|
|
||
| private PNPresenceEventResult DeserializePresenceEvent(IDictionary<string, object> jsonFields) | ||
| { | ||
| Dictionary<string, object> presenceDicObj = jsonLibrary.ConvertToDictionaryObject(jsonFields["payload"]); |
There was a problem hiding this comment.
Any chance the "payload" key won't be there? That goes for the other keys too, do we need TryGetValue-s here?
There was a problem hiding this comment.
Agree. I think it's just coding style I followed.
But true, We can avoid those checks.
Updating....
There was a problem hiding this comment.
updated for mandatory fields
| @@ -23,7 +20,7 @@ public T Deserialize<T>(IDictionary<string, object> json) | |||
| var typeInfo = typeof(T); | |||
| if (typeInfo.GetTypeInfo().IsGenericType && typeInfo.GetGenericTypeDefinition() == typeof(PNMessageResult<>)) | |||
| { | |||
| response = newtonSoftJsonLibrary.DeserializeMessageResultEvent<T>(json); | |||
| response = jsonLibrary.DeserializeToObject<T>(json); | |||
There was a problem hiding this comment.
@jakub-grzesiowski updated to json pluggable library
There was a problem hiding this comment.
instead jsonDotNet
…ializer dependency
ref(subscribe): events data parsing/deserialisation implementation enhancement
Scope:
Refactor Events data parsing implementation for subscribe responses to avoid major changes in future while updating any event data fields. [e.g Adding custom messageType in message event]
Serialisation library apis changes are out of scope for this change.