test(store): ensure state writes are allowed with fire & forget (#2241)

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;

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<number>) {
+      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'
+    ]);
+  });
 });