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

Migrating to System.Text.Json (#9727) #9728

Closed
wants to merge 2 commits into from
Closed

Migrating to System.Text.Json (#9727) #9728

wants to merge 2 commits into from

Conversation

just-ero
Copy link

Description

This PR addresses #9727.

Gradually, references to Newtonsoft.Json shall be converted to System.Text.Json.

Notes

During my work on this, I may discover and run into some oddities that I need the input of the team on. Should I simply add comments as these question arise? This is my first time working on such a big PR in such a big project.

@just-ero
Copy link
Author

There are significant compatibility issues with the Docfx.Common.JsonUtility class. Additionally, this class is public API. I'm not fully aware of .NET's policy on breaking changes, but I fear that this cannot be changed due to the large impact this may have on users. There is also no replacement for Newtonsoft.Json.Formatting.

@just-ero
Copy link
Author

I am beginning to change the couple JsonConverters in the library. There is no type safety around the FileMetadataPairsItem class. FileMetadataPairsItem.Value mentions that this shouldn't matter, sicne it is only used for serialization. This makes me wonder why it needs to be present at all. Additionally, could we still add type safety around it?

@yufeih
Copy link
Contributor

yufeih commented Feb 24, 2024

Should I simply add comments as these question arise?

Sounds good.

I'm not fully aware of .NET's policy on breaking changes, but I fear that this cannot be changed due to the large impact this may have on users

As you might have noticed, one of the biggest challenges, is that this project is big with quite a few "tech debts" and the need to support compatibility. This project does not abide .NET framework's breaking change policy, but still breaking changes for good needs to be introduced in a controlled way.

Since this is going to be a non-trivial effort with breaking changes along the way, I created a feature/stj branch for the development work. You can then send smaller PRs against that branch without affecting the main branch.

Docfx.Common.JsonUtility

We may not be able to retire it in one go because so many parts of the project depend on it. It might be easier to migrate over piece by piece, such as:

  1. Support S.T.J for FileItem
  2. Support S.T.J for MetadataJsonConfig
  3. Switch MetadataJsonConfig serialization to S.T.J
  4. Make MetadataJsonConfig public and allow the GenerateManagedReferenceYamlFiles to take config object

FileMetadataPairsItem: Additionally, could we still add type safety around it?

Yes we should replace it with a strongly-typed serialization-only data model for config. One challenge is to come up with a pattern to support short-hand form which isn't directly supported by S.T.J without union types in C#. We can start trying out a few patterns and see which works the best in the work to support S.T.J for FileItem. One of the pattern I'm using is OneOf for API Page with OneOfJsonConverterFactory. @filzrev was also looking to auto gen config docs from these models.

@just-ero
Copy link
Author

I created a feature/stj branch for the development work. You can then send smaller PRs against that branch without affecting the main branch.

This sounds like a good idea. Should I close this PR for now? How would you prefer I structure the PRs? I imagine that some checks will fail regardless until everything falls into place.

@filzrev
Copy link
Contributor

filzrev commented Feb 26, 2024

Currently most of System.Text.Json attributes are added for JSON schema generation (#8968) supports.
So it's not tested for serialize/deserialize by using System.Text.Json.

I thought it's need to add serialize/deserialize roundtrip tests first.

As far as I knows.
There some types that can't deserialize by System.Text.Json default settings.

  • JsonConverter for System.Text.Json is not missing. It need to add custom JsonConverters.
  • It need to add default public parameterless constructor for some types(e.g. FileItems) to support deserialization.
  • XRefSpec class can't deserialize by System.Text.Json.
    It needs to add custom JsonConverter(ObjectToInferredTypesConverter)
  • CompositeDictionary type seems not support JSON deserialization. (Both Newtonsoft.Json/System.Text.Json)

@yufeih
Copy link
Contributor

yufeih commented Feb 27, 2024

@just-ero Please create new PRs and close this one, but still make sure the new PRs pass validation. We need to do this large refactoring in small incremental shippable steps.

@just-ero just-ero closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants