Conversation
Co-Authored-By: Skyler Katz <skylerkatz@hey.com>
Co-Authored-By: Skyler Katz <skylerkatz@hey.com>
Co-Authored-By: Skyler Katz <skylerkatz@hey.com>
inxilpro
left a comment
inxilpro
left a comment
There was a problem hiding this comment.
@DanielCoulbourne @skylerkatz I just did a pass on this and added some notes in prep for Monday.
| /** @param Event[] $events */ | ||
| public function write(array $events): bool; | ||
|
|
||
| public function summarize(State ...$states): AggregateStateSummary; |
There was a problem hiding this comment.
I'm not 100% sure this needs to be on the StoresEvents interface. I think maybe it needs access to some store data, but it seems a little odd for it to live here.
src/Lifecycle/StateManager.php
Outdated
| // FIXME: Swap out existing state manager, push all related states into new state manager | ||
| // FIXME: run all the event on them, swap them out |
There was a problem hiding this comment.
This is the correct approach, I think. Ultimately, we just need an isolated state manager to handle everything and then pull those values over into the global state manager after it's done.
src/Lifecycle/StateManager.php
Outdated
| $min = $last_event_ids->min() ?? PHP_INT_MIN; | ||
| $max = $last_event_ids->max() ?? PHP_INT_MIN; | ||
|
|
||
| // If all states have had this or future events applied, just ignore them | ||
| if ($min >= $event->id && $max >= $event->id) { | ||
| return false; | ||
| } | ||
|
|
||
| // We should never be in a situation where some events are ahead and | ||
| // others are behind, so if that's the case we'll throw an exception | ||
| if ($max > $event->id && $min <= $event->id) { | ||
| throw new RuntimeException('Trying to apply an event to states that are out of sync.'); | ||
| } |
There was a problem hiding this comment.
This is all to account for cases where one state is ahead of another. We may want to decide that in the beginning we just don't support that…?
| use InvalidArgumentException; | ||
| use Thunk\Verbs\State; | ||
|
|
||
| class StateIdentity |
There was a problem hiding this comment.
There are too many different things that can point to a state. This class exists to normalize them all into one thing for ease of use around the codebase.
| foreach ($this->states as $type => $states) { | ||
| foreach ($states as $id => $state) { | ||
| if (in_array($id, $ids)) { | ||
| uniqid($this->states[$type][$id]); |
There was a problem hiding this comment.
lol, I think this should be unset not uniqid
| /* | ||
| * The Problem(s) | ||
| * | ||
| * FIRST PROBLEM: | ||
| * - We try to load state1, but we don't have an up-to-date snapshot | ||
| * - StateManager::load tries to reconstitute state from events | ||
| * - One of those Event::apply methods load state2 | ||
| * - Best case scenario: we reconstitute state2 before continuing | ||
| * - Worst case scenario: reconstituting state2 tries to reconstitute state1, and we're in an infinite loop | ||
| * - (if no loop) state1 continues to reconstitute, but it's acting with state2 FULLY up-to-date, not | ||
| * just up-to-date with where state1 happens to be | ||
| * | ||
| * TO TEST FIRST PROBLEM: | ||
| * - Event1 adds State1::counter to State2::counter and increments State2::counter | ||
| * - Event2 increments State2::counter | ||
| * | ||
| * SECOND PROBLEM: | ||
| * - We try to load state1, but we don't have an up-to-date snapshot | ||
| * - StateManager::load tries to reconstitute state from events | ||
| * - One of those Event::apply methods requires state1 and state2, so we need to load state2 | ||
| * - Reconstituting state2 re-runs the same apply method on state2 before also running it on state1 | ||
| * - Double-apply happens | ||
| */ |
There was a problem hiding this comment.
I think we should probably start here and review whether these are actually the problems and if so confirm that the tests effectively test them.
| $state_id = data_get($source, 'state_id'); | ||
| $state_type = data_get($source, 'state_type'); | ||
|
|
||
| if (is_int($state_id) && is_string($state_type)) { |
There was a problem hiding this comment.
| if (is_int($state_id) && is_string($state_type)) { | |
| if ( | |
| ( | |
| \Illuminate\Support\Str::isUlid($state_id) | |
| || \Illuminate\Support\Str::isUuid($state_id) | |
| || is_int($state_id) | |
| ) | |
| && is_string($state_type) | |
| ) { |
This is formatted terribly, but I think we need to check for more than just an int for the $state_id
3 participants
Just posting this as a draft for discussion.