Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch to newtonsoft.json #3

Open
apexdodge opened this issue Oct 20, 2013 · 14 comments
Open

switch to newtonsoft.json #3

apexdodge opened this issue Oct 20, 2013 · 14 comments

Comments

@apexdodge
Copy link

Hi there, great library.

I've run into a number of problems trying to compile the solution and it is largely due to JsonFX. I was able to get it running on some sites, but others would crash unexpectedly.

I modified your code to use http://www.nuget.org/packages/newtonsoft.json/ which is the most widely and popular used json library in .NET. Now it works without any issues at all.

Just thought you should know, I can provide details on the code I changed if you'd like.

@Dynalon
Copy link
Owner

Dynalon commented Oct 21, 2013

Hi,

I know Newtonsoft.JSON but IIRC I didn't chose it because of the lacking dynamic/ExpandoObject feature. Might be that it has that in the meantime or you added the code to JsonConfig.

Could you please open up a pull request so I can review that changes and merge it in?

@apexdodge
Copy link
Author

Part of the struggles with the library involved missing DLLs. So what I had
to do was copy over your code manually and put it into a Visual Studio 2012
project.

This is what I see when I load up the solution in VS2010 with no
modifications. Just downloaded as is:

http://snag.gy/zoag0.jpg

I'll see about doing a pull request and putting my changes in there, but I
don't think it'll solve the other above problems.

"By acknowledging the ruse, it is a brilliant ruse"

On Mon, Oct 21, 2013 at 1:30 AM, Dynalon notifications@github.com wrote:

Hi,

I know Newtonsoft.JSON but IIRC I didn't chose it because of the lacking
dynamic/ExpandoObject feature. Might be that it has that in the meantime
or you added the code to JsonConfig.

Could you please open up a pull request so I can review that changes and
merge it in?


Reply to this email directly or view it on GitHubhttps://github.com//issues/3#issuecomment-26693701
.

@apexdodge
Copy link
Author

Yeah, I'm having struggles with the library, so no-go on the pull request,
but here's what I did. The changes were extremely minor so you can make
this change in 5 minutes.

  1. Install-Package Newtonsoft.Json
  2. Remove reference to JsonFx
  3. In Config.cs, replace the following in ParseJson method:

var json_reader = new JsonReader ();
dynamic parsed = json_reader.Read (filtered_json);

with:

dynamic parsed =

JsonConvert.DeserializeObject(filtered_json);

  1. In ConfigObjects.cs remove the code in ToString() and replace it with
    the following:
        return JsonConvert.SerializeObject(this.members);
  1. Clean up using statements.

That's all I did. Let me know if you have more questions.

~Zack

@huntharo
Copy link

I've got a similar patch ready, but I got a few more of the unit tests to pass (up to 25 of 31) and I have it swapping between JsonFx and JSON.Net at compile time.

JSON.Net can return the top-level object as an ExpandoObject, but any nested objects don't appear to be ExpandoObjects and there doesn't seem to be a way to give JSON.Net a clue that that's what you want it to do. Even this doesn't help:

dynamic parsed = JsonConvert.DeserializeObject(filtered_json, new ExpandoObjectConverter());

The long-term solution for that is probably a change to JSON.Net to tell it that you want it to return the entire object tree as ExpandoObjects instead of just the top level.

So, setting that issue aside for the moment, which type of approach do you prefer for switching between JSON libs?

  1. Compile-time directive (e.g. USE_JSON_NET) with the .csproj referencing both libs (but the using's #if'd out based on the directive)

Advantages: simple, hides the incomplete JSON.Net support except for consumers who are willing to compile it on their own or until it's ready for prime time

Disadvantages: doesn't work too well for the long-term, it's hard to put two libs into NuGet without people getting confused about what the difference is between them

  1. Requiring consumers to pass in a shim class that implements an interface with two members. We can provide the example shim classes (they will contain only a few lines) for JsonFx and JSON.Net.

Advantages: removes the direct dependency between JsonConfig and the JSON lib (which avoids a whole slew of problems with versions of the lib, strong names, etc)

Disadvantages: requires this little shim class (how gross is that?)

What do you think?

Harold

@Dynalon
Copy link
Owner

Dynalon commented Jul 21, 2014

I think a compile-time options is currently the best option. JsonConfig isn't on NuGet (because I mostly deveop on linux and mac which has trouble working with nuget) and so I think most users add it as a sourcecode repository.

I am a big fan of DI Containers, but I don't think we should shove a DI Container down to the user's throat for JsonConfig.

@Dynalon
Copy link
Owner

Dynalon commented Jul 21, 2014

