From 1a906fd6fde63c240a47752ce5042f5fa596c8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Stanimirovi=C4=87?= Date: Mon, 15 Aug 2022 21:46:20 +0200 Subject: [PATCH] fix(component-store): make synchronous updater errors catchable (#3490) * fix(component-store): make synchronous updater errors catchable * chore: CR suggestions --- .../spec/component-store.spec.ts | 196 +++++++++++------- .../component-store/src/component-store.ts | 37 ++-- 2 files changed, 147 insertions(+), 86 deletions(-) diff --git a/modules/component-store/spec/component-store.spec.ts b/modules/component-store/spec/component-store.spec.ts index cd31d1fdd0..457dbe3af0 100644 --- 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((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(() => ({})); + + 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(() => ({})); + 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 index 6f2b1a02ab..0043b4ac2b 100644 --- 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 implements OnDestroy { return (( observableOrValue?: OriginType | Observable ): 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 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; }