-
Notifications
You must be signed in to change notification settings - Fork 180
Conversation
@josephdecock can you take a look at this one too? |
@josephdecock could you take a look at the latest iteration? Thanks! |
Ping |
I was still interested in why we need multi-targeting, so I pulled this branch down and tried compiling without the net6.0 TFM. I got this warning:
So, should we add that condition? I also don't see why we don't just say |
This is needed to make this assembly trimmable
For future reference: this seems to be unnecessary, because we aren't targeting more specific platforms. If we needed to target something like net6.0-windows, then these more complex conditions would make sense, but we don't, so we can keep it simple. |
public static string Serialize<T>(T logObject) | ||
{ | ||
return Enabled ? JsonSerializer.Serialize(logObject, JsonOptions) : "Logging has been disabled"; | ||
} | ||
return Enabled ? | ||
JsonSerializer.Serialize(logObject, (JsonTypeInfo<T>)SourceGenerationContext.Default.GetTypeInfo(typeof(T))) : | ||
"Logging has been disabled"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change because the LogSerializer can now only serialize the types in the source generator context. That works fine for our internal usage of the LogSerializer, but the LogSerializer is a public type. Anyone who is using this helper to serialize anything else will get an NRE when GetTypeInfo returns null.
Options:
- Make the LogSerializer internal.
- Don't enable trimmable
- Make this method private, but keep the original
Serialize(object logObject)
as a public overload with[RequiresUnreferencedCode]
Changing the log serializer to support trimming means that we need to add the types that we will serialize into a serialization context. Unfortunately, the serializer is a public type that serializes arbitrary objects today. This is incompatible with trimming. The solution is to mark the existing API as [RequiresExternalCode], which gives trimmed callers of the API a warning, and to add overloads that accept the types that are in the generation context.
With this change I am annotating the core assembly as IsTrimmable. This enables warnings for any known issues that may arise when publishing a final project as trimmed. The only warnings present were the use of runtime reflection code to perform the Json serialization. I converted over to using the Json Source Generator to move this logic from runtime to compile time (a net performance win too).
fixes #390