Skip to content

Commit

Permalink
fix(store): dispatch return observable should be <void>
Browse files Browse the repository at this point in the history
This commit updates all `dispatch` signatures to return `Observable<void>`. Initially, the
signature was intended to be `void`, but historically, it was returning `Observable<any>`,
where `any` represents the state snapshot.

This is a breaking change. We've been delaying this fix for years, but now all the necessary
changes should finally be made to allow us to progress with the NGXS code itself. It's especially
important to clean up old planned todos as well.
  • Loading branch information
arturovt committed Mar 14, 2024
1 parent e8a3d9f commit 963d9fb
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 60 deletions.
23 changes: 14 additions & 9 deletions packages/store/src/internal/dispatcher.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Injectable } from '@angular/core';
import { getActionTypeFromInstance } from '@ngxs/store/plugins';
import { EMPTY, forkJoin, Observable, of, Subject, throwError } from 'rxjs';
import { exhaustMap, filter, shareReplay, take } from 'rxjs/operators';
import { exhaustMap, filter, map, shareReplay, take } from 'rxjs/operators';

import { compose } from '../utils/compose';
import { InternalErrorReporter, ngxsErrorHandler } from './error-handler';
Expand Down Expand Up @@ -33,7 +33,7 @@ export class InternalDispatcher {
/**
* Dispatches event(s).
*/
dispatch(actionOrActions: any | any[]): Observable<any> {
dispatch(actionOrActions: any | any[]): Observable<void> {
const result = this._ngxsExecutionStrategy.enter(() =>
this.dispatchByEvents(actionOrActions)
);
Expand All @@ -43,16 +43,19 @@ export class InternalDispatcher {
);
}

private dispatchByEvents(actionOrActions: any | any[]): Observable<any> {
private dispatchByEvents(actionOrActions: any | any[]): Observable<void> {
if (Array.isArray(actionOrActions)) {
if (actionOrActions.length === 0) return of(this._stateStream.getValue());
return forkJoin(actionOrActions.map(action => this.dispatchSingle(action)));
if (actionOrActions.length === 0) return of(undefined);

return forkJoin(actionOrActions.map(action => this.dispatchSingle(action))).pipe(
map(() => undefined)
);
} else {
return this.dispatchSingle(actionOrActions);
}
}

private dispatchSingle(action: any): Observable<any> {
private dispatchSingle(action: any): Observable<void> {
if (typeof ngDevMode === 'undefined' || ngDevMode) {
const type: string | undefined = getActionTypeFromInstance(action);
if (!type) {
Expand All @@ -78,7 +81,7 @@ export class InternalDispatcher {
this._actions.next({ action: nextAction, status: ActionStatus.Dispatched });
return this.createDispatchObservable(actionResult$);
}
])(prevState, action) as Observable<any>
])(prevState, action) as Observable<void>
).pipe(shareReplay());
}

Expand All @@ -92,13 +95,15 @@ export class InternalDispatcher {
);
}

