-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[v5] Recursive persistence #3743
Conversation
🦋 Changeset detectedLatest commit: 7489eb6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7489eb6:
|
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
* Initialize the `Interpreter`'s state eagerly * Fixed parent assignment on children * remove what was supposed to be removed * pass parent to the spawner within assign * pass parent to the initial context factory * Reassign the old state after stopping the machine in React hooks * fixed things * fixed tests * move a comment back to its original place to minimize the diff
@@ -61,7 +72,7 @@ export function fromObservable<T, TEvent extends EventObject>( | |||
return state; | |||
case stopSignalType: | |||
state.canceled = true; | |||
state.subscription!.unsubscribe(); | |||
state.subscription?.unsubscribe(); |
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.
when the subscription can be missing when receiving stopSignalType
? This sounds like an impossible state
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, stopSignalType
is coming from an external .stop()
, hmmm. It might be needed to have some guard in place here for this case - but I wonder about the exact scenario related to this because it seems that we don't have a test for it.
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 restore the observable actor and it's already completed, subscription
will be undefined.
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, but this line of code is specifically called at some point - and we should have a test case that would hit this line of the code when the subscription is absent (and we dont have such rn)
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 can but isn't ?.
safer? I'm not sure what should be tested here since the code won't fail regardless of if it's defined or not. We would be testing the change from !
to ?
which is sorta meaningless.
Do we need to test all the places where we have ?.
?
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 can but isn't ?. safer?
It is. But it's also potentially a little bit of redundant code, runtime overhead etc. I find defensive programming often obscures the meaning of the code and covers up potential bugs.
For example, from my PoV the important question here is - can the stopSignalType
even be received when state.subscription === undefined
? If the answer is "no" but it happens anyway then this ?.
will "swallow" the logical/integration bug and we won't even learn about it if it ever happens. If the answer is "yes" then we can write a test case that would exercise this scenario and we can verify there that nothing throws (and then we will be sure that ?.
is actually needed!)
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 reverted this
@@ -76,6 +87,11 @@ export function fromObservable<T, TEvent extends EventObject>( | |||
}; | |||
}, | |||
start: (state, { self }) => { | |||
if (state.status === 'done') { | |||
// Do not restart a completed observable |
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.
what about a canceled actor? a canceled actor is pretty much "completed" (even if not successfully). OTOH, a canceled actor might not get persisted at all, right? Since it should be cleaned up from state.children
.
This also brings an important issue here - spawned actors (including stopped ones) might be part of the context
. We are not doing anything about this and likely we should. They should be rehydrated in the same way as the invoked ones - the fact that they live in context
complicates the matter though :s
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.
For spawned actors, follow-up PR?
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.
👍
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
This PR adds recursive persistence for actors:
You should no longer pass a persisted state into
actor.start(...)
, since it doesn't make sense to provide the persisted state that late (after the actor is already created) and it makes the initial state inconsistent: