From cc5652380036e77781793af49f3a1627899ffe21 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 27 Oct 2020 21:42:56 +0100 Subject: [PATCH] fix(google-maps): maintain subscriptions across event targets This came up during the review of #20873. Currently the class that manages events for the Google Maps objects assumes that once a target is assigned, it'll either be maintained or eventually removed which means that the fix from #20873 will cause any existing event listeners to be dropped. These changes add some code which will preserve the listeners. Furthermore, I've added a dedicated testing setup for the `MapEventManager` since there's a fair bit of logic encapsulated in it and we've only been testing it by proxy through the various Google Maps components. --- src/google-maps/map-event-manager.spec.ts | 170 ++++++++++++++++++++++ src/google-maps/map-event-manager.ts | 43 +++--- 2 files changed, 194 insertions(+), 19 deletions(-) create mode 100644 src/google-maps/map-event-manager.spec.ts diff --git a/src/google-maps/map-event-manager.spec.ts b/src/google-maps/map-event-manager.spec.ts new file mode 100644 index 000000000000..2d7f2e2dfe50 --- /dev/null +++ b/src/google-maps/map-event-manager.spec.ts @@ -0,0 +1,170 @@ +import {NgZone} from '@angular/core'; +import {MapEventManager} from './map-event-manager'; + +describe('MapEventManager', () => { + let dummyZone: NgZone; + let manager: MapEventManager; + let target: TestEventTarget; + + beforeEach(() => { + dummyZone = { + run: jasmine.createSpy('NgZone.run').and.callFake((callback: () => void) => callback()) + } as unknown as NgZone; + target = new TestEventTarget(); + manager = new MapEventManager(dummyZone); + }); + + afterEach(() => { + manager.destroy(); + }); + + it('should register a listener when subscribing to an event', () => { + expect(target.addListener).not.toHaveBeenCalled(); + + manager.setTarget(target); + const stream = manager.getLazyEmitter('click'); + + expect(target.addListener).not.toHaveBeenCalled(); + expect(target.events.get('click')).toBeFalsy(); + + const subscription = stream.subscribe(); + expect(target.addListener).toHaveBeenCalledTimes(1); + expect(target.events.get('click')?.size).toBe(1); + subscription.unsubscribe(); + }); + + it('should register a listener if the subscription happened before there was a target', () => { + const stream = manager.getLazyEmitter('click'); + const subscription = stream.subscribe(); + + expect(target.addListener).not.toHaveBeenCalled(); + expect(target.events.get('click')).toBeFalsy(); + + manager.setTarget(target); + + expect(target.addListener).toHaveBeenCalledTimes(1); + expect(target.events.get('click')?.size).toBe(1); + subscription.unsubscribe(); + }); + + it('should remove the listener when unsubscribing', () => { + const stream = manager.getLazyEmitter('click'); + const subscription = stream.subscribe(); + manager.setTarget(target); + expect(target.events.get('click')?.size).toBe(1); + + subscription.unsubscribe(); + expect(target.events.get('click')?.size).toBe(0); + }); + + it('should remove the listener when the manager is destroyed', () => { + const stream = manager.getLazyEmitter('click'); + stream.subscribe(); + manager.setTarget(target); + expect(target.events.get('click')?.size).toBe(1); + + manager.destroy(); + expect(target.events.get('click')?.size).toBe(0); + }); + + it('should remove the listener when the target is changed', () => { + const stream = manager.getLazyEmitter('click'); + stream.subscribe(); + manager.setTarget(target); + expect(target.events.get('click')?.size).toBe(1); + + manager.setTarget(undefined); + expect(target.events.get('click')?.size).toBe(0); + }); + + it('should trigger the subscription to an event', () => { + const spy = jasmine.createSpy('subscription'); + const stream = manager.getLazyEmitter('click'); + stream.subscribe(spy); + manager.setTarget(target); + expect(spy).not.toHaveBeenCalled(); + + target.triggerListeners('click'); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('should be able to register multiple listeners to the same event', () => { + const firstSpy = jasmine.createSpy('subscription one'); + const secondSpy = jasmine.createSpy('subscription two'); + const stream = manager.getLazyEmitter('click'); + manager.setTarget(target); + stream.subscribe(firstSpy); + stream.subscribe(secondSpy); + expect(firstSpy).not.toHaveBeenCalled(); + expect(secondSpy).not.toHaveBeenCalled(); + expect(target.events.get('click')?.size).toBe(2); + + target.triggerListeners('click'); + expect(firstSpy).toHaveBeenCalledTimes(1); + expect(secondSpy).toHaveBeenCalledTimes(1); + }); + + it('should run listeners inside the NgZone', () => { + const spy = jasmine.createSpy('subscription'); + const stream = manager.getLazyEmitter('click'); + stream.subscribe(spy); + manager.setTarget(target); + expect(dummyZone.run).not.toHaveBeenCalled(); + + target.triggerListeners('click'); + expect(dummyZone.run).toHaveBeenCalledTimes(1); + }); + + it('should maintain subscriptions when swapping out targets', () => { + const spy = jasmine.createSpy('subscription'); + const stream = manager.getLazyEmitter('click'); + stream.subscribe(spy); + manager.setTarget(target); + expect(spy).not.toHaveBeenCalled(); + + target.triggerListeners('click'); + expect(spy).toHaveBeenCalledTimes(1); + + const alternateTarget = new TestEventTarget(); + manager.setTarget(alternateTarget); + + expect(spy).toHaveBeenCalledTimes(1); + expect(target.events.get('click')?.size).toBe(0); + expect(alternateTarget.events.get('click')?.size).toBe(1); + + alternateTarget.triggerListeners('click'); + expect(spy).toHaveBeenCalledTimes(2); + + manager.setTarget(undefined); + expect(alternateTarget.events.get('click')?.size).toBe(0); + + alternateTarget.triggerListeners('click'); + expect(spy).toHaveBeenCalledTimes(2); + }); + +}); + +/** Imitates a Google Maps event target and keeps track of the registered events. */ +class TestEventTarget { + events = new Map void>>(); + + addListener = jasmine.createSpy('addListener').and.callFake( + (name: string, listener: () => void) => { + if (!this.events.has(name)) { + this.events.set(name, new Set()); + } + this.events.get(name)!.add(listener); + + return {remove: () => this.events.get(name)!.delete(listener)}; + }); + + triggerListeners(name: string) { + const listeners = this.events.get(name); + + if (!listeners) { + throw Error(`No listeners registered for "${name}" event.`); + } + + listeners.forEach(listener => listener()); + } +} diff --git a/src/google-maps/map-event-manager.ts b/src/google-maps/map-event-manager.ts index 8d84cf4d21df..cdc0d2b5e2f4 100644 --- a/src/google-maps/map-event-manager.ts +++ b/src/google-maps/map-event-manager.ts @@ -7,7 +7,8 @@ */ import {NgZone} from '@angular/core'; -import {Observable, Subscriber} from 'rxjs'; +import {BehaviorSubject, Observable, Subscriber} from 'rxjs'; +import {switchMap} from 'rxjs/operators'; type MapEventManagerTarget = { addListener: (name: string, callback: (...args: any[]) => void) => google.maps.MapsEventListener; @@ -18,11 +19,11 @@ export class MapEventManager { /** Pending listeners that were added before the target was set. */ private _pending: {observable: Observable, observer: Subscriber}[] = []; private _listeners: google.maps.MapsEventListener[] = []; - private _target: MapEventManagerTarget; + private _targetStream = new BehaviorSubject(undefined); /** Clears all currently-registered event listeners. */ private _clearListeners() { - for (let listener of this._listeners) { + for (const listener of this._listeners) { listener.remove(); } @@ -33,36 +34,40 @@ export class MapEventManager { /** Gets an observable that adds an event listener to the map when a consumer subscribes to it. */ getLazyEmitter(name: string): Observable { - const observable = new Observable(observer => { - // If the target hasn't been initialized yet, cache the observer so it can be added later. - if (!this._target) { - this._pending.push({observable, observer}); - return undefined; - } + return this._targetStream.pipe(switchMap(target => { + const observable = new Observable(observer => { + // If the target hasn't been initialized yet, cache the observer so it can be added later. + if (!target) { + this._pending.push({observable, observer}); + return undefined; + } - const listener = this._target.addListener(name, (event: T) => { - this._ngZone.run(() => observer.next(event)); + const listener = target.addListener(name, (event: T) => { + this._ngZone.run(() => observer.next(event)); + }); + this._listeners.push(listener); + return () => listener.remove(); }); - this._listeners.push(listener); - return () => listener.remove(); - }); - return observable; + return observable; + })); } /** Sets the current target that the manager should bind events to. */ setTarget(target: MapEventManagerTarget) { - if (target === this._target) { + const currentTarget = this._targetStream.value; + + if (target === currentTarget) { return; } // Clear the listeners from the pre-existing target. - if (this._target) { + if (currentTarget) { this._clearListeners(); this._pending = []; } - this._target = target; + this._targetStream.next(target); // Add the listeners that were bound before the map was initialized. this._pending.forEach(subscriber => subscriber.observable.subscribe(subscriber.observer)); @@ -73,6 +78,6 @@ export class MapEventManager { destroy() { this._clearListeners(); this._pending = []; - this._target = undefined; + this._targetStream.complete(); } }