Btw. I'd love to have Json.NET PCL so that JsonConfig can become PCL compatible (profile 78).

@huntharo
Copy link

I agree - while a consumer of the lib might choose to use a DI lib, we can't use one because it will cause nightmares for anyone not using the exact same lib / version / pattern of use as we do.

This is my first time working with github (but I used to use and push BitKeeper on people back in 2004, so I'm familiar with the concepts). I'll figure out how to make a pull request from my local branch and then send you one for the compile-time define switch that I have now.

@huntharo
Copy link

Created a pull request for the compile time option to use JSON.Net (initial version, not all unit tests pass... they probably won't pass without a change to JSON.Net):

#13

@Dynalon
Copy link
Owner

Dynalon commented Jul 22, 2014

Until there are failing unit tests I am unsure if and when to merge this.

I got some ideas for API breaking changes that will be the next major versions, but I have a lot on my plate and no ETA on this. Failing tests means that shouldn't go into master without changing the API or reworking the tests.

@huntharo
Copy link

I'm not sure it will be possible to make all the unit tests pass as the unit tests are very specific to how JsonFx deserializes arrays to the strongest-typed array that it can pick, such as string[]. JSON.Net's ExpandoObjectConverter just creates List for each of these lists, which causes casts in the unit tests to ICollection to fail. I can change the casts to ICollection to get more of them to pass, but that doesn't seem like what we want here.

We can create our own version of a JsonConverter, based on the ExpandoObjectConverter in JSON.Net that handles lists differently, but it seems we'd have to take most of the logic from TypeCoercionUtility.cs in JsonFx in order to get JSON.Net to do the same thing as JsonFx, which also doesn't seem right.

What do you think the end goal should be:

  1. Complete Transparency - Try to make JSON.Net return the same thing as JsonFx so that developers can use one or the other without changing a single line of code.

Advantages: Anyone depending on JsonFx can swap over to JSON.Net without changing their code, in theory.

Disadvantages: Developers who have used JSON.Net before will be getting back object types that they might not have expected to get from JSON.Net since we'd be using a custom converter class.

  1. Requiring Some Knowledge of the Parsing Lib - Allow JSON.Net to return it's object types even though those are different than what JsonFx returns.

Advantages: Possible to code without having to import JsonFx logic, developers familiar with either parsing lib will understand the object types that they see returned.

Disadvantages: Swapping parsing libs will require some small, one-time, code changes.

Which do you think we should aim for? I think that requiring knowledge of the parsing lib is ok, but I'd like to know what you think.

Harold

@cozy1
Copy link

cozy1 commented Jul 2, 2015

Amazing that so many are thinking the same thing. Any progress on this?

@pendenaor
Copy link

Why not using a custom converter ?

internal sealed class ExpandoObjectConverter : CustomCreationConverter<ExpandoObject>
{
    public override ExpandoObject Create(Type objectType)
    {
        return new ExpandoObject();
    }

    public override bool CanConvert(Type objectType)
    {
        // in addition to handling ExpandoObject
        // we want to handle the deserialization of dict value
        // which is of type object
        return objectType == typeof(object) || base.CanConvert(objectType);
    }

    public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
    {
        if (reader.TokenType == JsonToken.StartObject
            || reader.TokenType == JsonToken.Null)
            return base.ReadJson(reader, objectType, existingValue, serializer);

        // if the next token is not an object
        // then fall back on standard deserializer (strings, numbers etc.)
        return serializer.Deserialize(reader);
    }
}

(shamelessly adapted from DictionaryConverter)

Use :

    var sample = new
    {
        prop1 = 123.456,
        prop2 = "ldfkdlfm",
        prop3 = (object)null,
        prop4 = new
        {
            subprop1 = "test",
            subprop2 = (string)null,
        }
    };

    var jsonText = JsonConvert.SerializeObject(sample);
    dynamic dynObject = JsonConvert.DeserializeObject(jsonText, typeof(ExpandoObject), new ExpandoObjectConverter());

    Console.WriteLine("prop1=" + dynObject.prop1);
    Console.WriteLine("prop2=" + dynObject.prop2);
    Console.WriteLine("prop3=" + dynObject.prop3);
    Console.WriteLine("prop4 type=" + dynObject.prop4.GetType());
    Console.WriteLine("prop4.subprop1=" + dynObject.prop4.subprop1);
    Console.WriteLine("prop5.subprop2=" + dynObject.prop4.subprop2);    

@pendenaor
Copy link

Anyone ?

@thomasd3
Copy link

thomasd3 commented Nov 1, 2017

Is this still happening? I would like to use this lib, but I need json.net
I swapped the lib and changed the code, but as you guys wrote above, the tests are failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants