-
-
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
Changes from 6 commits
56a5717
f10f659
5eda50b
11de5d4
ff0385b
04bea45
1bdeb0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
--- | ||
'@xstate/store': minor | ||
--- | ||
|
||
Added support for effect-only transitions that don't trigger state updates. Now, when a transition returns the same state but includes effects, subscribers won't be notified of a state change, but the effects will still be executed. This helps prevent unnecessary re-renders while maintaining side effect functionality. | ||
|
||
```ts | ||
it('should not trigger update if the snapshot is the same even if there are effects', () => { | ||
const store = createStore({ | ||
context: { count: 0 }, | ||
on: { | ||
doNothing: (ctx, _, enq) => { | ||
enq.effect(() => { | ||
// … | ||
}); | ||
return ctx; // Context is the same, so no update is triggered | ||
// This is the same as not returning anything (void) | ||
} | ||
} | ||
}); | ||
|
||
const spy = vi.fn(); | ||
store.subscribe(spy); | ||
|
||
store.trigger.doNothing(); | ||
store.trigger.doNothing(); | ||
|
||
expect(spy).toHaveBeenCalledTimes(0); | ||
}); | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,6 @@ import { | |
ExtractEvents, | ||
InteropSubscribable, | ||
Observer, | ||
Recipe, | ||
Store, | ||
StoreAssigner, | ||
StoreContext, | ||
|
@@ -29,20 +28,6 @@ const symbolObservable: typeof Symbol.observable = (() => | |
(typeof Symbol === 'function' && Symbol.observable) || | ||
'@@observable')() as any; | ||
|
||
/** | ||
* Updates a context object using a recipe function. | ||
* | ||
* @param context - The current context | ||
* @param recipe - A function that describes how to update the context | ||
* @returns The updated context | ||
*/ | ||
function setter<TContext extends StoreContext>( | ||
context: TContext, | ||
recipe: Recipe<TContext, TContext> | ||
): TContext { | ||
return recipe(context); | ||
} | ||
|
||
const inspectionObservers = new WeakMap< | ||
Store<any, any, any>, | ||
Set<Observer<StoreInspectionEvent>> | ||
|
@@ -77,20 +62,25 @@ function createStoreCore< | |
const transition = logic.transition; | ||
|
||
function receive(event: StoreEvent) { | ||
let effects: StoreEffect<TEmitted>[]; | ||
[currentSnapshot, effects] = transition(currentSnapshot, event); | ||
const [nextSnapshot, effects] = transition(currentSnapshot, event); | ||
|
||
inspectionObservers.get(store)?.forEach((observer) => { | ||
davidkpiano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
observer.next?.({ | ||
type: '@xstate.snapshot', | ||
event, | ||
snapshot: currentSnapshot, | ||
snapshot: nextSnapshot, | ||
actorRef: store, | ||
rootId: store.sessionId | ||
}); | ||
}); | ||
|
||
atom.set(currentSnapshot); | ||
if (nextSnapshot === currentSnapshot && !effects.length) { | ||
return; | ||
|
||
} | ||
|
||
currentSnapshot = nextSnapshot; | ||
|
||
atom.set(nextSnapshot); | ||
|
||
for (const effect of effects) { | ||
if (typeof effect === 'function') { | ||
|
@@ -422,7 +412,7 @@ export function createStoreTransition< | |
event: ExtractEvents<TEventPayloadMap> | ||
): [StoreSnapshot<TContext>, StoreEffect<TEmitted>[]] => { | ||
type StoreEvent = ExtractEvents<TEventPayloadMap>; | ||
let currentContext = snapshot.context; | ||
const currentContext = snapshot.context; | ||
const assigner = transitions?.[event.type as StoreEvent['type']]; | ||
const effects: StoreEffect<TEmitted>[] = []; | ||
|
||
|
@@ -446,43 +436,22 @@ export function createStoreTransition< | |
return [snapshot, effects]; | ||
} | ||
|
||
if (typeof assigner === 'function') { | ||
currentContext = producer | ||
? producer(currentContext, (draftContext) => | ||
(assigner as StoreProducerAssigner<TContext, StoreEvent, TEmitted>)( | ||
draftContext, | ||
event, | ||
enqueue | ||
) | ||
const nextContext = producer | ||
? producer(currentContext, (draftContext) => | ||
(assigner as StoreProducerAssigner<TContext, StoreEvent, TEmitted>)( | ||
draftContext, | ||
event, | ||
enqueue | ||
) | ||
: setter(currentContext, (draftContext) => | ||
Object.assign( | ||
{}, | ||
currentContext, | ||
assigner?.( | ||
draftContext, | ||
event as any, // TODO: help me | ||
enqueue | ||
) | ||
) | ||
); | ||
} else { | ||
const partialUpdate: Record<string, unknown> = {}; | ||
davidkpiano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for (const key of Object.keys(assigner)) { | ||
const propAssignment = assigner[key]; | ||
partialUpdate[key] = | ||
typeof propAssignment === 'function' | ||
? (propAssignment as StoreAssigner<TContext, StoreEvent, TEmitted>)( | ||
currentContext, | ||
event, | ||
enqueue | ||
) | ||
: propAssignment; | ||
} | ||
currentContext = Object.assign({}, currentContext, partialUpdate); | ||
} | ||
) | ||
: (assigner(currentContext, event as any, enqueue) ?? currentContext); | ||
|
||
return [{ ...snapshot, context: currentContext }, effects]; | ||
return [ | ||
nextContext === currentContext | ||
? snapshot | ||
: { ...snapshot, context: nextContext }, | ||
effects | ||
]; | ||
}; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -446,6 +446,43 @@ it('effects can be enqueued', async () => { | |
expect(store.getSnapshot().context.count).toEqual(0); | ||
}); | ||
|
||
it('effect-only transitions should execute effects', () => { | ||
const spy = vi.fn(); | ||
const store = createStore({ | ||
context: { count: 0 }, | ||
on: { | ||
justEffect: (ctx, _, enq) => { | ||
enq.effect(spy); | ||
} | ||
} | ||
}); | ||
|
||
store.trigger.justEffect(); | ||
|
||
expect(spy).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('emits-only transitions should emit events', () => { | ||
const spy = vi.fn(); | ||
const store = createStore({ | ||
context: { count: 0 }, | ||
emits: { | ||
emitted: () => {} | ||
}, | ||
on: { | ||
justEmit: (ctx, _, enq) => { | ||
enq.emit.emitted(); | ||
} | ||
} | ||
}); | ||
|
||
store.on('emitted', spy); | ||
|
||
store.trigger.justEmit(); | ||
|
||
expect(spy).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('async effects can be enqueued', async () => { | ||
const store = createStore({ | ||
context: { | ||
|
@@ -798,6 +835,45 @@ it('can be created with a logic object', () => { | |
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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
const store = createStore({ | ||
context: { count: 0 }, | ||
on: { | ||
doNothing: (ctx) => ctx | ||
} | ||
}); | ||
|
||
const spy = vi.fn(); | ||
store.subscribe(spy); | ||
|
||
store.trigger.doNothing(); | ||
store.trigger.doNothing(); | ||
|
||
expect(spy).toHaveBeenCalledTimes(0); | ||
}); | ||
|
||
it('should not trigger update if the snapshot is the same even if there are effects', () => { | ||
const store = createStore({ | ||
context: { count: 0 }, | ||
on: { | ||
doNothing: (ctx, _, enq) => { | ||
enq.effect(() => { | ||
// … | ||
}); | ||
return ctx; | ||
} | ||
} | ||
}); | ||
|
||
const spy = vi.fn(); | ||
store.subscribe(spy); | ||
|
||
store.trigger.doNothing(); | ||
store.trigger.doNothing(); | ||
|
||
expect(spy).toHaveBeenCalledTimes(0); | ||
}); | ||
|
||
describe('types', () => { | ||
it('AnyStoreConfig', () => { | ||
function transformStoreConfig(_config: AnyStoreConfig): 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