Skip to content

refactor(google-maps): clean up internal setup #21079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 9, 2021
Merged

refactor(google-maps): clean up internal setup #21079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 51 additions & 80 deletions src/google-maps/google-map/google-map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ import {
Inject,
PLATFORM_ID,
NgZone,
SimpleChanges,
} from '@angular/core';
import {isPlatformBrowser} from '@angular/common';
import {BehaviorSubject, combineLatest, Observable, Subject} from 'rxjs';
import {map, shareReplay, take, takeUntil} from 'rxjs/operators';
import {Observable} from 'rxjs';
import {MapEventManager} from '../map-event-manager';

interface GoogleMapsWindow extends Window {
Expand Down Expand Up @@ -57,13 +57,6 @@ export const DEFAULT_WIDTH = '500px';
})
export class GoogleMap implements OnChanges, OnInit, OnDestroy {
private _eventManager: MapEventManager = new MapEventManager(this._ngZone);
private _googleMapChanges: Observable<google.maps.Map>;

private readonly _options = new BehaviorSubject<google.maps.MapOptions>(DEFAULT_OPTIONS);
private readonly _center =
new BehaviorSubject<google.maps.LatLngLiteral|google.maps.LatLng|undefined>(undefined);
private readonly _zoom = new BehaviorSubject<number|undefined>(undefined);
private readonly _destroy = new Subject<void>();
private _mapEl: HTMLElement;

/**
Expand All @@ -90,16 +83,21 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {

@Input()
set center(center: google.maps.LatLngLiteral|google.maps.LatLng) {
this._center.next(center);
this._center = center;
}
private _center: google.maps.LatLngLiteral|google.maps.LatLng;

@Input()
set zoom(zoom: number) {
this._zoom.next(zoom);
this._zoom = zoom;
}
private _zoom: number;

@Input()
set options(options: google.maps.MapOptions) {
this._options.next(options || DEFAULT_OPTIONS);
this._options = options || DEFAULT_OPTIONS;
}
private _options = DEFAULT_OPTIONS;

/**
* See
Expand Down Expand Up @@ -246,10 +244,30 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {
}
}

ngOnChanges() {
this._setSize();
if (this.googleMap && this.mapTypeId) {
this.googleMap.setMapTypeId(this.mapTypeId);
ngOnChanges(changes: SimpleChanges) {
if (changes['height'] || changes['width']) {
this._setSize();
}

const googleMap = this.googleMap;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All things being equal, I think we may as well just continue to reference this.googleMap instead of reassigning it to a local variable.

Copy link
Member Author

@crisbeto crisbeto Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of a micro-optimization, but I wrote it this way because it'll minify better with most common minifiers that don't do property renaming.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment saying that was the reason for doing this?


if (googleMap) {
if (changes['options'] && this._options) {
googleMap.setOptions(this._options);
}

if (changes['center'] && this._center) {
googleMap.setCenter(this._center);
}

// Note that the zoom can be zero.
if (changes['zoom'] && this._zoom != null) {
googleMap.setZoom(this._zoom);
}

if (changes['mapTypeId'] && this.mapTypeId) {
googleMap.setMapTypeId(this.mapTypeId);
}
}
}

Expand All @@ -258,22 +276,19 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {
if (this._isBrowser) {
this._mapEl = this._elementRef.nativeElement.querySelector('.map-container')!;
this._setSize();
this._googleMapChanges = this._initializeMap(this._combineOptions());
this._googleMapChanges.subscribe((googleMap: google.maps.Map) => {
this.googleMap = googleMap;
this._eventManager.setTarget(this.googleMap);
});

this._watchForOptionsChanges();
this._watchForCenterChanges();
this._watchForZoomChanges();
// Create the object outside the zone so its events don't trigger change detection.
// We'll bring it back in inside the `MapEventManager` only for the events that the
// user has subscribed to.
this._ngZone.runOutsideAngular(() => {
this.googleMap = new google.maps.Map(this._mapEl, this._combineOptions());
});
this._eventManager.setTarget(this.googleMap);
}
}

ngOnDestroy() {
this._eventManager.destroy();
this._destroy.next();
this._destroy.complete();
}

/**
Expand Down Expand Up @@ -443,60 +458,16 @@ export class GoogleMap implements OnChanges, OnInit, OnDestroy {
}

/** Combines the center and zoom and the other map options into a single object */
private _combineOptions(): Observable<google.maps.MapOptions> {
return combineLatest([this._options, this._center, this._zoom])
.pipe(map(([options, center, zoom]) => {
const combinedOptions: google.maps.MapOptions = {
...options,
// It's important that we set **some** kind of `center` and `zoom`, otherwise
// Google Maps will render a blank rectangle which looks broken.
center: center || options.center || DEFAULT_OPTIONS.center,
zoom: zoom ?? options.zoom ?? DEFAULT_OPTIONS.zoom,
mapTypeId: this.mapTypeId
};
return combinedOptions;
}));
}

private _initializeMap(optionsChanges: Observable<google.maps.MapOptions>):
Observable<google.maps.Map> {
return optionsChanges.pipe(
take(1),
map(options => {
// Create the object outside the zone so its events don't trigger change detection.
// We'll bring it back in inside the `MapEventManager` only for the events that the
// user has subscribed to.
return this._ngZone.runOutsideAngular(() => new google.maps.Map(this._mapEl, options));
}),
shareReplay(1));
}

private _watchForOptionsChanges() {
combineLatest([this._googleMapChanges, this._options])
.pipe(takeUntil(this._destroy))
.subscribe(([googleMap, options]) => {
googleMap.setOptions(options);
});
}

private _watchForCenterChanges() {
combineLatest([this._googleMapChanges, this._center])
.pipe(takeUntil(this._destroy))
.subscribe(([googleMap, center]) => {
if (center) {
googleMap.setCenter(center);
}
});
}

private _watchForZoomChanges() {
combineLatest([this._googleMapChanges, this._zoom])
.pipe(takeUntil(this._destroy))
.subscribe(([googleMap, zoom]) => {
if (zoom !== undefined) {
googleMap.setZoom(zoom);
}
});
private _combineOptions(): google.maps.MapOptions {
const options = this._options;
return {
...options,
// It's important that we set **some** kind of `center` and `zoom`, otherwise
// Google Maps will render a blank rectangle which looks broken.
center: this._center || options.center || DEFAULT_OPTIONS.center,
zoom: this._zoom ?? options.zoom ?? DEFAULT_OPTIONS.zoom,
mapTypeId: this.mapTypeId
};
}

/** Asserts that the map has been initialized. */
Expand Down
2 changes: 1 addition & 1 deletion tools/public_api_guard/google-maps/google-maps.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export declare class GoogleMap implements OnChanges, OnInit, OnDestroy {
getStreetView(): google.maps.StreetViewPanorama;
getTilt(): number;
getZoom(): number;
ngOnChanges(): void;
ngOnChanges(changes: SimpleChanges): void;
ngOnDestroy(): void;
ngOnInit(): void;
panBy(x: number, y: number): void;
Expand Down