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

Update Signature of Actions and Actions#ofType to infer action interface in Effects #860

Closed
MikeRyanDev opened this issue Feb 25, 2018 · 6 comments

Comments

@MikeRyanDev
Copy link
Member

Using the new mapped types feature in TypeScript 2.8 we can now write a type safe version of Actions#ofType that can correctly infer the interface of the action based on the provided action type:

type Action<Type extends string> = { type: Type };

class Actions<T extends Action> extends Observable<T> { }

type ActionsOfType<A, T extends string> = A extends Action<T> ? A : never;

function ofType<V, T1 extends string>(t1: T1): OperatorFunction<V, ActionsOfType<V, T1>>;
function ofType(...types: string[]) {
  return function (source: Observable<any>) {
    return source.pipe(filter(action => types.indexOf(action.type) !== -1)) as any;
  }
}

This will be a major breaking change for Effects. To minimize the impact we should add an @ngrx/effects/compat submodule that exports a backwards compatible version of Actions and ofType so that developers can incrementally upgrade. We will remove @ngrx/effects/compat in 7.0.

@timdeschryver
Copy link
Member

I don't know how much effort it will cost or if it's even possible.
But I was thinking if this could be resolved with a typescript linter (think of rxjs-tslint)?
Because then we could add a step during ng update to fix it automatically.

@timdeschryver
Copy link
Member

I have something ready at tdeschryver/platform/oftype, without the compat module for now.
When the 7x development has started I can create a PR.

@rkirov
Copy link
Contributor

rkirov commented Jul 12, 2018

This features is on the critical path for turning on --strictFunctionTypes in google's TS codebase, so I am very invested in getting a solution checked in ASAP.

Here is the problem. Currently the following code compiles without an error:

      this.actions.pipe(
          ofType(ActionTypes.CREATE),
          mergeMap(
              (action: CreatetAction) => {...}

How it actually works is that after ofType TS is inferring an Observalbe<Action>, but without strictFnTypes one is allowed to pipe a callback of type (action: CreateAction) => ... to such an observable. Of course, such a pipe is wrong in general and with strictFnType this starts throwing a compiler error.

It appears there are 3 different solutions for better typing 'oftype' floating around - @MikeRyanDev, @tdeschryver and by @mtaran-google. They are slightly different in terms of naming and maybe support for multiple arguments, but for my purposes I think any of them will fix the majority of the failures we are seeing. I want to get ball rolling, so who will create an actual PR for this, so that we can start reviewing it?

@mtaran-google
Copy link
Contributor

The version I came up with is at https://gist.github.com/mtaran-google/8f30d4853e225e830d9a3e712eb96618

Blame clang-format for the indentation.

@alex-okrushko
Copy link
Member

@rkirov Hey Rado, Does this patch passes Google tests? (I haven't tried - not sure if you ran those).

@rkirov
Copy link
Contributor

rkirov commented Jul 14, 2018

I did an internal test and there is just a single failure (though it only tests pipable ofType).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants