Skip to content
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

[core] Synchronizers #5071

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[core] Synchronizers #5071

wants to merge 12 commits into from

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Sep 12, 2024

Added support for synchronizers in XState, allowing state persistence and synchronization across different storage mechanisms.

  • Introduced Synchronizer interface for implementing custom synchronization logic
  • Added sync option to createActor for attaching synchronizers to actors
import { createActor } from 'xstate';
import { someMachine } from './someMachine';
import { createLocalStorageSync } from './localStorageSynchronizer';

const actor = createActor(someMachine, {
  sync: createLocalStorageSync('someKey')
});

Copy link

changeset-bot bot commented Sep 12, 2024

🦋 Changeset detected

Latest commit: 03854c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
xstate Minor
@xstate/graph Major
@xstate/react Major
@xstate/solid Major
@xstate/svelte Major
@xstate/vue Major

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

@davidkpiano davidkpiano marked this pull request as ready for review September 30, 2024 05:00
const restoredSnapshot =
this.logic.restoreSnapshot?.(rawSnapshot, this._actorScope) ??
rawSnapshot;
this.update(restoredSnapshot, { type: '@xstate.sync' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use @ for other builtin events like init, done.invoke and more

Suggested change
this.update(restoredSnapshot, { type: '@xstate.sync' });
this.update(restoredSnapshot, { type: 'xstate.sync' });

@@ -567,6 +588,7 @@ export class Actor<TLogic extends AnyActorLogic>
return this;
}
this.mailbox.clear();
this._synchronizerSubscription?.unsubscribe();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what snapshot should the synchronizer be aware of when somebody does smth like this:

const actorRef = createActor(m, { sync: someSynchronizer })

// ... do stuff

actorRef.stop()
actorRef.getSnapshot() // .status === 'stopped'

If I'm not mistaken the known snapshot after this operation has a stopped status but the synchronizer won't be aware of it because we have unsubscribed earlier.

@@ -263,6 +283,7 @@ export class Actor<TLogic extends AnyActorLogic>

switch ((this._snapshot as any).status) {
case 'active':
this._synchronizer?.setSnapshot(snapshot);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why only for the active status? done and error should likely be passed to it too (and likely stopped too)

const restoredSnapshot =
this.logic.restoreSnapshot?.(rawSnapshot, this._actorScope) ??
rawSnapshot;
this.update(restoredSnapshot, { type: '@xstate.sync' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should get enqueued and not handled immediately because with this model we allow for this.update to be potentially reentered (and that's a bad thing™). And if we are in the middle of processing 5 events mailbox and we receive a rawSnapshot here then likely we should append to the mailbox instead of processing it right away

(child: any) => {
if (child.getSnapshot().status === 'active') {
child.start();
if (snapshot.children) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could likely be reverted - no tests fails without this if 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants