Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Aug 15, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e1d366
fix(component-store): make synchronous updater errors catchable
markostanimirovic Jul 15, 2022
c3b3e00
chore: CR suggestions
markostanimirovic Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 124 additions & 72 deletions modules/component-store/spec/component-store.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
scheduled,
queueScheduler,
asyncScheduler,
throwError,
} from 'rxjs';
import {
delayWhen,
Expand Down Expand Up @@ -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(
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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$
Expand All @@ -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);
})
Expand Down Expand Up @@ -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;
Expand Down
37 changes: 23 additions & 14 deletions modules/component-store/src/component-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
queueScheduler,
scheduled,
asyncScheduler,
EMPTY,
} from 'rxjs';
import {
concatMap,
Expand All @@ -19,6 +20,8 @@ import {
distinctUntilChanged,
shareReplay,
take,
tap,
catchError,
} from 'rxjs/operators';
import { debounceSync } from './debounce-sync';
import {
Expand Down Expand Up @@ -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;
markostanimirovic marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! So this will error the async updater but not the general state$ 👍

}),
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;
markostanimirovic marked this conversation as resolved.
Show resolved Hide resolved
}
isSyncUpdate = false;

return subscription;
}) as unknown as ReturnType;
}
Expand Down