-
Notifications
You must be signed in to change notification settings - Fork 7
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
Refactor to use amphp and event-store v8 #51
Conversation
@@ -20,7 +21,7 @@ final class InMemoryEmailGuard implements UniqueEmailGuard | |||
|
|||
public function isUnique(string $email): bool |
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.
Shouldn't this return Promise<bool>
now?
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.
yes
examples/Model/User.php
Outdated
function registerUser(callable $stateResolver, RegisterUser $command, UniqueEmailGuard $guard): Promise | ||
{ | ||
if ($guard->isUnique($command->email())) { | ||
return [new UserWasRegistered($command->payload())]; | ||
return new Success(ImmList(new UserRegistered($command->payload()))); | ||
} | ||
|
||
return [new UserWasRegisteredWithDuplicateEmail($command->payload())]; | ||
return new Success(ImmList(new UserRegisteredWithDuplicateEmail($command->payload()))); | ||
} |
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 like that these functions are now async and as such can use async guards. But I'm slightly doubtful about it resolving to an ImmList
of event. In my project I let them return an asynchronous Iterator instead. What is your thought about that?
Note: Code examples assume that UniqueEmailGuard returns a Promise. Also I highly prefer to use the convention of handling error cases first with early return, leaving the happy scenario as main branch so I negated the condition.
function registerUser(callable $stateResolver, RegisterUser $command, UniqueEmailGuard $guard): Iterator
{
return new Producer(function (callable $emit) use ($command, $guard): Generator {
if (! yield $guard->isUnique($command->email())) {
yield $emit(new UserRegisteredWithDuplicateEmail($command->payload()));
return;
}
yield $emit(new UserRegistered($command->payload()));
});
}
Admittedly while pure code should return Amp\Iterator here, I later decided that I don't like the Producer + $emit
syntax so I actually let these functions return Generator<Promise|EventInterface>
. Then there is a wrapper function that transforms it into \Amp\Iterator<EventInterface>
. I consider such syntactic improvement as acceptable over code purity for my project although not sure if it's suitable for an example like this... But I'll show it to you anyway for comparison:
function registerUser(callable $stateResolver, RegisterUser $command, UniqueEmailGuard $guard): Generator
{
if (! yield $guard->isUnique($command->email())) {
yield new UserRegisteredWithDuplicateEmail($command->payload());
return;
}
yield new UserRegistered($command->payload());
}
The wrapper function looks something like this:
function transform(Generator $generator): Iterator
{
return new Producer(function (callable $emit) use ($generator) {
foreach ($generator as $eventOrPromise) {
if ($eventOrPromise instanceof Promise) {
yield $eventOrPromise;
} else {
yield $emit($eventOrPromise);
}
}
});
}
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.
awesome, thanks !!
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.
@enumag I ran into issues with this. On yield $eventOrPromise;
the promises is yielded, but not assigned to the variable in the handler function.
$state = yield $stateResolver();
results in $state = null
.
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.
Ah yeah... sorry, I copy pasted a wrong function. I'll post the correct one tomorrow.
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.
thanks
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.
maybe can you directly submit a PR to this branch?
@enumag changes done |
@prolic the implementation looks really great. Hopefully I find some time to try it out. I'd also look at a way to connect Event Engine Cockpit to it. |
src/Kernel.php
Outdated
foreach ($specification->handle(stateResolver($eventStore, $specification, $readBatchSize)) as $eventOrPromise) { | ||
if ($eventOrPromise instanceof Promise) { | ||
yield $eventOrPromise; | ||
} else { | ||
yield $emit($eventOrPromise); | ||
} | ||
} |
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.
foreach ($specification->handle(stateResolver($eventStore, $specification, $readBatchSize)) as $eventOrPromise) { | |
if ($eventOrPromise instanceof Promise) { | |
yield $eventOrPromise; | |
} else { | |
yield $emit($eventOrPromise); | |
} | |
} | |
$generator = $specification->handle(stateResolver($eventStore, $specification, $readBatchSize)); | |
while ($generator->valid()) { | |
$eventOrPromise = $generator->current(); | |
if ($eventOrPromise instanceof Promise) { | |
$generator->send(yield $eventOrPromise); | |
} else { | |
yield $emit($eventOrPromise); | |
$generator->next(); | |
} | |
} |
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.
@prolic This should work I think. ;-)
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.
Awesome, thank you.
I added new issues for snapshot support and documentation. I'll merge this to master. |
[WIP] knitted with a hot needle - some changes / bugfixes may be required before merge
How to use this?
CommandSpecification
, see f.e.UserSpecification
in the examples dirregister_and_change_username.php
in the examples dirTodos:
Support Event Publishingping @codeliner @enumag
As the kernel returns an immutable list of all emitted events, you can simply publish them yourself however you like, that's event a dedicated event publisher will not be part of this. Also in most cases you would rely on event store projections to be running anyway.