From 931adb1ddd0b9132830db3f78dc6f38fda5b40e3 Mon Sep 17 00:00:00 2001 From: Mike Ryan Date: Tue, 20 Jun 2017 12:04:16 -0500 Subject: [PATCH] fix(Effects): Start child effects after running root effects (#43) This removes the app initialization logic altogether. The 'OnRunEffects' lifecycle hook should be sufficient to control when your effects start. --- modules/effects/src/effect_notification.ts | 58 +++++++++++++++++++ modules/effects/src/effect_sources.ts | 43 +++----------- modules/effects/src/effects_feature_module.ts | 6 +- modules/effects/src/effects_module.ts | 5 +- modules/effects/src/effects_resolver.ts | 9 +-- modules/effects/src/effects_root_module.ts | 23 ++++++++ modules/effects/src/on_run_effects.ts | 2 +- modules/effects/src/run_effects.ts | 31 ---------- modules/effects/src/tokens.ts | 3 - 9 files changed, 96 insertions(+), 84 deletions(-) create mode 100644 modules/effects/src/effect_notification.ts create mode 100644 modules/effects/src/effects_root_module.ts delete mode 100644 modules/effects/src/run_effects.ts diff --git a/modules/effects/src/effect_notification.ts b/modules/effects/src/effect_notification.ts new file mode 100644 index 0000000000..7daf9410c1 --- /dev/null +++ b/modules/effects/src/effect_notification.ts @@ -0,0 +1,58 @@ +import { Observable } from 'rxjs/Observable'; +import { Notification } from 'rxjs/Notification'; +import { Action } from '@ngrx/store'; +import { ErrorReporter } from './error_reporter'; + +export interface EffectNotification { + effect: Observable | (() => Observable); + propertyName: string; + sourceName: string; + sourceInstance: any; + notification: Notification; +} + +export function verifyOutput(output: EffectNotification, reporter: ErrorReporter) { + reportErrorThrown(output, reporter); + reportInvalidActions(output, reporter); +} + +function reportErrorThrown(output: EffectNotification, reporter: ErrorReporter) { + if (output.notification.kind === 'E') { + const errorReason = `Effect ${getEffectName(output)} threw an error`; + + reporter.report(errorReason, { + Source: output.sourceInstance, + Effect: output.effect, + Error: output.notification.error, + Notification: output.notification, + }); + } +} + +function reportInvalidActions(output: EffectNotification, reporter: ErrorReporter) { + if (output.notification.kind === 'N') { + const action = output.notification.value; + const isInvalidAction = !isAction(action); + + if (isInvalidAction) { + const errorReason = `Effect ${getEffectName(output)} dispatched an invalid action`; + + reporter.report(errorReason, { + Source: output.sourceInstance, + Effect: output.effect, + Dispatched: action, + Notification: output.notification, + }); + } + } +} + +function isAction(action: any): action is Action { + return action && action.type && typeof action.type === 'string'; +} + +function getEffectName({ propertyName, sourceInstance, sourceName }: EffectNotification) { + const isMethod = typeof sourceInstance[propertyName] === 'function'; + + return `"${sourceName}.${propertyName}${isMethod ? '()' : ''}"`; +} diff --git a/modules/effects/src/effect_sources.ts b/modules/effects/src/effect_sources.ts index 766e7485b4..fae96f9647 100644 --- a/modules/effects/src/effect_sources.ts +++ b/modules/effects/src/effect_sources.ts @@ -3,17 +3,21 @@ import { mergeMap } from 'rxjs/operator/mergeMap'; import { exhaustMap } from 'rxjs/operator/exhaustMap'; import { map } from 'rxjs/operator/map'; import { dematerialize } from 'rxjs/operator/dematerialize'; +import { concat } from 'rxjs/observable/concat'; import { Observable } from 'rxjs/Observable'; import { Subject } from 'rxjs/Subject'; -import { Injectable, isDevMode } from '@angular/core'; +import { Injectable } from '@angular/core'; import { Action } from '@ngrx/store'; +import { EffectNotification, verifyOutput } from './effect_notification'; import { getSourceForInstance } from './effects_metadata'; -import { resolveEffectSource, EffectNotification } from './effects_resolver'; +import { resolveEffectSource } from './effects_resolver'; import { ErrorReporter } from './error_reporter'; @Injectable() export class EffectSources extends Subject { - constructor(private errorReporter: ErrorReporter) { + constructor( + private errorReporter: ErrorReporter, + ) { super(); } @@ -32,38 +36,7 @@ export class EffectSources extends Subject { map.call( exhaustMap.call(source$, resolveEffectSource), (output: EffectNotification) => { - switch (output.notification.kind) { - case 'N': { - const action = output.notification.value; - const isInvalidAction = - !action || !action.type || typeof action.type !== 'string'; - - if (isInvalidAction) { - const errorReason = `Effect "${output.sourceName}.${output.propertyName}" dispatched an invalid action`; - - this.errorReporter.report(errorReason, { - Source: output.sourceInstance, - Effect: output.effect, - Dispatched: action, - Notification: output.notification, - }); - } - - break; - } - case 'E': { - const errorReason = `Effect "${output.sourceName}.${output.propertyName}" threw an error`; - - this.errorReporter.report(errorReason, { - Source: output.sourceInstance, - Effect: output.effect, - Error: output.notification.error, - Notification: output.notification, - }); - - break; - } - } + verifyOutput(output, this.errorReporter); return output.notification; }, diff --git a/modules/effects/src/effects_feature_module.ts b/modules/effects/src/effects_feature_module.ts index 3c881d098a..34a46b653a 100644 --- a/modules/effects/src/effects_feature_module.ts +++ b/modules/effects/src/effects_feature_module.ts @@ -1,16 +1,16 @@ import { NgModule, Inject, Type } from '@angular/core'; -import { EffectSources } from './effect_sources'; +import { EffectsRootModule } from './effects_root_module'; import { FEATURE_EFFECTS } from './tokens'; @NgModule({}) export class EffectsFeatureModule { constructor( - private effectSources: EffectSources, + private root: EffectsRootModule, @Inject(FEATURE_EFFECTS) effectSourceGroups: any[][], ) { effectSourceGroups.forEach(group => group.forEach(effectSourceInstance => - effectSources.addEffects(effectSourceInstance), + root.addEffects(effectSourceInstance), ), ); } diff --git a/modules/effects/src/effects_module.ts b/modules/effects/src/effects_module.ts index fa8eb78aea..32c5aafa13 100644 --- a/modules/effects/src/effects_module.ts +++ b/modules/effects/src/effects_module.ts @@ -3,9 +3,9 @@ import { EffectSources } from './effect_sources'; import { Actions } from './actions'; import { ROOT_EFFECTS, FEATURE_EFFECTS, CONSOLE } from './tokens'; import { EffectsFeatureModule } from './effects_feature_module'; +import { EffectsRootModule } from './effects_root_module'; import { EffectsRunner } from './effects_runner'; import { ErrorReporter } from './error_reporter'; -import { RUN_EFFECTS } from './run_effects'; @NgModule({}) export class EffectsModule { @@ -26,13 +26,12 @@ export class EffectsModule { static forRoot(rootEffects: Type[]): ModuleWithProviders { return { - ngModule: EffectsModule, + ngModule: EffectsRootModule, providers: [ EffectsRunner, EffectSources, ErrorReporter, Actions, - RUN_EFFECTS, rootEffects, { provide: ROOT_EFFECTS, diff --git a/modules/effects/src/effects_resolver.ts b/modules/effects/src/effects_resolver.ts index 3cb63bbaa7..3ee6144d6d 100644 --- a/modules/effects/src/effects_resolver.ts +++ b/modules/effects/src/effects_resolver.ts @@ -5,17 +5,10 @@ import { map } from 'rxjs/operator/map'; import { Observable } from 'rxjs/Observable'; import { Notification } from 'rxjs/Notification'; import { Action } from '@ngrx/store'; +import { EffectNotification } from './effect_notification'; import { getSourceMetadata, getSourceForInstance } from './effects_metadata'; import { isOnRunEffects } from './on_run_effects'; -export interface EffectNotification { - effect: Observable | (() => Observable); - propertyName: string; - sourceName: string; - sourceInstance: any; - notification: Notification; -} - export function mergeEffects( sourceInstance: any, ): Observable { diff --git a/modules/effects/src/effects_root_module.ts b/modules/effects/src/effects_root_module.ts new file mode 100644 index 0000000000..1fdfe7eebf --- /dev/null +++ b/modules/effects/src/effects_root_module.ts @@ -0,0 +1,23 @@ +import { NgModule, Inject } from '@angular/core'; +import { EffectsRunner } from './effects_runner'; +import { EffectSources } from './effect_sources'; +import { ROOT_EFFECTS } from './tokens'; + +@NgModule({}) +export class EffectsRootModule { + constructor( + private sources: EffectSources, + runner: EffectsRunner, + @Inject(ROOT_EFFECTS) rootEffects: any[], + ) { + runner.start(); + + rootEffects.forEach(effectSourceInstance => + sources.addEffects(effectSourceInstance) + ); + } + + addEffects(effectSourceInstance: any) { + this.sources.addEffects(effectSourceInstance); + } +} diff --git a/modules/effects/src/on_run_effects.ts b/modules/effects/src/on_run_effects.ts index 9d0f3f4647..96bef7c01a 100644 --- a/modules/effects/src/on_run_effects.ts +++ b/modules/effects/src/on_run_effects.ts @@ -1,6 +1,6 @@ import { Observable } from 'rxjs/Observable'; import { getSourceForInstance } from './effects_metadata'; -import { EffectNotification } from './effects_resolver'; +import { EffectNotification } from './effect_notification'; export interface OnRunEffects { ngrxOnRunEffects( diff --git a/modules/effects/src/run_effects.ts b/modules/effects/src/run_effects.ts deleted file mode 100644 index 8d18c98f5a..0000000000 --- a/modules/effects/src/run_effects.ts +++ /dev/null @@ -1,31 +0,0 @@ -import { - APP_INITIALIZER, - Provider, - Optional, - Type, - Injector, -} from '@angular/core'; -import { EffectsRunner } from './effects_runner'; -import { EffectSources } from './effect_sources'; -import { BOOTSTRAP_EFFECTS, ROOT_EFFECTS } from './tokens'; - -export function createRunEffects( - effectSources: EffectSources, - runner: EffectsRunner, - rootEffects: any[], -) { - return function() { - runner.start(); - - rootEffects.forEach(effectSourceInstance => - effectSources.addEffects(effectSourceInstance), - ); - }; -} - -export const RUN_EFFECTS: Provider = { - provide: APP_INITIALIZER, - multi: true, - deps: [EffectSources, EffectsRunner, ROOT_EFFECTS], - useFactory: createRunEffects, -}; diff --git a/modules/effects/src/tokens.ts b/modules/effects/src/tokens.ts index 04f2abc664..88d04a52d4 100644 --- a/modules/effects/src/tokens.ts +++ b/modules/effects/src/tokens.ts @@ -3,9 +3,6 @@ import { InjectionToken, Type } from '@angular/core'; export const IMMEDIATE_EFFECTS = new InjectionToken( 'ngrx/effects: Immediate Effects', ); -export const BOOTSTRAP_EFFECTS = new InjectionToken( - 'ngrx/effects: Bootstrap Effects', -); export const ROOT_EFFECTS = new InjectionToken[]>( 'ngrx/effects: Root Effects', );