From 089abdcc4b2196e59d4d9f14f158b10f166f73dd Mon Sep 17 00:00:00 2001 From: Elwyn Date: Tue, 9 May 2017 01:24:25 +1200 Subject: [PATCH] feat(Effects): Ensure effects are only subscribed to once --- build/util.ts | 2 +- docs/effects/README.md | 18 ++++++++-- docs/effects/api.md | 8 +++++ .../effects/spec/effects-subscription.spec.ts | 33 ++++++++++++++++--- .../spec/singleton-effects.service.spec.ts | 32 ++++++++++++++++++ modules/effects/src/effects-subscription.ts | 4 +++ modules/effects/src/effects.module.ts | 10 ++++++ .../effects/src/singleton-effects.service.ts | 17 ++++++++++ 8 files changed, 115 insertions(+), 9 deletions(-) create mode 100644 modules/effects/spec/singleton-effects.service.spec.ts create mode 100644 modules/effects/src/singleton-effects.service.ts diff --git a/build/util.ts b/build/util.ts index 5fd098b370..8fb55ebb66 100644 --- a/build/util.ts +++ b/build/util.ts @@ -85,7 +85,7 @@ export async function ignoreErrors(promise: Promise): Promise { } export function fromNpm(command: string) { - return `./node_modules/.bin/${command}`; + return path.normalize(`./node_modules/.bin/${command}`); } export function getPackageFilePath(pkg: string, filename: string) { diff --git a/docs/effects/README.md b/docs/effects/README.md index f3c44a41e8..02d9987fa1 100644 --- a/docs/effects/README.md +++ b/docs/effects/README.md @@ -36,7 +36,19 @@ store. The `ofType` operator lets you filter for actions of a certain type in wh want to use to perform a side effect. ## Example -1. Create an AuthEffects service that describes a source of login actions: +1. Register the EffectsModule in your application root imports: +```ts +import { EffectsModule } from '@ngrx/effects'; + +@NgModule({ + imports: [ + EffectsModule.forRoot() + ] +}) +export class AppModule { } +``` + +2. Create an AuthEffects service that describes a source of login actions: ```ts // ./effects/auth.ts @@ -70,7 +82,7 @@ export class AuthEffects { } ``` -2. Register your effects via `EffectsModule.run` method in your module's `imports`: +3. Register your effects via `EffectsModule.run` method in your module's `imports`: ```ts import { EffectsModule } from '@ngrx/effects'; @@ -81,7 +93,7 @@ import { AuthEffects } from './effects/auth'; EffectsModule.run(AuthEffects) ] }) -export class AppModule { } +export class YourAuthModule { } ``` ## API Documentation diff --git a/docs/effects/api.md b/docs/effects/api.md index b50ed6be48..5194a77ad7 100644 --- a/docs/effects/api.md +++ b/docs/effects/api.md @@ -4,11 +4,19 @@ Feature module for @ngrx/effects. +### forRoot +Registers internal @ngrx/effects services to run in your application. +This is required once in your root app module. + ### run Registers an effects class to run when the target module bootstraps. Call once per each effect class you want to run. +Note that running an effects class multiple times (for example via +different lazy loaded modules) will not cause Effects to run multiple +times. + Usage: ```ts @NgModule({ diff --git a/modules/effects/spec/effects-subscription.spec.ts b/modules/effects/spec/effects-subscription.spec.ts index 6ef28a72a4..0cf0e02927 100644 --- a/modules/effects/spec/effects-subscription.spec.ts +++ b/modules/effects/spec/effects-subscription.spec.ts @@ -2,22 +2,25 @@ import { ReflectiveInjector } from '@angular/core'; import { of } from 'rxjs/observable/of'; import { Effect } from '../src/effects'; import { EffectsSubscription } from '../src/effects-subscription'; +import { SingletonEffectsService } from '../src/singleton-effects.service'; describe('Effects Subscription', () => { it('should add itself to a parent subscription if one exists', () => { const observer: any = { next() { } }; - const root = new EffectsSubscription(observer, undefined, undefined); + const singletonEffectsService = new SingletonEffectsService(); + const root = new EffectsSubscription(observer, singletonEffectsService, undefined, undefined); spyOn(root, 'add'); - const child = new EffectsSubscription(observer, root, undefined); + const child = new EffectsSubscription(observer, singletonEffectsService, root, undefined); expect(root.add).toHaveBeenCalledWith(child); }); it('should unsubscribe for all effects when destroyed', () => { const observer: any = { next() { } }; - const subscription = new EffectsSubscription(observer, undefined, undefined); + const singletonEffectsService = new SingletonEffectsService(); + const subscription = new EffectsSubscription(observer, singletonEffectsService, undefined, undefined); spyOn(subscription, 'unsubscribe'); subscription.ngOnDestroy(); @@ -33,12 +36,32 @@ describe('Effects Subscription', () => { } const instance = new Source(); const observer: any = { next: jasmine.createSpy('next') }; + const singletonEffectsService = new SingletonEffectsService(); - const subscription = new EffectsSubscription(observer, undefined, [ instance ]); + const subscription = new EffectsSubscription(observer, singletonEffectsService, undefined, [ instance ]); expect(observer.next).toHaveBeenCalledTimes(3); expect(observer.next).toHaveBeenCalledWith('a'); expect(observer.next).toHaveBeenCalledWith('b'); expect(observer.next).toHaveBeenCalledWith('c'); }); -}); \ No newline at end of file + + it('should not merge duplicate effects instances when a SingletonEffectsService is provided', () => { + class Source { + @Effect() a$ = of('a'); + @Effect() b$ = of('b'); + @Effect() c$ = of('c'); + } + const instance = new Source(); + const observer: any = { next: jasmine.createSpy('next') }; + const singletonEffectsService = new SingletonEffectsService(); + singletonEffectsService.removeExistingAndRegisterNew([ instance ]); + + const subscription = new EffectsSubscription(observer, singletonEffectsService, undefined, [ instance ]); + + expect(observer.next).not.toHaveBeenCalled(); + expect(observer.next).not.toHaveBeenCalledWith('a'); + expect(observer.next).not.toHaveBeenCalledWith('b'); + expect(observer.next).not.toHaveBeenCalledWith('c'); + }); +}); diff --git a/modules/effects/spec/singleton-effects.service.spec.ts b/modules/effects/spec/singleton-effects.service.spec.ts new file mode 100644 index 0000000000..d0ff30b20f --- /dev/null +++ b/modules/effects/spec/singleton-effects.service.spec.ts @@ -0,0 +1,32 @@ +import { of } from 'rxjs/observable/of'; +import { Effect } from '../src/effects'; +import { SingletonEffectsService } from '../src/singleton-effects.service'; + +describe('SingletonEffectsService', () => { + it('should filter out duplicate effect instances and register new ones', () => { + class Source1 { + @Effect() a$ = of('a'); + @Effect() b$ = of('b'); + @Effect() c$ = of('c'); + } + class Source2 { + @Effect() d$ = of('d'); + @Effect() e$ = of('e'); + @Effect() f$ = of('f'); + } + const instance1 = new Source1(); + const instance2 = new Source2(); + let singletonEffectsService = new SingletonEffectsService(); + + let result = singletonEffectsService.removeExistingAndRegisterNew([ instance1 ]); + expect(result).toContain(instance1); + + result = singletonEffectsService.removeExistingAndRegisterNew([ instance1, instance2 ]); + expect(result).not.toContain(instance1); + expect(result).toContain(instance2); + + result = singletonEffectsService.removeExistingAndRegisterNew([ instance1, instance2 ]); + expect(result).not.toContain(instance1); + expect(result).not.toContain(instance2); + }); +}); diff --git a/modules/effects/src/effects-subscription.ts b/modules/effects/src/effects-subscription.ts index 12d4804ac2..75c5ca938b 100644 --- a/modules/effects/src/effects-subscription.ts +++ b/modules/effects/src/effects-subscription.ts @@ -4,6 +4,7 @@ import { Observer } from 'rxjs/Observer'; import { Subscription } from 'rxjs/Subscription'; import { merge } from 'rxjs/observable/merge'; import { mergeEffects } from './effects'; +import { SingletonEffectsService } from './singleton-effects.service'; export const effects = new OpaqueToken('ngrx/effects: Effects'); @@ -12,6 +13,7 @@ export const effects = new OpaqueToken('ngrx/effects: Effects'); export class EffectsSubscription extends Subscription implements OnDestroy { constructor( @Inject(Store) private store: Observer, + @Inject(SingletonEffectsService) private singletonEffectsService: SingletonEffectsService, @Optional() @SkipSelf() public parent?: EffectsSubscription, @Optional() @Inject(effects) effectInstances?: any[] ) { @@ -27,6 +29,8 @@ export class EffectsSubscription extends Subscription implements OnDestroy { } addEffects(effectInstances: any[]) { + effectInstances = this.singletonEffectsService.removeExistingAndRegisterNew(effectInstances); + const sources = effectInstances.map(mergeEffects); const merged = merge(...sources); diff --git a/modules/effects/src/effects.module.ts b/modules/effects/src/effects.module.ts index ea2ff29857..cfeb587096 100644 --- a/modules/effects/src/effects.module.ts +++ b/modules/effects/src/effects.module.ts @@ -2,6 +2,7 @@ import { NgModule, Injector, Type, APP_BOOTSTRAP_LISTENER, OpaqueToken } from '@ import { Actions } from './actions'; import { EffectsSubscription, effects } from './effects-subscription'; import { runAfterBootstrapEffects, afterBootstrapEffects } from './bootstrap-listener'; +import { SingletonEffectsService } from './singleton-effects.service'; @NgModule({ @@ -17,6 +18,15 @@ import { runAfterBootstrapEffects, afterBootstrapEffects } from './bootstrap-lis ] }) export class EffectsModule { + static forRoot() { + return { + ngModule: EffectsModule, + providers: [ + SingletonEffectsService + ] + }; + } + static run(type: Type) { return { ngModule: EffectsModule, diff --git a/modules/effects/src/singleton-effects.service.ts b/modules/effects/src/singleton-effects.service.ts new file mode 100644 index 0000000000..44eb744971 --- /dev/null +++ b/modules/effects/src/singleton-effects.service.ts @@ -0,0 +1,17 @@ +import { Injectable } from '@angular/core'; + +@Injectable() +export class SingletonEffectsService { + private registeredEffects: string[] = []; + + removeExistingAndRegisterNew (effectInstances: any[]): any[] { + return effectInstances.filter(instance => { + const instanceAsString = instance.constructor.toString(); + if (this.registeredEffects.indexOf(instanceAsString) === -1) { + this.registeredEffects.push(instanceAsString); + return true; + } + return false; + }); + } +}