Conversation
Remove [RequiresUnreferencedCode] and [RequiresDynamicCode] from the IEventSerializer interface and all 43 consumer files across the codebase. Extract the reflection-based DefaultEventSerializer to a new Eventuous.Serialization.Json package. The core serialization interface and DefaultStaticEventSerializer are now fully AOT-clean. - Clean IEventSerializer interface (no AOT attributes) - Add EventSerializer static holder replacing DefaultEventSerializer.Instance - Move DefaultEventSerializer to Eventuous.Serialization.Json package - Remove AOT attributes from Persistence, Producers, Subscriptions, Application, and all integration packages (KurrentDB, Sql.Base, RabbitMQ, Sqlite) - Delete 6 Constants.cs files with DynamicSerializationMessage - Update all samples and tests to use new API Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review by Qodo
1.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19ac3f8952
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| public static void SetDefaultSerializer(IEventSerializer serializer) => Instance = serializer; | ||
| // Auto-register as default if none is set | ||
| EventSerializer.SetDefault(this); |
There was a problem hiding this comment.
Avoid resetting global serializer in constructor
Calling EventSerializer.SetDefault(this) inside DefaultEventSerializer's constructor makes every new instance mutate global process state, which is a behavioral regression from the previous implementation where construction was side-effect free unless callers explicitly set the default. This can break apps that create multiple serializers (for different ITypeMapper/options scopes) because the last constructed instance silently changes what all ... ?? EventSerializer.Default call sites use; at minimum this should only set the default when none is configured, as the comment indicates.
Useful? React with 👍 / 👎.
src/Core/src/Eventuous.Serialization.Json.Dynamic/DefaultEventSerializer.cs
Show resolved
Hide resolved
src/Azure/test/Eventuous.Tests.Azure.ServiceBus/ConvertEventToMessage.cs
Show resolved
Hide resolved
Test Results 60 files + 40 60 suites +40 39m 23s ⏱️ + 25m 14s Results for commit f1dae72. ± Comparison against base commit a604f77. This pull request removes 5 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
- Add TrySetDefault to EventSerializer using Interlocked.CompareExchange so DefaultEventSerializer constructor doesn't overwrite an existing serializer configuration - Add module initializer to Eventuous.Tests.Subscriptions.Base so integration tests (Azure, Kafka, KurrentDB) have EventSerializer.Default configured before use Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These test projects use EventSerializer.Default without configuring it. Add module initializers with DefaultEventSerializer to fix CI failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use EventSerializer.SetDefault() explicitly instead of relying on the constructor's hidden TrySetDefault side effect. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Dynamic The old name was ambiguous since DefaultStaticEventSerializer also uses System.Text.Json. The new name clarifies this package contains the reflection-based (non-AOT) dynamic serializer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrite the default serializer section to document both DefaultStaticEventSerializer (AOT, recommended) and DefaultEventSerializer (reflection-based, separate package). Update API references from the old DefaultEventSerializer.SetDefaultSerializer to the new EventSerializer.SetDefault. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deploying eventuous-main with
|
| Latest commit: |
f1dae72
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://058533bf.eventuous-main.pages.dev |
| Branch Preview URL: | https://feature-aot-serializer-clean.eventuous-main.pages.dev |
Summary
[RequiresUnreferencedCode]and[RequiresDynamicCode]fromIEventSerializerinterface and all 43 consumer files across core and integration packagesDefaultEventSerializerto newEventuous.Serialization.Jsonpackage (intentionally not AOT-compatible)EventSerializerstatic holder class replacingDefaultEventSerializer.Instance/SetDefaultSerializer()DefaultStaticEventSerializerand core serialization interface are now fully AOT-clean with zero suppression attributesBreaking changes
DefaultEventSerializermoved fromEventuous.SerializationtoEventuous.Serialization.JsonpackageDefaultEventSerializer.Instancereplaced byEventSerializer.DefaultDefaultEventSerializer.SetDefaultSerializer()replaced byEventSerializer.SetDefault()DefaultStaticEventSerializerfor AOT orDefaultEventSerializerfrom the new package)Test plan
🤖 Generated with Claude Code