-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[@xstate/store] Refactor store logic to improve context updates and snapshot handling #5323
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
Conversation
davidkpiano
commented
Jul 6, 2025
- Removed unused setter function and associated recipe parameter.
- Updated transition logic to prevent unnecessary updates when the snapshot remains unchanged.
- Enhanced tests to verify behavior of effect-only and emit-only transitions, ensuring no updates trigger when the snapshot is the same.
- Removed unused setter function and associated recipe parameter. - Updated transition logic to prevent unnecessary updates when the snapshot remains unchanged. - Enhanced tests to verify behavior of effect-only and emit-only transitions, ensuring no updates trigger when the snapshot is the same.
🦋 Changeset detectedLatest commit: 1bdeb0f 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 |
store.getSnapshot().context.count satisfies string; | ||
}); | ||
|
||
it('should not trigger update if the snapshot is the same', () => { |
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.
note this diverges from XState too:
import { createActor, createMachine } from "xstate";
const m = createMachine({
on: {
FOO: {
actions: () => {},
},
},
});
const a = createActor(m).start();
let s = a.getSnapshot();
a.subscribe((_s) => {
console.log("next", s === _s, _s);
s = _s;
});
a.send({ type: "FOO" });
a.send({ type: "FOO" });
a.send({ type: "FOO" });
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 do you think about this behavior? If only actions/effects occur (invisible to the state, at least right now), then the state is going to be exactly the same. But I guess that's the responsibility of the consumer to "filter"
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.
Similarly here, I think it would be totally OK to filter those out - but I think it would be great if this could be consistent between XState libraries.
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, I think in XState v6, the purpose of the observer is to observe snapshot changes, so notifying on the same snapshot isn't useful.
@Andarist Is this one good to go? Since XState Store is forward-looking, I'll make sure that v6 has the same behavior of only updating when the snapshot changes. |
// … | ||
}); | ||
return ctx; // Context is the same, so no update is triggered | ||
// This is the same as not returning anything (void) |
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.
nit: it feels this "variant" is kind of hidden in this comment when "same state" is mentioned explicitly in the text leading to this code sample
packages/xstate-store/src/store.ts
Outdated
if (nextSnapshot === currentSnapshot && !effects.length) { | ||
return; |
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.
all tests pass without this, I pushed out a commit removing this check