Skip to content

Commit

Permalink
fix(google-maps): maintain subscriptions across event targets (#20897)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
crisbeto committed Oct 28, 2020
1 parent 5f2407e commit cc503c4
Show file tree
Hide file tree
Showing 2 changed files with 194 additions and 19 deletions.
170 changes: 170 additions & 0 deletions src/google-maps/map-event-manager.spec.ts
Original file line number Diff line number Diff line change
@@ -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<string, Set<() => 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());
}
}
43 changes: 24 additions & 19 deletions src/google-maps/map-event-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -18,11 +19,11 @@ export class MapEventManager {
/** Pending listeners that were added before the target was set. */
private _pending: {observable: Observable<any>, observer: Subscriber<any>}[] = [];
private _listeners: google.maps.MapsEventListener[] = [];
private _target: MapEventManagerTarget;
private _targetStream = new BehaviorSubject<MapEventManagerTarget>(undefined);

/** Clears all currently-registered event listeners. */
private _clearListeners() {
for (let listener of this._listeners) {
for (const listener of this._listeners) {
listener.remove();
}

Expand All @@ -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<T>(name: string): Observable<T> {
const observable = new Observable<T>(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<T>(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));
Expand All @@ -73,6 +78,6 @@ export class MapEventManager {
destroy() {
this._clearListeners();
this._pending = [];
this._target = undefined;
this._targetStream.complete();
}
}

0 comments on commit cc503c4

Please sign in to comment.