From daf1e64a9bc1175de0d4c31041c860e508803143 Mon Sep 17 00:00:00 2001 From: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com> Date: Tue, 18 Feb 2020 14:23:39 +0100 Subject: [PATCH] fix(effects): dispatch OnInitEffects action after registration (#2386) Closes #2373 --- modules/effects/spec/effect_sources.spec.ts | 152 +++++------ modules/effects/spec/integration.spec.ts | 267 ++++++++++++++++---- modules/effects/src/effect_sources.ts | 43 ++-- 3 files changed, 305 insertions(+), 157 deletions(-) diff --git a/modules/effects/spec/effect_sources.spec.ts b/modules/effects/spec/effect_sources.spec.ts index a5354a0f2e..58f72ff39d 100644 --- a/modules/effects/spec/effect_sources.spec.ts +++ b/modules/effects/spec/effect_sources.spec.ts @@ -69,89 +69,6 @@ describe('EffectSources', () => { expect(effectSources.next).toHaveBeenCalledWith(effectSource); }); - it('should dispatch an action on ngrxOnInitEffects after being registered', () => { - class EffectWithInitAction implements OnInitEffects { - ngrxOnInitEffects() { - return { type: '[EffectWithInitAction] Init' }; - } - } - const store = TestBed.get(Store); - - effectSources.addEffects(new EffectWithInitAction()); - - expect(store.dispatch).toHaveBeenCalledTimes(1); - expect(store.dispatch).toHaveBeenCalledWith({ - type: '[EffectWithInitAction] Init', - }); - }); - - it('should dispatch an action on ngrxOnInitEffects after being registered (class has effects)', () => { - class EffectWithInitActionAndEffects implements OnInitEffects { - effectOne = createEffect(() => { - return this.actions$.pipe( - ofType('Action 1'), - mapTo({ type: 'Action 1 Response' }) - ); - }); - effectTwo = createEffect(() => { - return this.actions$.pipe( - ofType('Action 2'), - mapTo({ type: 'Action 2 Response' }) - ); - }); - - ngrxOnInitEffects() { - return { type: '[EffectWithInitAction] Init' }; - } - - constructor(private actions$: Actions) {} - } - const store = TestBed.get(Store); - - effectSources.addEffects(new EffectWithInitActionAndEffects(new Subject())); - - expect(store.dispatch).toHaveBeenCalledTimes(1); - expect(store.dispatch).toHaveBeenCalledWith({ - type: '[EffectWithInitAction] Init', - }); - }); - - it('should only dispatch an action on ngrxOnInitEffects once after being registered', () => { - class EffectWithInitAction implements OnInitEffects { - ngrxOnInitEffects() { - return { type: '[EffectWithInitAction] Init' }; - } - } - const store = TestBed.get(Store); - - effectSources.addEffects(new EffectWithInitAction()); - effectSources.addEffects(new EffectWithInitAction()); - - expect(store.dispatch).toHaveBeenCalledTimes(1); - }); - - it('should dispatch an action on ngrxOnInitEffects multiple times after being registered with different identifiers', () => { - let id = 0; - class EffectWithInitAction implements OnInitEffects, OnIdentifyEffects { - effectId = ''; - ngrxOnIdentifyEffects(): string { - return this.effectId; - } - ngrxOnInitEffects() { - return { type: '[EffectWithInitAction] Init' }; - } - constructor() { - this.effectId = (id++).toString(); - } - } - const store = TestBed.get(Store); - - effectSources.addEffects(new EffectWithInitAction()); - effectSources.addEffects(new EffectWithInitAction()); - - expect(store.dispatch).toHaveBeenCalledTimes(2); - }); - describe('toActions() Operator', () => { function toActions(source: any): Observable { source['errorHandler'] = mockErrorReporter; @@ -413,6 +330,7 @@ describe('EffectSources', () => { const f = null; const i = { type: 'From Source Identifier' }; const i2 = { type: 'From Source Identifier 2' }; + const initAction = { type: '[SourceWithInitAction] Init' }; let circularRef = {} as any; circularRef.circularRef = circularRef; @@ -499,6 +417,35 @@ describe('EffectSources', () => { this.effectIdentifier = identifier; } } + class SourceWithInitAction implements OnInitEffects, OnIdentifyEffects { + effectIdentifier: string; + + ngrxOnInitEffects() { + return initAction; + } + + ngrxOnIdentifyEffects() { + return this.effectIdentifier; + } + + effectOne = createEffect(() => { + return this.actions$.pipe( + ofType('Action 1'), + mapTo({ type: 'Action 1 Response' }) + ); + }); + + effectTwo = createEffect(() => { + return this.actions$.pipe( + ofType('Action 2'), + mapTo({ type: 'Action 2 Response' }) + ); + }); + + constructor(private actions$: Actions, identifier: string = '') { + this.effectIdentifier = identifier; + } + } it('should resolve effects from instances', () => { const sources$ = cold('--a--', { a: new SourceA() }); @@ -553,7 +500,44 @@ describe('EffectSources', () => { c: new SourceWithIdentifier('b'), d: new SourceWithIdentifier2('b'), }); - const expected = cold('--a--b--a--b--', { a: i, b: i2 }); + const expected = cold('--a--b--a--b--', { + a: i, + b: i2, + }); + + const output = toActions(sources$); + + expect(output).toBeObservable(expected); + }); + + it('should start with an action after being registered with OnInitEffects', () => { + const sources$ = cold('--a--', { + a: new SourceWithInitAction(new Subject()), + }); + const expected = cold('--a--', { a: initAction }); + + const output = toActions(sources$); + + expect(output).toBeObservable(expected); + }); + + it('should not start twice for the same instance', () => { + const sources$ = cold('--a--a--', { + a: new SourceWithInitAction(new Subject()), + }); + const expected = cold('--a--', { a: initAction }); + + const output = toActions(sources$); + + expect(output).toBeObservable(expected); + }); + + it('should start twice for the same instance with a different key', () => { + const sources$ = cold('--a--b--', { + a: new SourceWithInitAction(new Subject(), 'a'), + b: new SourceWithInitAction(new Subject(), 'b'), + }); + const expected = cold('--a--a--', { a: initAction }); const output = toActions(sources$); diff --git a/modules/effects/spec/integration.spec.ts b/modules/effects/spec/integration.spec.ts index 2a02f895a9..035da47e22 100644 --- a/modules/effects/spec/integration.spec.ts +++ b/modules/effects/spec/integration.spec.ts @@ -1,101 +1,245 @@ -import { NgModuleFactoryLoader, NgModule } from '@angular/core'; +import { NgModuleFactoryLoader, NgModule, Injectable } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { RouterTestingModule, SpyNgModuleFactoryLoader, } from '@angular/router/testing'; import { Router } from '@angular/router'; -import { Store, Action } from '@ngrx/store'; +import { Action, StoreModule, INIT } from '@ngrx/store'; import { EffectsModule, OnInitEffects, ROOT_EFFECTS_INIT, OnIdentifyEffects, EffectSources, + Actions, } from '..'; +import { ofType, createEffect } from '../src'; +import { mapTo } from 'rxjs/operators'; describe('NgRx Effects Integration spec', () => { - let dispatch: jasmine.Spy; - - beforeEach(() => { + it('throws if forRoot() is used more than once', (done: DoneFn) => { TestBed.configureTestingModule({ imports: [ - EffectsModule.forRoot([ - RootEffectWithInitAction, - RootEffectWithoutLifecycle, - RootEffectWithInitActionWithPayload, - ]), - EffectsModule.forFeature([FeatEffectWithInitAction]), + StoreModule.forRoot({}), + EffectsModule.forRoot([]), RouterTestingModule.withRoutes([]), ], - providers: [ - { - provide: Store, - useValue: { - dispatch: jasmine.createSpy('dispatch'), - }, - }, - ], }); - const store = TestBed.get(Store) as Store; + let router: Router = TestBed.get(Router); + const loader: SpyNgModuleFactoryLoader = TestBed.get(NgModuleFactoryLoader); - const effectSources = TestBed.get(EffectSources) as EffectSources; - effectSources.addEffects(new FeatEffectWithIdentifierAndInitAction('one')); - effectSources.addEffects(new FeatEffectWithIdentifierAndInitAction('two')); - effectSources.addEffects(new FeatEffectWithIdentifierAndInitAction('one')); + loader.stubbedModules = { feature: FeatModuleWithForRoot }; + router.resetConfig([{ path: 'feature-path', loadChildren: 'feature' }]); - dispatch = store.dispatch as jasmine.Spy; + router.navigateByUrl('/feature-path').catch((err: TypeError) => { + expect(err.message).toBe( + 'EffectsModule.forRoot() called twice. Feature modules should use EffectsModule.forFeature() instead.' + ); + done(); + }); }); - it('should dispatch init actions in the correct order', () => { - expect(dispatch.calls.count()).toBe(6); + describe('actions', () => { + const createDispatchedReducer = (dispatchedActions: string[] = []) => ( + state = {}, + action: Action + ) => { + dispatchedActions.push(action.type); + return state; + }; - // All of the root effects init actions are dispatched first - expect(dispatch.calls.argsFor(0)).toEqual([ - { type: '[RootEffectWithInitAction]: INIT' }, - ]); + describe('init actions', () => { + it('should dispatch and react to init effect', () => { + let dispatchedActionsLog: string[] = []; + TestBed.configureTestingModule({ + imports: [ + StoreModule.forRoot({ + dispatched: createDispatchedReducer(dispatchedActionsLog), + }), + EffectsModule.forRoot([EffectWithOnInitAndResponse]), + ], + }); + TestBed.get(EffectSources); - expect(dispatch.calls.argsFor(1)).toEqual([new ActionWithPayload()]); + expect(dispatchedActionsLog).toEqual([ + INIT, - // After all of the root effects are registered, the ROOT_EFFECTS_INIT action is dispatched - expect(dispatch.calls.argsFor(2)).toEqual([{ type: ROOT_EFFECTS_INIT }]); + '[EffectWithOnInitAndResponse]: INIT', + '[EffectWithOnInitAndResponse]: INIT Response', - // After the root effects init, the feature effects are dispatched - expect(dispatch.calls.argsFor(3)).toEqual([ - { type: '[FeatEffectWithInitAction]: INIT' }, - ]); + ROOT_EFFECTS_INIT, + ]); + }); - expect(dispatch.calls.argsFor(4)).toEqual([ - { type: '[FeatEffectWithIdentifierAndInitAction]: INIT' }, - ]); + it('should dispatch once for an instance', () => { + let dispatchedActionsLog: string[] = []; + TestBed.configureTestingModule({ + imports: [ + StoreModule.forRoot({ + dispatched: createDispatchedReducer(dispatchedActionsLog), + }), + EffectsModule.forRoot([ + RootEffectWithInitAction, + RootEffectWithInitAction, + RootEffectWithInitAction2, + ]), + EffectsModule.forFeature([ + RootEffectWithInitAction, + RootEffectWithInitAction2, + ]), + ], + }); + TestBed.get(EffectSources); - expect(dispatch.calls.argsFor(5)).toEqual([ - { type: '[FeatEffectWithIdentifierAndInitAction]: INIT' }, - ]); - }); + expect(dispatchedActionsLog).toEqual([ + INIT, - it('throws if forRoot() is used more than once', (done: DoneFn) => { - let router: Router = TestBed.get(Router); - const loader: SpyNgModuleFactoryLoader = TestBed.get(NgModuleFactoryLoader); + '[RootEffectWithInitAction]: INIT', + '[RootEffectWithInitAction2]: INIT', - loader.stubbedModules = { feature: FeatModuleWithForRoot }; - router.resetConfig([{ path: 'feature-path', loadChildren: 'feature' }]); + ROOT_EFFECTS_INIT, + ]); + }); - router.navigateByUrl('/feature-path').catch((err: TypeError) => { - expect(err.message).toBe( - 'EffectsModule.forRoot() called twice. Feature modules should use EffectsModule.forFeature() instead.' + it('should dispatch once per instance key', () => { + let dispatchedActionsLog: string[] = []; + TestBed.configureTestingModule({ + imports: [ + StoreModule.forRoot({ + dispatched: createDispatchedReducer(dispatchedActionsLog), + }), + EffectsModule.forRoot([]), + ], + }); + const effectsSources = TestBed.inject(EffectSources); + + effectsSources.addEffects( + new FeatEffectWithIdentifierAndInitAction('One') + ); + effectsSources.addEffects( + new FeatEffectWithIdentifierAndInitAction('Two') + ); + effectsSources.addEffects( + new FeatEffectWithIdentifierAndInitAction('One') + ); + effectsSources.addEffects( + new FeatEffectWithIdentifierAndInitAction('Two') + ); + + expect(dispatchedActionsLog).toEqual([ + INIT, + ROOT_EFFECTS_INIT, + + // for One + '[FeatEffectWithIdentifierAndInitAction]: INIT', + // for Two + '[FeatEffectWithIdentifierAndInitAction]: INIT', + ]); + }); + }); + + it('should dispatch actions in the correct order', async () => { + let dispatchedActionsLog: string[] = []; + TestBed.configureTestingModule({ + imports: [ + StoreModule.forRoot({ + dispatched: createDispatchedReducer(dispatchedActionsLog), + }), + EffectsModule.forRoot([ + RootEffectWithInitAction, + EffectWithOnInitAndResponse, + RootEffectWithoutLifecycle, + RootEffectWithInitActionWithPayload, + ]), + EffectsModule.forFeature([FeatEffectWithInitAction]), + RouterTestingModule.withRoutes([]), + ], + }); + + const effectSources = TestBed.get(EffectSources) as EffectSources; + effectSources.addEffects( + new FeatEffectWithIdentifierAndInitAction('one') ); - done(); + effectSources.addEffects( + new FeatEffectWithIdentifierAndInitAction('two') + ); + effectSources.addEffects( + new FeatEffectWithIdentifierAndInitAction('one') + ); + + let router: Router = TestBed.get(Router); + const loader: SpyNgModuleFactoryLoader = TestBed.get( + NgModuleFactoryLoader + ); + + loader.stubbedModules = { feature: FeatModuleWithForFeature }; + router.resetConfig([{ path: 'feature-path', loadChildren: 'feature' }]); + + await router.navigateByUrl('/feature-path'); + + expect(dispatchedActionsLog).toEqual([ + // first store init + INIT, + + // second root effects + '[RootEffectWithInitAction]: INIT', + '[EffectWithOnInitAndResponse]: INIT', + '[EffectWithOnInitAndResponse]: INIT Response', + '[RootEffectWithInitActionWithPayload]: INIT', + + // third effects init + ROOT_EFFECTS_INIT, + + // next feat effects + '[FeatEffectWithInitAction]: INIT', + + // lastly added features (3 effects but 2 unique keys) + '[FeatEffectWithIdentifierAndInitAction]: INIT', + '[FeatEffectWithIdentifierAndInitAction]: INIT', + + // from lazy loaded module + '[FeatEffectFromLazyLoadedModuleWithInitAction]: INIT', + ]); }); }); + @Injectable() + class EffectWithOnInitAndResponse implements OnInitEffects { + ngrxOnInitEffects(): Action { + return { type: '[EffectWithOnInitAndResponse]: INIT' }; + } + + response = createEffect(() => { + return this.actions$.pipe( + ofType('[EffectWithOnInitAndResponse]: INIT'), + mapTo({ type: '[EffectWithOnInitAndResponse]: INIT Response' }) + ); + }); + + noop = createEffect(() => { + return this.actions$.pipe( + ofType('noop'), + mapTo({ type: 'noop response' }) + ); + }); + + constructor(private actions$: Actions) {} + } + class RootEffectWithInitAction implements OnInitEffects { ngrxOnInitEffects(): Action { return { type: '[RootEffectWithInitAction]: INIT' }; } } + class RootEffectWithInitAction2 implements OnInitEffects { + ngrxOnInitEffects(): Action { + return { type: '[RootEffectWithInitAction2]: INIT' }; + } + } + class ActionWithPayload implements Action { readonly type = '[RootEffectWithInitActionWithPayload]: INIT'; readonly payload = 47; @@ -128,8 +272,23 @@ describe('NgRx Effects Integration spec', () => { constructor(private effectIdentifier: string) {} } + class FeatEffectFromLazyLoadedModuleWithInitAction implements OnInitEffects { + ngrxOnInitEffects(): Action { + return { type: '[FeatEffectFromLazyLoadedModuleWithInitAction]: INIT' }; + } + } + @NgModule({ imports: [EffectsModule.forRoot([])], }) class FeatModuleWithForRoot {} + + @NgModule({ + imports: [ + EffectsModule.forFeature([FeatEffectFromLazyLoadedModuleWithInitAction]), + // should not be loaded because it's already loaded in forRoot + EffectsModule.forFeature([FeatEffectWithInitAction]), + ], + }) + class FeatModuleWithForFeature {} }); diff --git a/modules/effects/src/effect_sources.ts b/modules/effects/src/effect_sources.ts index e2b9e8afd1..a6d3b9cf99 100644 --- a/modules/effects/src/effect_sources.ts +++ b/modules/effects/src/effect_sources.ts @@ -1,6 +1,6 @@ import { ErrorHandler, Inject, Injectable } from '@angular/core'; -import { Action, Store } from '@ngrx/store'; -import { Notification, Observable, Subject } from 'rxjs'; +import { Action } from '@ngrx/store'; +import { Notification, Observable, Subject, merge } from 'rxjs'; import { dematerialize, exhaustMap, @@ -8,7 +8,7 @@ import { groupBy, map, mergeMap, - tap, + take, } from 'rxjs/operators'; import { @@ -33,7 +33,6 @@ import { getSourceForInstance } from './utils'; export class EffectSources extends Subject { constructor( private errorHandler: ErrorHandler, - private store: Store, @Inject(EFFECTS_ERROR_HANDLER) private effectsErrorHandler: EffectsErrorHandler ) { @@ -51,20 +50,16 @@ export class EffectSources extends Subject { return this.pipe( groupBy(getSourceForInstance), mergeMap(source$ => { - return source$.pipe( - groupBy(effectsInstance), - tap(() => { - if (isOnInitEffects(source$.key)) { - this.store.dispatch(source$.key.ngrxOnInitEffects()); - } - }) - ); + return source$.pipe(groupBy(effectsInstance)); }), - mergeMap(source$ => - source$.pipe( - exhaustMap( - resolveEffectSource(this.errorHandler, this.effectsErrorHandler) - ), + mergeMap(source$ => { + const effect$ = source$.pipe( + exhaustMap(sourceInstance => { + return resolveEffectSource( + this.errorHandler, + this.effectsErrorHandler + )(sourceInstance); + }), map(output => { reportInvalidActions(output, this.errorHandler); return output.notification; @@ -74,8 +69,18 @@ export class EffectSources extends Subject { notification.kind === 'N' ), dematerialize() - ) - ) + ); + + // start the stream with an INIT action + // do this only for the first Effect instance + const init$ = source$.pipe( + take(1), + filter(isOnInitEffects), + map(instance => instance.ngrxOnInitEffects()) + ); + + return merge(effect$, init$); + }) ); } }