From a528320dc05fbe91959f91870535b1bd7487d142 Mon Sep 17 00:00:00 2001 From: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com> Date: Thu, 16 Jan 2020 01:36:54 +0100 Subject: [PATCH] fix(effects): dispatch init action once (#2164) Closes #2106 BREAKING CHANGE: BEFORE: When the effect class was registered, the init action would be dispatched. If the effect was provided in multiple lazy loaded modules, the init action would be dispatched for every module. AFTER: The init action is only dispatched once The init action is now dispatched based on the identifier of the effect (via ngrxOnIdentifyEffects) --- modules/effects/spec/effect_sources.spec.ts | 87 ++++++++++++++++++++- modules/effects/spec/integration.spec.ts | 7 +- modules/effects/src/effect_sources.ts | 22 ++++-- 3 files changed, 100 insertions(+), 16 deletions(-) diff --git a/modules/effects/spec/effect_sources.spec.ts b/modules/effects/spec/effect_sources.spec.ts index f50797b1f0..b7411ddde0 100644 --- a/modules/effects/spec/effect_sources.spec.ts +++ b/modules/effects/spec/effect_sources.spec.ts @@ -1,8 +1,16 @@ import { ErrorHandler } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { cold, hot, getTestScheduler } from 'jasmine-marbles'; -import { concat, NEVER, Observable, of, throwError, timer } from 'rxjs'; -import { concatMap, map } from 'rxjs/operators'; +import { + concat, + NEVER, + Observable, + of, + throwError, + timer, + Subject, +} from 'rxjs'; +import { map, mapTo } from 'rxjs/operators'; import { Effect, @@ -10,8 +18,11 @@ import { OnIdentifyEffects, OnInitEffects, createEffect, + Actions, } from '../'; +import { EffectsRunner } from '../src/effects_runner'; import { Store } from '@ngrx/store'; +import { ofType } from '../src'; describe('EffectSources', () => { let mockErrorReporter: ErrorHandler; @@ -21,6 +32,7 @@ describe('EffectSources', () => { TestBed.configureTestingModule({ providers: [ EffectSources, + EffectsRunner, { provide: Store, useValue: { @@ -30,6 +42,9 @@ describe('EffectSources', () => { ], }); + const effectsRunner = TestBed.get(EffectsRunner); + effectsRunner.start(); + mockErrorReporter = TestBed.get(ErrorHandler); effectSources = TestBed.get(EffectSources); @@ -51,15 +66,83 @@ describe('EffectSources', () => { 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', () => { describe('with @Effect()', () => { const a = { type: 'From Source A' }; diff --git a/modules/effects/spec/integration.spec.ts b/modules/effects/spec/integration.spec.ts index fe58788a17..2a02f895a9 100644 --- a/modules/effects/spec/integration.spec.ts +++ b/modules/effects/spec/integration.spec.ts @@ -49,7 +49,7 @@ describe('NgRx Effects Integration spec', () => { }); it('should dispatch init actions in the correct order', () => { - expect(dispatch.calls.count()).toBe(7); + expect(dispatch.calls.count()).toBe(6); // All of the root effects init actions are dispatched first expect(dispatch.calls.argsFor(0)).toEqual([ @@ -73,11 +73,6 @@ describe('NgRx Effects Integration spec', () => { expect(dispatch.calls.argsFor(5)).toEqual([ { type: '[FeatEffectWithIdentifierAndInitAction]: INIT' }, ]); - - // While the effect has the same identifier the init effect action is still being dispatched - expect(dispatch.calls.argsFor(6)).toEqual([ - { type: '[FeatEffectWithIdentifierAndInitAction]: INIT' }, - ]); }); it('throws if forRoot() is used more than once', (done: DoneFn) => { diff --git a/modules/effects/src/effect_sources.ts b/modules/effects/src/effect_sources.ts index 97796dfc45..db7099cec0 100644 --- a/modules/effects/src/effect_sources.ts +++ b/modules/effects/src/effect_sources.ts @@ -8,6 +8,7 @@ import { groupBy, map, mergeMap, + tap, } from 'rxjs/operators'; import { @@ -31,13 +32,6 @@ export class EffectSources extends Subject { addEffects(effectSourceInstance: any): void { this.next(effectSourceInstance); - - if ( - onInitEffects in effectSourceInstance && - typeof effectSourceInstance[onInitEffects] === 'function' - ) { - this.store.dispatch(effectSourceInstance[onInitEffects]()); - } } /** @@ -46,7 +40,19 @@ export class EffectSources extends Subject { toActions(): Observable { return this.pipe( groupBy(getSourceForInstance), - mergeMap(source$ => source$.pipe(groupBy(effectsInstance))), + mergeMap(source$ => { + return source$.pipe( + groupBy(effectsInstance), + tap(() => { + if ( + onInitEffects in source$.key && + typeof source$.key[onInitEffects] === 'function' + ) { + this.store.dispatch(source$.key.ngrxOnInitEffects()); + } + }) + ); + }), mergeMap(source$ => source$.pipe( exhaustMap(resolveEffectSource(this.errorHandler)),