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

JsonSerializer support for JsonDocument types in source generation mode #59954

Closed
Tracked by #63762
layomia opened this issue Oct 4, 2021 · 8 comments · Fixed by #60236
Closed
Tracked by #63762

JsonSerializer support for JsonDocument types in source generation mode #59954

layomia opened this issue Oct 4, 2021 · 8 comments · Fixed by #60236
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json Cost:S Work that requires one engineer up to 1 week Priority:1 Work that is critical for the release, but we could probably ship without source-generator Indicates an issue with a source generator feature
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented Oct 4, 2021

JsonSerializer supports (de)serialization of JsonDocument instances. We should add the same support in source-generation mode by exposing the JsonDocumentConverter via a new JsonMetadataServices API.

@layomia layomia added area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors labels Oct 4, 2021
@layomia layomia added this to the 7.0.0 milestone Oct 4, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 4, 2021
@ghost
Copy link

ghost commented Oct 4, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

JsonSerializer supports (de)serialization of JsonDocument instances. We should add the same support in source-generation mode by exposing the JsonDocumentConverter via a new JsonMetadataServices API.

Author: layomia
Assignees: -
Labels:

area-System.Text.Json, up-for-grabs

Milestone: 7.0.0

@layomia layomia added bug source-generator Indicates an issue with a source generator feature and removed untriaged New issue has not been triaged by the area owner labels Oct 4, 2021
@strobelt
Copy link
Contributor

I can do this!

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 10, 2021
@strobelt
Copy link
Contributor

I created PR #60236 to fix this. Let me know if I did it right and if there are any improvements I can do!

@eiriktsarpalis
Copy link
Member

Does this need API review before we mark up-for-grabs?

@layomia
Copy link
Contributor Author

layomia commented Oct 11, 2021

Yes the following API needs to be reviewed and approved before we can merge #60236. My apologies @strobelt.

namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {
        public JsonConverter<JsonElement> JsonElementConverter { get; }
+       public JsonConverter<JsonDocument> JsonDocumentConverter { get; }
    }
}

@layomia layomia added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed help wanted [up-for-grabs] Good issue for external contributors labels Oct 11, 2021
@layomia layomia self-assigned this Oct 11, 2021
@strobelt
Copy link
Contributor

No problem, @layomia!
Just to be clear: should I make any corrections to the PR I created? Or will there be some workk before we create any code for this? I saw the ocmment you added tomy PR and donn't know if I should keep working on it or abandon it.

Thanks for the help!

@eiriktsarpalis
Copy link
Member

I think we can use your existing PR @strobelt. We can look at getting the API approved concurrently.

@terrajobst
Copy link
Member

terrajobst commented Oct 12, 2021

Video

  • Looks good as proposed
namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {
        // Existing
        // public JsonConverter<JsonElement> JsonElementConverter { get; }
        public JsonConverter<JsonDocument> JsonDocumentConverter { get; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 12, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 15, 2021
@eiriktsarpalis eiriktsarpalis added Cost:S Work that requires one engineer up to 1 week Priority:1 Work that is critical for the release, but we could probably ship without help wanted [up-for-grabs] Good issue for external contributors and removed help wanted [up-for-grabs] Good issue for external contributors labels Feb 7, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 20, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 20, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json Cost:S Work that requires one engineer up to 1 week Priority:1 Work that is critical for the release, but we could probably ship without source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants