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.

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

Add smarter type inference for ofType pipe. #1183

Merged
merged 1 commit into from
Oct 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 26 additions & 14 deletions modules/effects/spec/actions.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,20 @@ import { hot, cold } from 'jasmine-marbles';
import { of } from 'rxjs';

describe('Actions', function() {
let actions$: Actions;
let actions$: Actions<AddAction | SubtractAction>;
let dispatcher: ScannedActionsSubject;

const ADD = 'ADD';
const SUBTRACT = 'SUBTRACT';

interface AddAction extends Action {
type: 'ADD';
}

interface SubtractAction extends Action {
type: 'SUBTRACT';
}

function reducer(state: number = 0, action: Action) {
switch (action.type) {
case ADD:
Expand Down Expand Up @@ -58,10 +66,10 @@ describe('Actions', function() {
actions.forEach(action => dispatcher.next(action));
});

it('should let you filter out actions', function() {
const actions = [ADD, ADD, SUBTRACT, ADD, SUBTRACT];
const expected = actions.filter(type => type === ADD);
const actions = [ADD, ADD, SUBTRACT, ADD, SUBTRACT];
const expected = actions.filter(type => type === ADD);

it('should let you filter out actions', function() {
actions$
.pipe(
ofType(ADD),
timdeschryver marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -78,16 +86,20 @@ describe('Actions', function() {
dispatcher.complete();
});

it('should support using the ofType instance operator', () => {
const action = { type: ADD };

const response = cold('-b', { b: true });
const expected = cold('--c', { c: true });

const effect$ = new Actions(hot('-a', { a: action }))
.ofType(ADD)
.pipe(switchMap(() => response));
it('should let you filter out actions and ofType can take an explicit type argument', function() {
actions$
.pipe(
ofType<AddAction>(ADD),
map(update => update.type),
toArray()
)
.subscribe({
next(actual) {
expect(actual).toEqual(expected);
},
});

expect(effect$).toBeObservable(expected);
actions.forEach(action => dispatcher.next({ type: action }));
dispatcher.complete();
});
});
3 changes: 2 additions & 1 deletion modules/effects/spec/effects_feature_module.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ class FeatureEffects {
);

@Effect()
effectWithStore = this.actions.ofType('INCREMENT').pipe(
effectWithStore = this.actions.pipe(
ofType('INCREMENT'),
withLatestFrom(this.store.select(getDataState)),
map(([action, state]) => ({ type: 'INCREASE' }))
);
Expand Down
85 changes: 72 additions & 13 deletions modules/effects/src/actions.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Inject, Injectable } from '@angular/core';
import { Action, ScannedActionsSubject } from '@ngrx/store';
import { Observable, Operator, OperatorFunction } from 'rxjs';
import { Observable, OperatorFunction, Operator } from 'rxjs';
import { filter } from 'rxjs/operators';

@Injectable()
Expand All @@ -19,20 +19,79 @@ export class Actions<V = Action> extends Observable<V> {
observable.operator = operator;
return observable;
}

/**
* @deprecated from 6.1.0. Use the pipeable `ofType` operator instead.
*/
ofType<V2 extends V = V>(...allowedTypes: string[]): Actions<V2> {
Copy link
Member

Choose a reason for hiding this comment

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

This may sound stupid, but here it goes anyways:
Since we have the fallback ofType, could we not remove this but mark it as deprecated?
With the next release we can remove it.
I say this because while the compat solution works, the user would have to change the import statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no opinions on backwards compat (google operates on one-version policy), and will do whatever you guys decide.

Copy link
Member

Choose a reason for hiding this comment

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

I was planning on doing a 6.1 release, so maybe we can deprecate the instance operators before release and then move forward with the removal for the next major release.

return ofType<any>(...allowedTypes)(this as Actions<any>) as Actions<V2>;
}
}

export function ofType<T extends Action>(
/**
* 'ofType' filters an Observable of Actions into an observable of the actions
* whose type strings are passed to it.
*
* For example, `actions.pipe(ofType('add'))` returns an
* `Observable<AddtionAction>`
*
* Properly typing this function is hard and requires some advanced TS tricks
* below.
*
* Type narrowing automatically works, as long as your `actions` object
* starts with a `Actions<SomeUnionOfActions>` instead of generic `Actions`.
*
* For backwards compatibility, when one passes a single type argument
* `ofType<T>('something')` the result is an `Observable<T>`. Note, that `T`
* completely overrides any possible inference from 'something'.
*
* Unfortunately, for unknown 'actions: Actions' these types will produce
* 'Observable<never>'. In such cases one has to manually set the generic type
* like `actions.ofType<AdditionAction>('add')`.
*/
export function ofType<
V extends Extract<U, { type: T1 }>,
T1 extends string = string,
U extends Action = Action
>(t1: T1): OperatorFunction<U, V>;
export function ofType<
V extends Extract<U, { type: T1 | T2 }>,
T1 extends string = string,
T2 extends string = string,
U extends Action = Action
>(t1: T1, t2: T2): OperatorFunction<U, V>;
export function ofType<
V extends Extract<U, { type: T1 | T2 | T3 }>,
T1 extends string = string,
T2 extends string = string,
T3 extends string = string,
U extends Action = Action
>(t1: T1, t2: T2, t3: T3): OperatorFunction<U, V>;
export function ofType<
V extends Extract<U, { type: T1 | T2 | T3 | T4 }>,
T1 extends string = string,
T2 extends string = string,
T3 extends string = string,
T4 extends string = string,
U extends Action = Action
>(t1: T1, t2: T2, t3: T3, t4: T4): OperatorFunction<U, V>;
export function ofType<
V extends Extract<U, { type: T1 | T2 | T3 | T4 | T5 }>,
T1 extends string = string,
T2 extends string = string,
T3 extends string = string,
T4 extends string = string,
T5 extends string = string,
U extends Action = Action
>(t1: T1, t2: T2, t3: T3, t4: T4, t5: T5): OperatorFunction<U, V>;
/**
* Fallback for more than 5 arguments.
* There is no inference, so the return type is the same as the input -
* Observable<Action>.
*
* We provide a type parameter, even though TS will not infer it from the
* arguments, to preserve backwards compatibility with old versions of ngrx.
*/
export function ofType<V extends Action>(
...allowedTypes: string[]
): OperatorFunction<Action, V>;
export function ofType(
...allowedTypes: string[]
): OperatorFunction<Action, T> {
return filter(
(action: Action): action is T =>
allowedTypes.some(type => type === action.type)
): OperatorFunction<Action, Action> {
return filter((action: Action) =>
allowedTypes.some(type => type === action.type)
);
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,4 @@
"url": "https://opencollective.com/ngrx",
"logo": "https://opencollective.com/opencollective/logo.txt"
}
}
}