private createDispatchObservable(actionResult$: Observable<ActionContext>): Observable<any> {
private createDispatchObservable(
actionResult$: Observable<ActionContext>
): Observable<void> {
return actionResult$
.pipe(
exhaustMap((ctx: ActionContext) => {
switch (ctx.status) {
case ActionStatus.Successful:
return of(this._stateStream.getValue());
return of(undefined);
case ActionStatus.Errored:
return throwError(ctx.error);
default:
Expand Down
6 changes: 3 additions & 3 deletions packages/store/src/internal/error-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ import { NgxsExecutionStrategy } from '../execution/symbols';
* 4) `toPromise()` without `catch` -> do `handleError()`
* 5) `toPromise()` with `catch` -> don't `handleError()`
*/
export function ngxsErrorHandler<T>(
export function ngxsErrorHandler(
internalErrorReporter: InternalErrorReporter,
ngxsExecutionStrategy: NgxsExecutionStrategy
) {
return (source: Observable<T>) => {
return (source: Observable<void>) => {
let subscribed = false;

source.subscribe({
Expand All @@ -40,7 +40,7 @@ export function ngxsErrorHandler<T>(
}
});

return new Observable(subscriber => {
return new Observable<void>(subscriber => {
subscribed = true;
return source.pipe(leaveNgxs(ngxsExecutionStrategy)).subscribe(subscriber);
});
Expand Down
3 changes: 1 addition & 2 deletions packages/store/src/store.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
// tslint:disable:unified-signatures
import { Inject, Injectable, Optional, Signal, Type, computed } from '@angular/core';
import { Observable, of, Subscription, throwError } from 'rxjs';
import { catchError, distinctUntilChanged, map, shareReplay, take } from 'rxjs/operators';
Expand Down Expand Up @@ -41,7 +40,7 @@ export class Store {
/**
* Dispatches event(s).
*/
dispatch(actionOrActions: any | any[]): Observable<any> {
dispatch(actionOrActions: any | any[]): Observable<void> {
return this._internalStateOperations.getRootStateOperations().dispatch(actionOrActions);
}

Expand Down
88 changes: 50 additions & 38 deletions packages/store/tests/dispatch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -924,23 +924,23 @@ describe('Dispatch', () => {
describe('when many separate actions dispatched return out of order', () => {
it('should notify of the completion of the relative observable', fakeAsync(() => {
// Arrange
class Append {
static type = 'Test';
class Add {
static type = 'Increment';

constructor(public payload: string) {}
constructor(readonly payload: number) {}
}

@State<string>({
name: 'text',
defaults: ''
@State<number>({
name: 'number',
defaults: 0
})
@Injectable()
class MyState {
@Action(Append)
append({ getState, setState }: StateContext<string>, { payload }: Append) {
@Action(Add)
add(ctx: StateContext<string>, { payload }: Add) {
return of({}).pipe(
delay(payload.length * 10),
tap(() => setState(getState() + payload))
delay(payload),
tap(() => ctx.setState(ctx.getState() + payload))
);
}
}
Expand All @@ -952,14 +952,29 @@ describe('Dispatch', () => {
const store = TestBed.inject(Store);

// Act & assert
store
.dispatch(new Append('dddd'))
.subscribe(state => expect(state.text).toEqual('abbcccdddd'));
store.dispatch(new Append('a')).subscribe(state => expect(state.text).toEqual('a'));
store
.dispatch(new Append('ccc'))
.subscribe(state => expect(state.text).toEqual('abbccc'));
store.dispatch(new Append('bb')).subscribe(state => expect(state.text).toEqual('abb'));
store.dispatch(new Add(10)).subscribe(() => {
expect(store.snapshot()).toEqual({
number: 10
});
});

store.dispatch(new Add(20)).subscribe(() => {
expect(store.snapshot()).toEqual({
number: 30
});
});

store.dispatch(new Add(30)).subscribe(() => {
expect(store.snapshot()).toEqual({
number: 60
});
});

store.dispatch(new Add(40)).subscribe(() => {
expect(store.snapshot()).toEqual({
number: 100
});
});

tick(100);
}));
Expand All @@ -968,23 +983,23 @@ describe('Dispatch', () => {
describe('when many actions dispatched together', () => {
it('should notify once all completed', fakeAsync(() => {
// Arrange
class Append {
static type = 'Test';
class Add {
static type = 'Increment';

constructor(public payload: string) {}
constructor(readonly payload: number) {}
}

@State<string>({
name: 'text',
defaults: ''
@State<number>({
name: 'number',
defaults: 0
})
@Injectable()
class MyState {
@Action(Append)
append({ getState, setState }: StateContext<string>, { payload }: Append) {
@Action(Add)
add(ctx: StateContext<string>, { payload }: Add) {
return of({}).pipe(
delay(payload.length * 10),
tap(() => setState(getState() + payload))
delay(payload),
tap(() => ctx.setState(ctx.getState() + payload))
);
}
}
Expand All @@ -995,19 +1010,16 @@ describe('Dispatch', () => {

const store = TestBed.inject(Store);

const finalNumber = 100; // 10 + 20 + 30 + 40

// Act & assert
store
.dispatch([new Append('dddd'), new Append('a'), new Append('ccc'), new Append('bb')])
.subscribe(results => {
expect(results.map((r: any) => r.text)).toEqual([
'abbcccdddd',
'a',
'abbccc',
'abb'
]);
store.dispatch([new Add(10), new Add(20), new Add(30), new Add(40)]).subscribe(() => {
expect(store.snapshot()).toEqual({
number: finalNumber
});
});

tick(100);
tick(finalNumber);
}));
});
});
Expand Down
16 changes: 8 additions & 8 deletions packages/store/types/tests/dispatch.lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ describe('[TEST]: Action Types', () => {
});

it('should be correct type in dispatch', () => {
assertType(() => store.dispatch([])); // $ExpectType Observable<any>
assertType(() => store.dispatch(new FooAction('payload'))); // $ExpectType Observable<any>
assertType(() => store.dispatch(new BarAction('foo'))); // $ExpectType Observable<any>
assertType(() => store.dispatch([])); // $ExpectType Observable<void>
assertType(() => store.dispatch(new FooAction('payload'))); // $ExpectType Observable<void>
assertType(() => store.dispatch(new BarAction('foo'))); // $ExpectType Observable<void>
assertType(() => store.dispatch()); // $ExpectError
assertType(() => store.dispatch({})); // $ExpectType Observable<any>
assertType(() => store.dispatch({})); // $ExpectType Observable<void>
});

it('should prevent invalid types passed through', () => {
Expand All @@ -83,10 +83,10 @@ describe('[TEST]: Action Types', () => {
@Action({ foo: 123 }) increment4() {} // $ExpectError
}

assertType(() => store.dispatch(new Increment())); // $ExpectType Observable<any>
assertType(() => store.dispatch({ type: 'INCREMENT' })); // $ExpectType Observable<any>
assertType(() => store.dispatch(Increment)); // $ExpectType Observable<any>
assertType(() => store.dispatch({ foo: 123 })); // $ExpectType Observable<any>
assertType(() => store.dispatch(new Increment())); // $ExpectType Observable<void>
assertType(() => store.dispatch({ type: 'INCREMENT' })); // $ExpectType Observable<void>
assertType(() => store.dispatch(Increment)); // $ExpectType Observable<void>
assertType(() => store.dispatch({ foo: 123 })); // $ExpectType Observable<void>
});

it('should be correct type base API', () => {
Expand Down

0 comments on commit 963d9fb

Please sign in to comment.