-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add Message header interface for more decoupling and type safety #496
Conversation
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.
That's kind of how I thought it too 👍
Hello 👋 here is the most recent benchmark result:
This comment gets update everytime a new commit comes in! |
1684cb5
to
d2f090a
Compare
Do we even need the |
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.
Nice work 👍 I still have a few wishes :D
@@ -49,6 +50,10 @@ public function setUp(): void | |||
$this->store = new DoctrineDbalStore( | |||
$connection, | |||
DefaultEventSerializer::createFromPaths([__DIR__ . '/BasicImplementation/Events']), | |||
DefaultHeadersSerializer::createFromPaths([ |
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.
We should add the default headers internally, so the developer no need to know, that he need a specific path. And we should do this without a path: we should add our headers directly. auto-discovery should only used in user space.
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.
Hm, I'm not sure about that. Due to that it's more masked for the user that this is happening and if he implements his own version of the HeadersSerializer
he needs to know this internal logic in our DefaultHeadersSerializer
.
But if we we do this then i think we should keep it as auto discover due two reasons:
- We cannot forget to add headers then there
- No need for extra logic
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.
-
I don't think it's a relevant point that you forget it, since we'll hopefully cover it with tests so that everything fits.
-
We can push the logic further down to
MessageHeaderRegistry
.
class MessageHeaderRegistry
{
// ...
/** @param array<string, class-string<Header>> $headerNameToClassMap */
public static function withInternalHeaders(array $headerNameToClassMap): self
{
$internalHeaders = [
'aggregate' => AggregateHeader::class,
// ...
];
return new self($headerNameToClassMap + $internalHeaders);
}
}
This means it cannot be forgotten in the serializer.
To the point that no extra logic is necessary: It's an array merge, there isn't that much extra logic. On the other hand, we have an IO search in our code, which is not necessary because we know exactly where the classes are.
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.
Then we are duplicating the naming code 🤔
So i think, if we move it, it should land in AttributeMessageHeaderRegistryFactory
but still feels weird.
$classes = (new ClassFinder())->findClassNames($paths);
$classes = array_merge([AggregateHeader::class, ...], $classes);
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.
Then we are duplicating the naming code 🤔
I do not understand that.
For me, MessageHeaderRegistry
would be the more correct place. I wouldn't put it any further down.
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.
If you mean the attributes. I would remove these from the internal headers because the attributes are only used for configuration. And since we define the internal headers differently, they have no meaning there. The attribute is for user space.
aa43408
to
42ab335
Compare
49ecc51
to
5c90bb4
Compare
No description provided.