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

SystemTextJson: Support automatic TypeSafeEnum/Union converter selection #69

Merged
merged 14 commits into from Jan 5, 2022

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented Jan 4, 2022

While the general advice, esp in README.md is that Explicit is Better than Implicit and having a JsonConverter tag per type explicitly indicating for all DU types whether to:
a) use the TypeSafeEnumConverter to map it to a string if possible
b) use the more general UnionConverter if that's not the case
c) trigger System.Text.Jsons fail fast guard if neither is present

UnionOrTypeSafeEnumConverterFactory implements a behavior that dynamically selects from the first two.

The primary risk (aside from the fact that its no longer clear per Union type in the code that it is participating in JSON encoding/decoding) is that a type that starts off using TSEC (and rendering as a JSON string) suddenly flips to requiring/rendering a JSON object. The thought of some automatic upconversion logic does not give me warm fuzzies.

For more details and discussion of the tradeoffs, see the associated STJ feature RFC

@bartelink bartelink mentioned this pull request Jan 5, 2022
@bartelink
Copy link
Collaborator Author

bartelink commented Jan 5, 2022

@Tarmil As a matter of interest, does FSharp.SystemTextJson offer a way to define a convention like UnionOrTypeSafeEnumConverterFactory implements? (And/or do you have any thoughts as to whether this should ever be done and/or a cleaner way to structure this?)

i.e. as covered in /tests/FsCodec.SystemTextJson.Tests/AutoUnionTests.fs:
a) Convert to a JSON string iff all cases are nullary
b) Convert with an internal case field where that's not possible

Base automatically changed from stateful-serdes to master January 5, 2022 15:38
@bartelink bartelink merged commit 49cbe22 into master Jan 5, 2022
@bartelink bartelink deleted the auto-union branch January 5, 2022 15:39
@Tarmil
Copy link

Tarmil commented Jan 6, 2022

@bartelink The "iff all cases are nullary" aspect is not possible right now in FSharp.SystemTextJson, but I would be open to adding it as an option.

@bartelink
Copy link
Collaborator Author

Thanks @Tarmil. For current projects, I personally am happy with FsCodec's featureset and the fact that it is a small codebase. But knowing that there's a way more complete superset of it out there, should I wish to vary the representation from a single (pair of) hard wired ones etc is definitely a big part of that happiness.

I'm also not 100% decided on whether it's a misfeature in practice - it took me a long time to talk myself into risking providing a feature that can fail at run time for any DU when it crosses the divide from all nullary to/from one or more cases with bodies during the lifetime of a project, especially when there are no hints nearby to a random developer making what they perceive as a tiny change.

I guess if anyone also wants it, they'll ask and/or send a PR in due course ;)

@bartelink
Copy link
Collaborator Author

bartelink commented Jan 7, 2022

To add this "autoUnion mode" into FsCodec.NewtonsoftJson would involve:

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.

None yet

2 participants