refactor(google-maps): Removes reference to GoogleMap from MapMarker

mbehrlich · mbehrlich · commit 5fcf26f6ca71 · 2019-09-27T18:42:44.000-07:00
Get access to the GoogleMap parent component from a MapMarker through
the constructor instead of calling a "setMap" method on the marker from
the GoogleMap to prevent MapMarker being loaded in all maps.
diff --git a/src/google-maps/google-map/google-map.spec.ts b/src/google-maps/google-map/google-map.spec.ts
@@ -2,7 +2,6 @@ import {Component} from '@angular/core';
 import {async, TestBed} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 
-import {MapMarker, MapMarkerModule} from '../map-marker/index';
 import {
   createMapConstructorSpy,
   createMapSpy,
@@ -40,7 +39,6 @@ describe('GoogleMap', () => {
     TestBed.configureTestingModule({
       imports: [
         GoogleMapModule,
-        MapMarkerModule,
       ],
       declarations: [TestApp],
     });
@@ -242,18 +240,6 @@ describe('GoogleMap', () => {
     expect(mapSpy.addListener).not.toHaveBeenCalledWith('tilt_changed', jasmine.any(Function));
     expect(mapSpy.addListener).not.toHaveBeenCalledWith('zoom_changed', jasmine.any(Function));
   });
-
-  it('calls setMap on child marker components', () => {
-    mapSpy = createMapSpy(DEFAULT_OPTIONS);
-    createMapConstructorSpy(mapSpy).and.callThrough();
-
-    const fixture = TestBed.createComponent(TestApp);
-    const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    spyOn(markerComponent, '_setMap').and.callThrough();
-    fixture.detectChanges();
-
-    expect(markerComponent._setMap).toHaveBeenCalledWith(mapSpy);
-  });
 });
 
 @Component({
@@ -266,7 +252,6 @@ describe('GoogleMap', () => {
                          (mapClick)="handleClick($event)"
                          (centerChanged)="handleCenterChanged()"
                          (mapRightclick)="handleRightclick($event)">
-              <map-marker></map-marker>
             </google-map>`,
 })
 class TestApp {
diff --git a/src/google-maps/google-map/google-map.ts b/src/google-maps/google-map/google-map.ts
@@ -7,18 +7,15 @@
  */
 
 import {
-  AfterContentInit,
   ChangeDetectionStrategy,
   Component,
-  ContentChildren,
   ElementRef,
   EventEmitter,
   Input,
   OnChanges,
   OnDestroy,
   OnInit,
   Output,
-  QueryList,
   ViewEncapsulation,
 } from '@angular/core';
 import {BehaviorSubject, combineLatest, Observable, Subject} from 'rxjs';
@@ -62,7 +59,7 @@ export const DEFAULT_WIDTH = '500px';
   template: '<div class="map-container"></div><ng-content></ng-content>',
   encapsulation: ViewEncapsulation.None,
 })
-export class GoogleMap implements OnChanges, OnInit, AfterContentInit, OnDestroy {
+export class GoogleMap implements OnChanges, OnInit, OnDestroy {
   @Input() height = DEFAULT_HEIGHT;
 
   @Input() width = DEFAULT_WIDTH;
@@ -188,8 +185,6 @@ export class GoogleMap implements OnChanges, OnInit, AfterContentInit, OnDestroy
    */
   @Output() zoomChanged = new EventEmitter<void>();
 
-  @ContentChildren(MapMarker) _markers: QueryList<MapMarker>;
-
   private _mapEl: HTMLElement;
   _googleMap!: UpdatedGoogleMap;
 
@@ -234,13 +229,6 @@ export class GoogleMap implements OnChanges, OnInit, AfterContentInit, OnDestroy
     this._watchForOptionsChanges(combinedOptionsChanges);
   }
 
-  ngAfterContentInit() {
-    for (const marker of this._markers.toArray()) {
-      marker._setMap(this._googleMap);
-    }
-    this._watchForMarkerChanges();
-  }
-
   ngOnDestroy() {
     this._destroy.next();
     this._destroy.complete();
@@ -472,14 +460,4 @@ export class GoogleMap implements OnChanges, OnInit, AfterContentInit, OnDestroy
           }));
     }
   }
-
-  private _watchForMarkerChanges() {
-    combineLatest(this._googleMapChanges, this._markers.changes)
-        .pipe(takeUntil(this._destroy))
-        .subscribe(([googleMap, markers]) => {
-          for (let marker of markers) {
-            marker._setMap(googleMap);
-          }
-        });
-  }
 }
diff --git a/src/google-maps/map-marker/map-marker.spec.ts b/src/google-maps/map-marker/map-marker.spec.ts
@@ -2,7 +2,10 @@ import {Component} from '@angular/core';
 import {async, TestBed} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 
+import {DEFAULT_OPTIONS, GoogleMapModule, UpdatedGoogleMap} from '../google-map/index';
 import {
+  createMapConstructorSpy,
+  createMapSpy,
   createMarkerConstructorSpy,
   createMarkerSpy,
   TestingWindow
@@ -11,15 +14,23 @@ import {
 import {DEFAULT_MARKER_OPTIONS, MapMarker, MapMarkerModule} from './index';
 
 describe('MapMarker', () => {
+  let mapSpy: jasmine.SpyObj<UpdatedGoogleMap>;
+
   beforeEach(async(() => {
     TestBed.configureTestingModule({
-      imports: [MapMarkerModule],
+      imports: [
+        GoogleMapModule,
+        MapMarkerModule,
+      ],
       declarations: [TestApp],
     });
   }));
 
   beforeEach(() => {
     TestBed.compileComponents();
+
+    mapSpy = createMapSpy(DEFAULT_OPTIONS);
+    createMapConstructorSpy(mapSpy).and.callThrough();
   });
 
   afterEach(() => {
@@ -32,28 +43,24 @@ describe('MapMarker', () => {
     const markerConstructorSpy = createMarkerConstructorSpy(markerSpy).and.callThrough();
 
     const fixture = TestBed.createComponent(TestApp);
-    const fakeMap = {} as unknown as google.maps.Map;
-    const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    markerComponent._setMap(fakeMap);
     fixture.detectChanges();
 
     expect(markerConstructorSpy).toHaveBeenCalledWith({
       ...DEFAULT_MARKER_OPTIONS,
       title: undefined,
       label: undefined,
       clickable: undefined,
-      map: fakeMap
+      map: mapSpy,
     });
   });
 
   it('sets marker inputs', () => {
-    const fakeMap = {} as unknown as google.maps.Map;
     const options: google.maps.MarkerOptions = {
       position: {lat: 3, lng: 5},
       title: 'marker title',
       label: 'marker label',
       clickable: false,
-      map: fakeMap,
+      map: mapSpy,
     };
     const markerSpy = createMarkerSpy(options);
     const markerConstructorSpy = createMarkerConstructorSpy(markerSpy).and.callThrough();
@@ -63,15 +70,12 @@ describe('MapMarker', () => {
     fixture.componentInstance.title = options.title;
     fixture.componentInstance.label = options.label;
     fixture.componentInstance.clickable = options.clickable;
-    const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    markerComponent._setMap(fakeMap);
     fixture.detectChanges();
 
     expect(markerConstructorSpy).toHaveBeenCalledWith(options);
   });
 
   it('sets marker options, ignoring map', () => {
-    const fakeMap = {} as unknown as google.maps.Map;
     const options: google.maps.MarkerOptions = {
       position: {lat: 3, lng: 5},
       title: 'marker title',
@@ -84,15 +88,12 @@ describe('MapMarker', () => {
 
     const fixture = TestBed.createComponent(TestApp);
     fixture.componentInstance.options = options;
-    const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    markerComponent._setMap(fakeMap);
     fixture.detectChanges();
 
-    expect(markerConstructorSpy).toHaveBeenCalledWith({...options, map: fakeMap});
+    expect(markerConstructorSpy).toHaveBeenCalledWith({...options, map: mapSpy});
   });
 
   it('gives precedence to specific inputs over options', () => {
-    const fakeMap = {} as unknown as google.maps.Map;
     const options: google.maps.MarkerOptions = {
       position: {lat: 3, lng: 5},
       title: 'marker title',
@@ -106,7 +107,7 @@ describe('MapMarker', () => {
       label: 'updated label',
       clickable: true,
       icon: 'icon name',
-      map: fakeMap,
+      map: mapSpy,
     };
     const markerSpy = createMarkerSpy(options);
     const markerConstructorSpy = createMarkerConstructorSpy(markerSpy).and.callThrough();
@@ -117,43 +118,17 @@ describe('MapMarker', () => {
     fixture.componentInstance.label = expectedOptions.label;
     fixture.componentInstance.clickable = expectedOptions.clickable;
     fixture.componentInstance.options = options;
-    const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    markerComponent._setMap(fakeMap);
     fixture.detectChanges();
 
     expect(markerConstructorSpy).toHaveBeenCalledWith(expectedOptions);
   });
 
-  it('sets the map on the marker only once', () => {
-    const markerSpy = createMarkerSpy(DEFAULT_MARKER_OPTIONS);
-    const markerConstructorSpy = createMarkerConstructorSpy(markerSpy).and.callThrough();
-
-    const fixture = TestBed.createComponent(TestApp);
-    const fakeMap = {} as unknown as google.maps.Map;
-    const fakeMap2 = {testValue: 'test'} as unknown as google.maps.Map;
-    const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    markerComponent._setMap(fakeMap);
-    markerComponent._setMap(fakeMap2);
-    fixture.detectChanges();
-
-    expect(markerConstructorSpy).toHaveBeenCalledWith({
-      ...DEFAULT_MARKER_OPTIONS,
-      title: undefined,
-      label: undefined,
-      clickable: undefined,
-      map: fakeMap
-    });
-    expect(markerSpy.setOptions).not.toHaveBeenCalled();
-  });
-
   it('exposes methods that provide information about the marker', () => {
     const markerSpy = createMarkerSpy(DEFAULT_MARKER_OPTIONS);
     createMarkerConstructorSpy(markerSpy).and.callThrough();
 
     const fixture = TestBed.createComponent(TestApp);
-    const fakeMap = {} as unknown as google.maps.Map;
     const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    markerComponent._setMap(fakeMap);
     fixture.detectChanges();
 
     markerSpy.getAnimation.and.returnValue(null);
@@ -198,9 +173,7 @@ describe('MapMarker', () => {
     createMarkerConstructorSpy(markerSpy).and.callThrough();
 
     const fixture = TestBed.createComponent(TestApp);
-    const fakeMap = {} as unknown as google.maps.Map;
     const markerComponent = fixture.debugElement.query(By.directive(MapMarker)).componentInstance;
-    markerComponent._setMap(fakeMap);
     fixture.detectChanges();
 
     expect(markerSpy.addListener)
@@ -233,13 +206,16 @@ describe('MapMarker', () => {
 
 @Component({
   selector: 'test-app',
-  template: `<map-marker [title]="title"
-                         [position]="position"
-                         [label]="label"
-                         [clickable]="clickable"
-                         [options]="options"
-                         (mapClick)="handleClick()"
-                         (positionChanged)="handlePositionChanged()"></map-marker>`,
+  template: `<google-map>
+               <map-marker [title]="title"
+                           [position]="position"
+                           [label]="label"
+                           [clickable]="clickable"
+                           [options]="options"
+                           (mapClick)="handleClick()"
+                           (positionChanged)="handlePositionChanged()">
+               </map-marker>
+             </google-map>`,
 })
 class TestApp {
   title?: string;
diff --git a/src/google-maps/map-marker/map-marker.ts b/src/google-maps/map-marker/map-marker.ts
@@ -16,9 +16,11 @@ import {
   Output,
   ViewEncapsulation
 } from '@angular/core';
-import {BehaviorSubject, combineLatest, Observable, ReplaySubject, Subject} from 'rxjs';
+import {BehaviorSubject, combineLatest, Observable, Subject} from 'rxjs';
 import {map, takeUntil} from 'rxjs/operators';
 
+import {GoogleMap} from '../google-map/index';
+
 /**
  * Default options for the Google Maps marker component. Displays a marker
  * at the Googleplex.
@@ -198,30 +200,26 @@ export class MapMarker implements OnInit, OnDestroy {
       new BehaviorSubject<string|google.maps.MarkerLabel|undefined>(undefined);
   private readonly _clickable = new BehaviorSubject<boolean|undefined>(undefined);
 
-  private readonly _map = new ReplaySubject<google.maps.Map>(1);
-
   private readonly _destroy = new Subject<void>();
 
   private readonly _listeners: google.maps.MapsEventListener[] = [];
 
-  private _hasMap = false;
-
   _marker?: google.maps.Marker;
 
+  constructor(private readonly googleMap: GoogleMap) {}
+
   ngOnInit() {
     const combinedOptionsChanges = this._combineOptions();
 
-    combineLatest(this._map, combinedOptionsChanges)
-        .pipe(takeUntil(this._destroy))
-        .subscribe(([googleMap, options]) => {
-          if (this._marker) {
-            this._marker.setOptions(options);
-          } else {
-            this._marker = new google.maps.Marker(options);
-            this._marker.setMap(googleMap);
-            this._initializeEventHandlers();
-          }
-        });
+    combinedOptionsChanges.pipe(takeUntil(this._destroy)).subscribe(options => {
+      if (this._marker) {
+        this._marker.setOptions(options);
+      } else {
+        this._marker = new google.maps.Marker(options);
+        this._marker.setMap(this.googleMap._googleMap);
+        this._initializeEventHandlers();
+      }
+    });
   }
 
   ngOnDestroy() {
@@ -235,13 +233,6 @@ export class MapMarker implements OnInit, OnDestroy {
     }
   }
 
-  _setMap(googleMap: google.maps.Map) {
-    if (!this._hasMap) {
-      this._map.next(googleMap);
-      this._hasMap = true;
-    }
-  }
-
   /**
    * See
    * developers.google.com/maps/documentation/javascript/reference/marker#Marker.getAnimation
@@ -339,16 +330,15 @@ export class MapMarker implements OnInit, OnDestroy {
   }
 
   private _combineOptions(): Observable<google.maps.MarkerOptions> {
-    return combineLatest(
-               this._options, this._title, this._position, this._label, this._clickable, this._map)
-        .pipe(map(([options, title, position, label, clickable, googleMap]) => {
+    return combineLatest(this._options, this._title, this._position, this._label, this._clickable)
+        .pipe(map(([options, title, position, label, clickable]) => {
           const combinedOptions: google.maps.MarkerOptions = {
             ...options,
             title: title || options.title,
             position: position || options.position,
             label: label || options.label,
             clickable: clickable !== undefined ? clickable : options.clickable,
-            map: googleMap || null,
+            map: this.googleMap._googleMap || null,
           };
           return combinedOptions;
         }));
diff --git a/src/google-maps/testing/fake-google-map-utils.ts b/src/google-maps/testing/fake-google-map-utils.ts
