Migrate to system.text.json

[kblok]

Description

Work in progress. Early review is welcome

[jnyrup]

I didn't look at the uses of attributes on types

[jnyrup]

Suggestion: statically cache JsonSerializerOptions due to CA1869

[kblok]

done!

[jnyrup]

Seems we can remove these.
They are both transitively included by PuppeteerSharp.csproj's dependency on Microsoft.Extensions.Logging

[kblok]

done!

[jnyrup]

It seems the two methods using _stringToEnumCache have slightly different delegates to GetOrAdd

Perhaps out-of-scope for this PR, but we could use the trick from https://mariusgundersen.net/type-dictionary-trick/ to avoid constructing the ConcurrentDictionary and the lookup into it.

internal static class EnumHelper
{
    public static TEnum ToEnum<TEnum>(this string value)
        where TEnum : struct, Enum => StringToEnum<TEnum>.Cache[value];

    public static string ToValueString<TEnum>(this TEnum value)
        where TEnum : struct, Enum => EnumToString<TEnum>.Cache[value];

    public static TEnum FromValueString<TEnum>(string value)
        where TEnum : struct, Enum
    {
        if (StringToEnum<TEnum>.Cache.TryGetValue(value, out var enumValue))
        {
            return enumValue;
        }

        var defaultEnumAttribute = typeof(TEnum).GetCustomAttribute<DefaultEnumValueAttribute>();
        if (defaultEnumAttribute != null)
        {
            return (TEnum)(object)defaultEnumAttribute.Value;
        }

        throw new ArgumentException($"Unknown value '{value}' for enum {typeof(TEnum).Name}");
    }

    private static class StringToEnum<TEnum>
        where TEnum : struct, Enum
    {
        public static readonly IReadOnlyDictionary<string, TEnum> Cache = Compute();

        private static Dictionary<string, TEnum> Compute()
        {
            var names = Enum.GetNames(typeof(TEnum));
            var values = (TEnum[])Enum.GetValues(typeof(TEnum));
            var dictionary = new Dictionary<string, TEnum>(StringComparer.OrdinalIgnoreCase);
            for (var i = 0; i < names.Length; i++)
            {
                dictionary.Add(names[i], values[i]);
                var memberValue = typeof(TEnum).GetField(names[i]).GetCustomAttribute<EnumMemberAttribute>()?.Value;
                if (memberValue != null)
                {
                    dictionary[memberValue] = values[i];
                }
            }

            return dictionary;
        }
    }

    private static class EnumToString<TEnum>
        where TEnum : struct, Enum
    {
        public static readonly IReadOnlyDictionary<TEnum, string> Cache = Compute();

        private static Dictionary<TEnum, string> Compute()
        {
            var names = Enum.GetNames(typeof(TEnum));
            var dictionary = new Dictionary<TEnum, string>();
            foreach (var t in names)
            {
                var field = typeof(TEnum).GetField(t);
                var valueName = field.GetCustomAttribute<EnumMemberAttribute>()?.Value ?? t;
                var fieldValue = (TEnum)field.GetValue(null);
                dictionary[fieldValue] = valueName;
            }

            return dictionary;
        }
    }
}

[kblok]

Follow up PR :)

[jnyrup]

Don't we always want to use SystemTextJsonSerializationContext.Default?

[kblok]

I think that with the conditional build that makes sense.

[kblok]

It seems that with this change, I would be forcing people to use declared values in the Context.

In fact, I think that if we want Evaluate calls to work on AOT, you might need to use basic types or provide an extended context.

[kblok]

I confirm this. If the consumer doesn't enforce this. There is no need to do this.
If the consumer does enable this. They will also have to provide a Context for any extra types they use.

[jnyrup]

Fine with me 👍
As a follow-up later, we could perhaps change it such that all types that don't rely on user types, are (de)serialized using a source-generated JSON serializer when using the .NET8 target.

[jnyrup]

Do we need to write indented JSON?

[kblok]

not at all :)

[kblok]

@jnyrup some updates:

We can't set the JsonTypeInfoResolver by default. That would force the user to do the same thing.
If you build your project with AOT, and you use some special types when you call some Evaluate function. You have to provide a serialization context using Puppeteer.ExtraJsonSerializerContext. You should do that as a first step. After that, the DefaultJsonSerializerSettings will be cached.

I added a demo in the console app.

Thoughts?

[jnyrup]

I don't have that much experience with AOT and JsonTypeInfoResolver, so can't provide the best feedback here.

If I have two different types A and B and also two different source-generated JsonSerializerContexts, does the current design allow me two use them without using Puppeteer.ExtraJsonSerializerContext?

[kblok]

If I have two different types A and B and also two different source-generated JsonSerializerContexts, does the current design allow me two use them without using Puppeteer.ExtraJsonSerializerContext?

You could pass the result a JsonTypeInfoResolver.Combine() to Puppeteer.ExtraJsonSerializerContext.