Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add System.Json #9897

Merged
merged 7 commits into from
Jul 8, 2016
Merged

Add System.Json #9897

merged 7 commits into from
Jul 8, 2016

Conversation

stephentoub
Copy link
Member

System.Json.dll is part of Silverlight and doesn't ship in the .NET Framework, nor do we encourage its usage. But, it's available and used with Mono, and in order to enable compatibility between Mono and .NET Core, we're adding it as an available assembly.

This PR ports the Mono implementation over to corefx. This includes porting the tests, adding packaging, cleaning up the source, etc. I've not done any performance work, increased code coverage of the tests, etc. This is mainly about making the APIs available. We can tweak is subsequently as is necessary.

cc: @danmosemsft, @KrzysztofCwalina, @ericstj, @ianhays

"System.Resources.ResourceManager": "4.0.0",
"System.Runtime": "4.0.20",
"System.Runtime.Extensions": "4.0.10",
"System.Runtime.Handles": "4.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What actually uses handles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing. I copied this from another assembly and never trimmed it. I can probably get rid of most of these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Targeting netstandard1.0 with the following dependencies worked for me:

    "netstandard1.0": {
      "dependencies": {
        "System.Collections": "4.0.11",
        "System.IO": "4.1.0",
        "System.Linq": "4.1.0",
        "System.Runtime": "4.1.0",
        "System.Runtime.Extensions": "4.1.0",
        "System.Resources.ResourceManager": "4.0.1"
      }
    }

@ericstj
Copy link
Member

ericstj commented Jul 7, 2016

See ericstj@9a1cf68 & ericstj@2400019. If you make me a collaborator I'll push to your branch.

@ericstj
Copy link
Member

ericstj commented Jul 7, 2016

@mhutch, would like your opinion on ericstj@2400019. This has us deferring to the inbox implementation of System.Json.dll on xamarin profiles. Otherwise we could give Xamarin profiles the implementation from the package itself.

ericstj added 2 commits July 7, 2016 15:14
Build and package as a netstandard1.0 assembly.  Clean up project file.

namespace System.Json
{
public class JsonObject : JsonValue, IDictionary<string, JsonValue>, ICollection<JsonPair>
Copy link
Member

@ericstj ericstj Jul 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this expose more interfaces like IReadOnly*, same q for all types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider it, but my understanding is we're only really adding this assembly for compatibility with Mono (it doesn't exist on desktop), and that we'd suggest developers write new JSON-related code using Json.NET and/or https://github.com/dotnet/corefxlab/tree/master/src/System.Text.Json depending on how that experiment turns out, rather than writing new code using this library. So I'm not sure how much effort we want to expend on adding APIs, interface implementations, etc. We could certainly look into it, though, but I'd like that to be a separate effort it if happens.

@danmoseley
Copy link
Member

yay for a new assembly! thank you

@danmoseley
Copy link
Member

:shipit:

@ericstj
Copy link
Member

ericstj commented Jul 7, 2016

Yeah, LGTM too. For the question about adding more interfaces I think that could be addressed separately.

@stephentoub
Copy link
Member Author

Thanks, guys, and Eric, thank you very much for helping to fix the packaging.

@stephentoub stephentoub merged commit 3f4c1df into dotnet:dev/api Jul 8, 2016
@stephentoub stephentoub deleted the systemjson branch July 8, 2016 00:40
@mhutch
Copy link

mhutch commented Jul 8, 2016

I would definitely prefer to defer to our implementation on Mono-based frameworks in order to fuse the types (e.g. what happens if someone gets a System.Json object from a netstandard library and passes it to an Xamarin.iOS library) and to shrink our deployed app size by ensuring we don't deploy multiple implementations.

<Project ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Build">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<AssemblyVersion>4.0.0.0</AssemblyVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this version needs to match the version xamarin has inbox (2.0.5.0). We should test that.

@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants