DISCUSSION DRAFT: Facility to support dependency-free JSON, System.Text.Json, and Newtonsoft.Json#93
Conversation
sblom
force-pushed
the
prototype/generic-json-wrapper
branch
from
5f50bc9 to
991ef99
Compare
asbjornu
left a comment
asbjornu
left a comment
There was a problem hiding this comment.
- Lost support for .NET Standard 1.x (even 1.3) because of reflection requirements in
TinyJson.
Not a huge loss, imho.
This can be overcome.
How?
- Many tests currently failing. I had almost all of the conformance cases passing with
OmniJsonin a previous commit. I'll go back and make a pass to fix up broken cases once I get the Newtonsoft implementation finished up.
👍🏼
This change removes the
Newtonsoft.Jsondependency from the main project, and replaces it with a home-built facility called 'OmniJson' that provides a similar interface which supports dependency-free JSON by way ofTinyJson, or can be enhanced with additional NuGet mix-ins (possibly calledjson-ld.net.Newtonsoftandjson-ld.net.SystemTextJson) that would overrideOmniJsonTokenet al, and provide API methods that expect and return NewtonsoftJTokens orSystem.Text.Json-backed JSDM values.
Besides the naming and some nits regarding the exposed interface, I think this is great. I like that we take ownership of our exposed API and are able to reduce its surface considerably.
Regarding the naming, I think the fact that the types reside within then JsonLD namespace is enough to differentiate them from other serializers and can thus just give the classes names such as JsonArray, JsonObject, etc.
|
|
||
| namespace JsonLD.OmniJson | ||
| { | ||
| public class OmniJsonArray : OmniJsonToken, IList<OmniJsonToken>, IList |
There was a problem hiding this comment.
I'm not sold on the OmniJson name, but I do like that we take ownership of the external API we provide.
There was a problem hiding this comment.
Really good point about just calling it JsonLD.Json{Token|Object|Array|Value}. That also helps get me over the other hang-up I had around how much of the Newtonsoft JToken interface(s) I needed to clone. (Exactly to your points below.)
|
|
||
| public virtual void CopyTo(OmniJsonToken[] array, int arrayIndex) | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
Wouldn't it be better to not implement IList and thus reduce the API surface? IEnumerable is probably sufficient, no?
|
|
||
| public virtual OmniJsonToken this[string key] { get => _wrapped.ContainsKey(key) ? Wrap(_wrapped[key]) : null; set => _wrapped[key] = value?.Unwrap(); } | ||
|
|
||
| public virtual ICollection<string> Keys => throw new NotImplementedException(); |
There was a problem hiding this comment.
Better to not implement IDictionary than to throw NotImplementedException, no?
|
This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions. |
|
I haven't touched this in a while, but I still believe in the approach. Let me find some time in the next several weeks to update my draft with @asbjornu's feedback. |
|
This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions. |
2 participants
Code to illustrate my thoughts on #65
Current Regressions
TinyJson. This can be overcome.OmniJsonin a previous commit. I'll go back and make a pass to fix up broken cases once I get the Newtonsoft implementation finished up.Overview
This change removes the
Newtonsoft.Jsondependency from the main project, and replaces it with a home-built facility called 'OmniJson' that provides a similar interface which supports dependency-free JSON by way ofTinyJson, or can be enhanced with additional NuGet mix-ins (possibly calledjson-ld.net.Newtonsoftandjson-ld.net.SystemTextJson) that would overrideOmniJsonTokenet al, and provide API methods that expect and return NewtonsoftJTokens orSystem.Text.Json-backed JSDM values.