fix(component-store): make synchronous updater errors catchable (#3490)

* fix(component-store): make synchronous updater errors catchable * chore: CR suggestions
ngrx · Aug 15, 2022 · 1a906fd · 1a906fd
1 parent 4da9a67
commit 1a906fd
Show file tree

Hide file tree

Showing 2 changed files with 147 additions and 86 deletions.
diff --git a/modules/component-store/spec/component-store.spec.ts b/modules/component-store/spec/component-store.spec.ts
@@ -17,6 +17,7 @@ import {
   scheduled,
   queueScheduler,
   asyncScheduler,
+  throwError,
 } from 'rxjs';
 import {
   delayWhen,
@@ -79,20 +80,9 @@ describe('Component Store', () => {
     it(
       'throws an Error when setState with a function/callback is called' +
         ' before initialization',
-      marbles((m) => {
+      () => {
         const componentStore = new ComponentStore();
 
-        m.expect(componentStore.state$).toBeObservable(
-          m.hot(
-            '#',
-            {},
-            new Error(
-              'ComponentStore has not been initialized yet. ' +
-                'Please make sure it is initialized before updating/getting.'
-            )
-          )
-        );
-
         expect(() => {
           componentStore.setState(() => ({ setState: 'new state' }));
         }).toThrow(
@@ -101,7 +91,7 @@ describe('Component Store', () => {
               'Please make sure it is initialized before updating/getting.'
           )
         );
-      })
+      }
     );
 
     it('throws an Error when patchState with an object is called before initialization', () => {
@@ -147,55 +137,30 @@ describe('Component Store', () => {
       }
     );
 
-    it(
-      'throws an Error when updater is called before initialization',
-      marbles((m) => {
-        const componentStore = new ComponentStore();
-
-        m.expect(componentStore.state$).toBeObservable(
-          m.hot(
-            '#',
-            {},
-            new Error(
-              'ComponentStore has not been initialized yet. ' +
-                'Please make sure it is initialized before updating/getting.'
-            )
-          )
-        );
+    it('throws an Error synchronously when updater is called before initialization', () => {
+      const componentStore = new ComponentStore();
 
-        expect(() => {
-          componentStore.updater((state, value: object) => value)({
-            updater: 'new state',
-          });
-        }).toThrow(
-          new Error(
-            'ComponentStore has not been initialized yet. ' +
-              'Please make sure it is initialized before updating/getting.'
-          )
-        );
-      })
-    );
+      expect(() => {
+        componentStore.updater((state, value: object) => value)({
+          updater: 'new state',
+        });
+      }).toThrow(
+        new Error(
+          'ComponentStore has not been initialized yet. ' +
+            'Please make sure it is initialized before updating/getting.'
+        )
+      );
+    });
 
     it(
       'throws an Error when updater is called with sync Observable' +
         ' before initialization',
-      marbles((m) => {
+      () => {
         const componentStore = new ComponentStore();
         const syncronousObservable$ = of({
           updater: 'new state',
         });
 
-        m.expect(componentStore.state$).toBeObservable(
-          m.hot(
-            '#',
-            {},
-            new Error(
-              'ComponentStore has not been initialized yet. ' +
-                'Please make sure it is initialized before updating/getting.'
-            )
-          )
-        );
-
         expect(() => {
           componentStore.updater<object>((state, value) => value)(
             syncronousObservable$
@@ -206,39 +171,32 @@ describe('Component Store', () => {
               'Please make sure it is initialized before updating/getting.'
           )
         );
-      })
+      }
     );
 
     it(
-      'does not throw an Error when updater is called with async Observable' +
-        ' before initialization, however closes the subscription and does not' +
-        ' update the state and sends error in state$',
+      'throws an Error asynchronously when updater is called with async' +
+        ' Observable before initialization, however closes the subscription' +
+        ' and does not update the state',
       marbles((m) => {
         const componentStore = new ComponentStore();
-        const asyncronousObservable$ = m.cold('-u', {
+        const asynchronousObservable$ = m.cold('-u', {
           u: { updater: 'new state' },
         });
 
         let subscription: Subscription | undefined;
 
-        m.expect(componentStore.state$).toBeObservable(
-          m.hot(
-            '-#',
-            {},
-            new Error(
-              'ComponentStore has not been initialized yet. ' +
-                'Please make sure it is initialized before updating/getting.'
-            )
-          )
-        );
-
         expect(() => {
           subscription = componentStore.updater(
             (state, value: object) => value
-          )(asyncronousObservable$);
-        }).not.toThrow();
-
-        m.flush();
+          )(asynchronousObservable$);
+          m.flush();
+        }).toThrow(
+          new Error(
+            'ComponentStore has not been initialized yet. ' +
+              'Please make sure it is initialized before updating/getting.'
+          )
+        );
 
         expect(subscription!.closed).toBe(true);
       })
@@ -638,6 +596,100 @@ describe('Component Store', () => {
     );
   });
 
+  describe('throws an error', () => {
+    it('synchronously when synchronous error is thrown within updater', () => {
+      const componentStore = new ComponentStore({});
+      const error = new Error('ERROR!');
+      const updater = componentStore.updater(() => {
+        throw error;
+      });
+
+      expect(() => updater()).toThrow(error);
+    });
+
+    it('synchronously when synchronous error is thrown within setState callback', () => {
+      const componentStore = new ComponentStore({});
+      const error = new Error('ERROR!');
+
+      expect(() => {
+        componentStore.setState(() => {
+          throw error;
+        });
+      }).toThrow(error);
+    });
+
+    it('synchronously when synchronous error is thrown within patchState callback', () => {
+      const componentStore = new ComponentStore({});
+      const error = new Error('ERROR!');
+
+      expect(() => {
+        componentStore.patchState(() => {
+          throw error;
+        });
+      }).toThrow(error);
+    });
+
+    it('synchronously when synchronous observable throws an error with updater', () => {
+      const componentStore = new ComponentStore({});
+      const error = new Error('ERROR!');
+      const updater = componentStore.updater<unknown>(() => ({}));
+
+      expect(() => {
+        updater(throwError(() => error));
+      }).toThrow(error);
+    });
+
+    it('synchronously when synchronous observable throws an error with patchState', () => {
+      const componentStore = new ComponentStore({});
+      const error = new Error('ERROR!');
+
+      expect(() => {
+        componentStore.patchState(throwError(() => error));
+      }).toThrow(error);
+    });
+
+    it(
+      'asynchronously when asynchronous observable throws an error with updater',
+      marbles((m) => {
+        const componentStore = new ComponentStore({});
+        const error = new Error('ERROR!');
+        const updater = componentStore.updater<unknown>(() => ({}));
+        const asyncObs$ = m.cold('-#', {}, error);
+
+        expect(() => {
+          try {
+            updater(asyncObs$);
+          } catch {
+            throw new Error('updater should not throw an error synchronously');
+          }
+
+          m.flush();
+        }).toThrow(error);
+      })
+    );
+
+    it(
+      'asynchronously when asynchronous observable throws an error with patchState',
+      marbles((m) => {
+        const componentStore = new ComponentStore({});
+        const error = new Error('ERROR!');
+        const asyncObs$ = m.cold('-#', {}, error);
+
+        expect(() => {
+          try {
+            componentStore.patchState(asyncObs$);
+          } catch {
+            throw new Error(
+              'patchState should not throw an error synchronously'
+            );
+          }
+
+          m.flush();
+        }).toThrow(error);
+      })
+    );
+  });
+
   describe('selector', () => {
     interface State {
       value: string;

diff --git a/modules/component-store/src/component-store.ts b/modules/component-store/src/component-store.ts
@@ -10,6 +10,7 @@ import {
   queueScheduler,
   scheduled,
   asyncScheduler,
+  EMPTY,
 } from 'rxjs';
 import {
   concatMap,
@@ -19,6 +20,8 @@ import {
   distinctUntilChanged,
   shareReplay,
   take,
+  tap,
+  catchError,
 } from 'rxjs/operators';
 import { debounceSync } from './debounce-sync';
 import {
@@ -110,7 +113,10 @@ export class ComponentStore<T extends object> implements OnDestroy {
     return ((
       observableOrValue?: OriginType | Observable<OriginType>
     ): Subscription => {
-      let initializationError: Error | undefined;
+      // We need to explicitly throw an error if a synchronous error occurs.
+      // This is necessary to make synchronous errors catchable.
+      let isSyncUpdate = true;
+      let syncError: unknown;
       // We can receive either the value or an observable. In case it's a
       // simple value, we'll wrap it with `of` operator to turn it into
       // Observable.
@@ -128,23 +134,26 @@ export class ComponentStore<T extends object> implements OnDestroy {
               : // If state was not initialized, we'll throw an error.
                 throwError(() => new Error(this.notInitializedErrorMessage))
           ),
+          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+          map(([value, currentState]) => updaterFn(currentState, value!)),
+          tap((newState) => this.stateSubject$.next(newState)),
+          catchError((error: unknown) => {
+            if (isSyncUpdate) {
+              syncError = error;
+              return EMPTY;
+            }
+
+            return throwError(() => error);
+          }),
           takeUntil(this.destroy$)
         )
-        .subscribe({
-          next: ([value, currentState]) => {
-            // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-            this.stateSubject$.next(updaterFn(currentState, value!));
-          },
-          error: (error: Error) => {
-            initializationError = error;
-            this.stateSubject$.error(error);
-          },
-        });
+        .subscribe();
 
-      if (initializationError) {
-        // prettier-ignore
-        throw /** @type {!Error} */ (initializationError);
+      if (syncError) {
+        throw syncError;
       }
+      isSyncUpdate = false;
+
       return subscription;
     }) as unknown as ReturnType;
   }