diff --git a/packages/store/src/internal/state-factory.ts b/packages/store/src/internal/state-factory.ts index e41a56131..f6e27eadc 100644 --- a/packages/store/src/internal/state-factory.ts +++ b/packages/store/src/internal/state-factory.ts @@ -360,17 +360,16 @@ export class StateFactory { ); if (cancelable) { - const notifier$ = dispatched$.pipe(ofActionDispatched(action)); - result = result.pipe(takeUntil(notifier$)); + const canceled = dispatched$.pipe(ofActionDispatched(action)); + result = result.pipe(takeUntil(canceled)); } result = result.pipe( // Note that we use the `finalize` operator only when the action handler - // returns an observable. If the action handler is synchronous, we do not - // need to set the state context functions to `noop`, as the absence of a - // return value indicates no asynchronous functionality. If the handler's - // result is unsubscribed (either because the observable has completed or it - // was unsubscribed by `takeUntil` due to a new action being dispatched), + // explicitly returns an observable (or a promise) to wait for. This means + // the action handler is written in a "fire & wait" style. If the handler’s + // result is unsubscribed (either because the observable has completed or + // it was unsubscribed by `takeUntil` due to a new action being dispatched), // we prevent writing to the state context. finalize(() => { stateContext.setState = noop; diff --git a/packages/store/tests/issues/canceling-promises.spec.ts b/packages/store/tests/issues/canceling-promises.spec.ts index 7e1ddac60..45ff0a594 100644 --- a/packages/store/tests/issues/canceling-promises.spec.ts +++ b/packages/store/tests/issues/canceling-promises.spec.ts @@ -15,6 +15,10 @@ describe('Canceling promises (preventing state writes)', () => { static readonly type = 'Increment with then'; } + class IncrementWithFireAndForget { + static readonly type = 'Increment with fire and forget'; + } + const { promise: promiseAwaitReady, markPromiseResolved: markPromiseAwaitReady } = createPromiseTestHelper(); @@ -45,6 +49,16 @@ describe('Canceling promises (preventing state writes)', () => { recorder.push(`value: ${ctx.getState()}`); }); } + + @Action(IncrementWithFireAndForget, { cancelUncompleted: true }) + incrementWithFireAndForget(ctx: StateContext) { + recorder.push('before promise then ready'); + promiseThenReady.then(() => { + recorder.push('after promise then ready'); + ctx.setState(value => value + 1); + recorder.push(`value: ${ctx.getState()}`); + }); + } } beforeEach(() => { @@ -125,4 +139,36 @@ describe('Canceling promises (preventing state writes)', () => { 'value: 1' ]); }); + + it('should allow state writes when the action is written in "fire & forget" style', async () => { + // Arrange + const store = TestBed.inject(Store); + + // Act + store.dispatch(new IncrementWithFireAndForget()); + + // Assert + expect(recorder).toEqual(['before promise then ready']); + + // Act + store.dispatch(new IncrementWithFireAndForget()); + + // Assert + expect(recorder).toEqual(['before promise then ready', 'before promise then ready']); + + // Act + markPromiseThenReady(); + await promiseThenReady; + + // Assert + expect(store.snapshot()).toEqual({ counter: 2 }); + expect(recorder).toEqual([ + 'before promise then ready', + 'before promise then ready', + 'after promise then ready', + 'value: 1', + 'after promise then ready', + 'value: 2' + ]); + }); });