Platform: RFC: Introduce new "effect" function as an alternative/replacement for the @Effect() decorator

[MikeRyanDev]

The @Effect() decorator is used to annotate classes with metadata about which properties on the class are effects. At runtime @ngrx/effects reads this metadata to determine which properties on the class instance should be subscribed to. This has four disadvantages:

1. No Type Checking

This would be legal using the decorator and would result in a runtime error:

class MyEffects {
  @Effect() source$ = 'not an observable';
}

Similarly, it can't type check that the items emitted from an observable are actions if dispatch is true.

This would be legal using the decorator and would also result in a runtime error:

class MyEffects {
  @Effect() source$ = observableOf('not an action');
}

2. Incompatibility with Closure

More information about this is captured in #362, but basically the @Effect() decorator does not work out of the box with Closure's property renaming. It requires that the developers write an additional interface to declare the property names so that Closure doesn't rename them.

3. Requires a reflect-metadata polyfill

All built-in Angular decorators are removed when compiled with the AOT compiler. We can't tap into this, so the @Effect() decorator needs the reflect-metadata polyfill even for production builds.

4. Needs defer for testing some effects

If an effect uses some other observable to start you typically need to wrap it in defer(...) so that you have an opportunity to interact with the source service in a test:

class MyEffects {
  @Effect() source$ = defer(() => {
    return mySocketService.connect();
  });
}

Alternative

I recommend adding a new effect function that can be used instead of the @Effect() decorator to address the above concerns:

class MyEffect {
  source$ = effect(() => this.actions.ofType(...));

  source$ = effect(() => {...}, { dispatch: false });
}

At runtime @ngrx/effects would enumerate over the properties of each registered class instance and look for any effects that were created with the effect(...) function.

This function can be strongly typed to verify that an observable is always returned by the inner callback and that the returned stream is a stream of Actions if dispatch is true. Here is the proposed type signature:

export function effect<T>(source: () => Observable<T>, options: { dispatch: false }): Observable<T>;
export function effect<T extends Action>(
  source: () => Observable<T>,
  options?: { dipsatch: true },
): Observable<T>;

If accepted, I would be willing to submit a PR for this feature

• [x] Yes (Assistance is provided if you need help submitting a pull request)
• [ ] No

cc @alex-okrushko @rkirov

[rkirov]

Decorators have been irresistible for framework authors.

Meanwhile, as maintainers of the TypeScript infrastructure at Google, we don't like decorators. They are known to be experimental in TS and bound to change as the spec moves along. Angular has decided to handle them through the AOT compiler, which is not a reusable mechanism for other frameworks. So from that perspective I am welcoming this change and can help roll it out to our internal users.

But be warned, people love their syntactic sugar.

Can you elaborate on "At runtime @ngrx/effects would enumerate over the properties of each registered class instance and look for any effects that were created with the effect(...) function." I want to make sure that there will no issues with Closure.

[alex-okrushko]

Huge :+1: !

While we found the combination of closure flags that would work with property renaming, this combination is NOT what TS eng team at Google recommends as defaults for TS projects AND it still requires annotating them with /** @export */.

[wardbell]

I liked the proposal on sight.

Decorators are lovely for developers but problematic as you say and, honestly, should be reserved for tasks that cannot be easily performed another way. Your effect() function is no harder to use than the decorator.

Go for it!

[brandonroberts]

I like the proposal for the increased type safety for declaring effects. I still think the decorator is more visually appealing and consistent with how the framework does it, but I suppose you can't have both unless you explicitly type the effect.

[dummdidumm]

How about passing the actions and the state as observable parameters to the effect function (similar to how redux-observable does ist)?

export function effect<T>(source: (actions$: Actions, state$: Observable<any>) => Observable<T>, options: { dispatch: false }): Observable<T>;
export function effect<T extends Action>(
  source: (actions$: Actions, state$: Observable<any>) => Observable<T>,
  options?: { dispatch: true },
): Observable<T>;

This has two benefits in my opinion:

1. possibly skip the constructor altogether if you only inject Actions, either way it's one thing less to inject (two things less if you need the store state)
2. effects are easier to test

[babeal]

Will this setup break the effects as functions testing strategy? We use that everywhere as it is the only effective way to get marble testing to properly work with async operators like debounceTime/auditTime.

[ovangle]

What about the following interfaces, which mimic angular factory provider declarations, with the deps array of angular dependency tokens?

export interface DispatchingEffect {
  readonly dispatch?: true;
  /**
   * A sequence of angular dependency injection tokens that will be passed to apply
   * as arguments when the effect is run on the store.
   */
  deps?: any[];

  /**
   * Apply the effect to the given actions, with injected values for the requested
   * dependencies.
   */
  apply: (actions$: Observable<Action>, ...deps: any[]) => Observable<Action>;
}

export interface ReceivingEffect {
  readonly dispatch: false;
  deps?: any[];
  apply: (action$: Observable<Action>, ...deps: any[]) => Observable<void>;
}

export type Effect = DispatchingEffect | ReceivingEffect;

Effects would typically be declared as application constants:

export const MyEffect: Effect = {
  dispatch: false,
  apply: (action$: Observable<Action>, store: Store<any>) => {
    // dispatch is false, so must return a void observable.
    return of(undefined);
  },
  deps: [Store]
};

And providing the effects module is almost the same as before

@NgModule({
  imports: [
    EffectsModule.forRoot([
      MyEffect,
      /// Also, effects can be bundled together easily
      ...MyEffectGroup
    ])
  ]
})
export class MyModule {}

Although, it might be better to have a provideEffect function, to give some control over which injector is in scope when the effect is provided. Component scoped effects (once injectable lifecycle is available in Ivy)?

[timdeschryver]

To allow 'effects as functions testing strategy' as @babeal is asking about, I would propose the following typings:

export function effect<T extends Action>(
  source: (() => Observable<T>),
  options: { dispatch: false }
): Observable<T>;
export function effect<T extends Action>(
  source: (() => (...args: any[]) => Observable<T>),
  options: { dispatch: false }
): ((...args: any[]) => Observable<T>);
export function effect<T extends Action>(
  source: (() => Observable<T>),
  options?: { dispatch: true }
): Observable<T>;
export function effect<T extends Action>(
  source: (() => (...args: any[]) => Observable<T>),
  options?: { dispatch: true }
): ((...args: any[]) => Observable<T>);

I have been trying to implement this and ended up with @wip/effects, allthough I'm not sure if this is how you had in mind @MikeRyanDev, and if this would be compliant with the Closure compiler.