From 5eef56d78bc5b874b9d49999adacb05938197a37 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Fri, 6 Dec 2024 10:08:48 +1000 Subject: [PATCH 01/27] Fix hard failure on google maps error Fixes: #2175 --- .../components/shared/map/map.component.ts | 8 +- .../helpers/embedScript/embedGoogleMaps.ts | 68 ----------------- src/app/services/config/config.service.ts | 2 - src/app/services/maps/maps.service.spec.ts | 18 +++++ src/app/services/maps/maps.service.ts | 74 +++++++++++++++++++ 5 files changed, 98 insertions(+), 72 deletions(-) create mode 100644 src/app/services/maps/maps.service.spec.ts create mode 100644 src/app/services/maps/maps.service.ts diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index 78736f19c..a1262b14b 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -10,8 +10,8 @@ import { ViewChildren, } from "@angular/core"; import { GoogleMap, MapInfoWindow, MapMarker } from "@angular/google-maps"; -import { googleMapsLoaded } from "@helpers/embedScript/embedGoogleMaps"; import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; +import { MapsService } from "@services/maps/maps.service"; import { List } from "immutable"; import { takeUntil } from "rxjs/operators"; @@ -57,6 +57,10 @@ export class MapComponent extends withUnsubscribe() implements OnChanges, AfterViewChecked { + public constructor(private maps: MapsService) { + super(); + } + @ViewChild(GoogleMap, { static: false }) public map: GoogleMap; @ViewChild(MapInfoWindow, { static: false }) public info: MapInfoWindow; @ViewChildren(MapMarker) public mapMarkers: QueryList; @@ -75,7 +79,7 @@ export class MapComponent private updateMap: boolean; public get googleMapsLoaded(): boolean { - return googleMapsLoaded(); + return this.maps.googleMapsLoaded(); } public ngOnChanges(): void { diff --git a/src/app/helpers/embedScript/embedGoogleMaps.ts b/src/app/helpers/embedScript/embedGoogleMaps.ts index 258cee694..e69de29bb 100644 --- a/src/app/helpers/embedScript/embedGoogleMaps.ts +++ b/src/app/helpers/embedScript/embedGoogleMaps.ts @@ -1,68 +0,0 @@ -import { defaultDebounceTime } from "src/app/app.helper"; -import { environment } from "src/environments/environment"; - -declare let google: any; - -export const googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; - -/** - * Embed google maps script into the document. This makes use of the document - * so it is not compatible with SSR, and is dangerous to use in tests - * - * @param key Google maps API key - */ -export async function embedGoogleMaps(key?: string): Promise { - // Do not insert during testing - if (environment.testing) { - return; - } - - let googleMapsUrl = googleMapsBaseUrl; - if (key) { - googleMapsUrl += "?key=" + key; - } - - const node: HTMLScriptElement = document.createElement("script"); - node.id = "google-maps"; - node.type = "text/javascript"; - node.async = true; - node.src = googleMapsUrl; - document.getElementsByTagName("head")[0].appendChild(node); - - // Detect when google maps properly embeds - await new Promise((resolve, reject) => { - let count = 0; - - function mapLoaded(): void { - if (typeof google !== "undefined") { - resolve(); - } else if (count > 10) { - console.error("Failed to load google maps."); - reject("Google Maps API Bundle took too long to download."); - } else { - count++; - setTimeout(() => mapLoaded(), defaultDebounceTime); - } - } - - mapLoaded(); - }); -} - -/** - * Remove the google maps script from the document. This should - * only be accessed by unit tests. - */ -export function destroyGoogleMaps(): void { - document.getElementById("google-maps").remove(); -} - -export function googleMapsLoaded(): boolean { - let isDefined = false; - try { - isDefined = !!google; - } catch (err) { - console.error(err); - } - return isDefined; -} diff --git a/src/app/services/config/config.service.ts b/src/app/services/config/config.service.ts index 7ba9af88b..419138377 100644 --- a/src/app/services/config/config.service.ts +++ b/src/app/services/config/config.service.ts @@ -9,7 +9,6 @@ import { Settings, } from "@helpers/app-initializer/app-initializer"; import { embedGoogleAnalytics } from "@helpers/embedScript/embedGoogleAnalytics"; -import { embedGoogleMaps } from "@helpers/embedScript/embedGoogleMaps"; import { ThemeService } from "@services/theme/theme.service"; import { ToastrService } from "ngx-toastr"; import { catchError, firstValueFrom, mergeMap, of, retry } from "rxjs"; @@ -45,7 +44,6 @@ export class ConfigService { // Only insert if valid config, and not SSR if (this.validConfig && !this.isServer) { embedGoogleAnalytics(this.keys.googleAnalytics.trackingId); - await embedGoogleMaps(this.keys.googleMaps); } }; diff --git a/src/app/services/maps/maps.service.spec.ts b/src/app/services/maps/maps.service.spec.ts new file mode 100644 index 000000000..00d3d3875 --- /dev/null +++ b/src/app/services/maps/maps.service.spec.ts @@ -0,0 +1,18 @@ +import { createServiceFactory, SpectatorService } from "@ngneat/spectator"; +import { MapsService } from "./maps.service"; + +describe("MapsService", () => { + let spec: SpectatorService; + + const createService = createServiceFactory({ + service: MapsService, + }); + + beforeEach(() => { + spec = createService(); + }); + + it("should create", () => { + expect(spec.service).toBeInstanceOf(MapsService); + }); +}); diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts new file mode 100644 index 000000000..914e62bbd --- /dev/null +++ b/src/app/services/maps/maps.service.ts @@ -0,0 +1,74 @@ +import { Injectable } from "@angular/core"; +import { defaultDebounceTime } from "src/app/app.helper"; +import { environment } from "src/environments/environment"; + +@Injectable({ providedIn: "root" }) +export class MapsService { + public constructor() { + this.embedGoogleMaps(); + } + + private googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; + + public googleMapsLoaded(): boolean { + let isDefined = false; + try { + isDefined = !!google; + } catch (err) { + console.error(err); + } + return isDefined; + } + + /** + * Embed google maps script into the document. This makes use of the document + * so it is not compatible with SSR, and is dangerous to use in tests + * + * @param key Google maps API key + */ + private async embedGoogleMaps(key?: string): Promise { + // Do not insert during testing + if (environment.testing) { + return; + } + + let googleMapsUrl = this.googleMapsBaseUrl; + if (key) { + googleMapsUrl += "?key=" + key; + } + + const node: HTMLScriptElement = document.createElement("script"); + node.id = "google-maps"; + node.type = "text/javascript"; + node.async = true; + node.src = googleMapsUrl; + document.getElementsByTagName("head")[0].appendChild(node); + + // Detect when google maps properly embeds + await new Promise((resolve, reject) => { + let count = 0; + + function mapLoaded(): void { + if (typeof google !== "undefined") { + resolve(); + } else if (count > 10) { + console.error("Failed to load google maps."); + reject("Google Maps API Bundle took too long to download."); + } else { + count++; + setTimeout(() => mapLoaded(), defaultDebounceTime); + } + } + + mapLoaded(); + }); + } + + /** + * Remove the google maps script from the document. This should + * only be accessed by unit tests. + */ + private destroyGoogleMaps(): void { + document.getElementById("google-maps").remove(); + } +} From 16914fae34248a10378b224322196b6c7a487f75 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Fri, 6 Dec 2024 14:38:44 +1000 Subject: [PATCH 02/27] Small changes --- .../data-request/data-request.component.ts | 3 +- .../components/shared/map/map.component.html | 29 +++++++ .../components/shared/map/map.component.ts | 61 ++++---------- .../helpers/embedScript/embedGoogleMaps.ts | 0 src/app/services/maps/maps.service.ts | 79 ++++++++++++------- 5 files changed, 97 insertions(+), 75 deletions(-) create mode 100644 src/app/components/shared/map/map.component.html delete mode 100644 src/app/helpers/embedScript/embedGoogleMaps.ts diff --git a/src/app/components/data-request/data-request.component.ts b/src/app/components/data-request/data-request.component.ts index 0aecbbdc2..0eb2bed74 100644 --- a/src/app/components/data-request/data-request.component.ts +++ b/src/app/components/data-request/data-request.component.ts @@ -19,14 +19,13 @@ import schema from "./data-request.schema.json";

To download a standard CSV of annotations

    -
  1. Open the site or point page you're interested in
    2. +
  2. Open the project, site, or point page you're interested in
  3. Use the Download Annotations button to download annotations
+ + + + {{ infoContent }} + + +} + +
+ @if(googleMapsLoaded()) { +

Map loading

+ } @else if (maps.mapsState() === mapStates.Failed) { +

Failure loading maps

+ } @else if (!hasMarkers) { +

No locations specified

+ } +
diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index a1262b14b..d59471b71 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -1,6 +1,7 @@ import { AfterViewChecked, Component, + computed, EventEmitter, Input, OnChanges, @@ -11,7 +12,7 @@ import { } from "@angular/core"; import { GoogleMap, MapInfoWindow, MapMarker } from "@angular/google-maps"; import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; -import { MapsService } from "@services/maps/maps.service"; +import { MapsService, MapState } from "@services/maps/maps.service"; import { List } from "immutable"; import { takeUntil } from "rxjs/operators"; @@ -21,48 +22,19 @@ import { takeUntil } from "rxjs/operators"; */ @Component({ selector: "baw-map", - template: ` - - - - - - {{ infoContent }} - - - - - -

Map loading

- - - - -

No locations specified

- - `, - styleUrls: ["./map.component.scss"], + templateUrl: "./map.component.html", + styleUrl: "./map.component.scss", }) export class MapComponent extends withUnsubscribe() implements OnChanges, AfterViewChecked { - public constructor(private maps: MapsService) { + public constructor(protected maps: MapsService) { super(); } - @ViewChild(GoogleMap, { static: false }) public map: GoogleMap; - @ViewChild(MapInfoWindow, { static: false }) public info: MapInfoWindow; + @ViewChild(GoogleMap) public map: GoogleMap; + @ViewChild(MapInfoWindow) public info: MapInfoWindow; @ViewChildren(MapMarker) public mapMarkers: QueryList; @Input() public markers: List; @@ -76,24 +48,24 @@ export class MapComponent // Setting to "hybrid" can increase load times and looks like the map is bugged public mapOptions: MapOptions = { mapTypeId: "satellite" }; public bounds: google.maps.LatLngBounds; + protected googleMapsLoaded = computed( + () => this.maps.mapsState() === MapState.Loaded + ); + protected mapStates = MapState; private updateMap: boolean; - public get googleMapsLoaded(): boolean { - return this.maps.googleMapsLoaded(); - } - public ngOnChanges(): void { this.hasMarkers = false; this.filteredMarkers = []; // Google global may not be declared - if (!this.googleMapsLoaded) { + if (this.maps.mapsState() !== MapState.Loaded) { return; } // Calculate pin boundaries so that map can be auto-focused properly this.bounds = new google.maps.LatLngBounds(); - this.markers?.forEach((marker): void => { + this.markers?.forEach((marker) => { if (isMarkerValid(marker)) { this.hasMarkers = true; this.filteredMarkers.push(marker); @@ -114,12 +86,13 @@ export class MapComponent // Setup info windows for each marker this.mapMarkers?.forEach((marker, index) => { marker.mapMouseover.pipe(takeUntil(this.unsubscribe)).subscribe({ - next: (): void => { + next: () => { this.infoContent = this.filteredMarkers[index].label as string; this.info.open(marker); }, - error: (): void => - console.error("Failed to create info content for map marker"), + error: () => { + console.error("Failed to create info content for map marker"); + }, }); }); } diff --git a/src/app/helpers/embedScript/embedGoogleMaps.ts b/src/app/helpers/embedScript/embedGoogleMaps.ts deleted file mode 100644 index e69de29bb..000000000 diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 914e62bbd..7edda3380 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -1,24 +1,41 @@ -import { Injectable } from "@angular/core"; -import { defaultDebounceTime } from "src/app/app.helper"; +import { Inject, Injectable, signal } from "@angular/core"; +import { defaultDebounceTime, IS_SERVER_PLATFORM } from "src/app/app.helper"; import { environment } from "src/environments/environment"; +export enum MapState { + NotLoaded, + Loading, + Loaded, + Failed, +} + @Injectable({ providedIn: "root" }) export class MapsService { - public constructor() { - this.embedGoogleMaps(); + // By embedding the google maps script in the services constructor, we can + // start loading the script as soon as the service is injected, and we don't + // have to wait for the underlying component to be created. + // + // The underlying component can subscribe to the mapsState signal to know + // when the google maps script has been loaded. + public constructor(@Inject(IS_SERVER_PLATFORM) private isServer: boolean) { + // while angular services are singletons, it is still possible to create + // multiple instances of the service with hacky code + // e.g. new MapsService() + // This is a safeguard to prevent multiple instances of the service + // from embedding the google maps script. It should not be triggered + // in normal use, but is defensive programming against misuse. + if (!MapsService.embeddedService) { + MapsService.embeddedService = true; + this.embedGoogleMaps(); + } else { + console.warn("Google Maps Service already embedded."); + } } - private googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; + private static embeddedService = false; - public googleMapsLoaded(): boolean { - let isDefined = false; - try { - isDefined = !!google; - } catch (err) { - console.error(err); - } - return isDefined; - } + public mapsState = signal(MapState.NotLoaded); + private googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; /** * Embed google maps script into the document. This makes use of the document @@ -27,14 +44,18 @@ export class MapsService { * @param key Google maps API key */ private async embedGoogleMaps(key?: string): Promise { - // Do not insert during testing - if (environment.testing) { + // TODO: during SSR we might be able to render a static image of the map + // using the StaticMapService + // https://developers.google.com/maps/documentation/maps-static/overview + if (environment.testing || this.isServer) { return; } + this.mapsState.set(MapState.Loading); + let googleMapsUrl = this.googleMapsBaseUrl; if (key) { - googleMapsUrl += "?key=" + key; + googleMapsUrl += `?key=${key}&callback=initMap`; } const node: HTMLScriptElement = document.createElement("script"); @@ -42,33 +63,33 @@ export class MapsService { node.type = "text/javascript"; node.async = true; node.src = googleMapsUrl; - document.getElementsByTagName("head")[0].appendChild(node); + + node.addEventListener("error", () => { + this.mapsState.set(MapState.Failed); + }); + + document.head.appendChild(node); // Detect when google maps properly embeds + const instantiationRetries = 10; + await new Promise((resolve, reject) => { let count = 0; - function mapLoaded(): void { + const mapLoaded = () => { if (typeof google !== "undefined") { + this.mapsState.set(MapState.Loaded); resolve(); - } else if (count > 10) { + } else if (count > instantiationRetries) { console.error("Failed to load google maps."); reject("Google Maps API Bundle took too long to download."); } else { count++; setTimeout(() => mapLoaded(), defaultDebounceTime); } - } + }; mapLoaded(); }); } - - /** - * Remove the google maps script from the document. This should - * only be accessed by unit tests. - */ - private destroyGoogleMaps(): void { - document.getElementById("google-maps").remove(); - } } From bb1adcf8b49f406ee021ca6621a013dacf6d7ae5 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Fri, 6 Dec 2024 14:49:11 +1000 Subject: [PATCH 03/27] Implement async google maps loading api --- src/app/services/maps/maps.service.ts | 59 ++++++++++++++++++--------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 7edda3380..504685d9e 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -35,8 +35,23 @@ export class MapsService { private static embeddedService = false; public mapsState = signal(MapState.NotLoaded); + private promises: { + res: (...args: any) => void; + rej: (...args: any) => void; + }[] = []; private googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; + public loadedAsync(): Promise { + if (this.mapsState() === MapState.Loaded) { + return Promise.resolve(); + } + + const newPromise = new Promise((res, rej) => { + this.promises.push({ res, rej }); + }); + return newPromise; + } + /** * Embed google maps script into the document. This makes use of the document * so it is not compatible with SSR, and is dangerous to use in tests @@ -65,31 +80,37 @@ export class MapsService { node.src = googleMapsUrl; node.addEventListener("error", () => { - this.mapsState.set(MapState.Failed); + this.handleGoogleMapsFailed(); }); document.head.appendChild(node); // Detect when google maps properly embeds const instantiationRetries = 10; + let count = 0; - await new Promise((resolve, reject) => { - let count = 0; - - const mapLoaded = () => { - if (typeof google !== "undefined") { - this.mapsState.set(MapState.Loaded); - resolve(); - } else if (count > instantiationRetries) { - console.error("Failed to load google maps."); - reject("Google Maps API Bundle took too long to download."); - } else { - count++; - setTimeout(() => mapLoaded(), defaultDebounceTime); - } - }; - - mapLoaded(); - }); + const mapLoaded = () => { + if (typeof google !== "undefined") { + this.handleGoogleMapsLoaded(); + } else if (count > instantiationRetries) { + console.error("Failed to load google maps."); + this.handleGoogleMapsFailed(); + } else { + count++; + setTimeout(() => mapLoaded(), defaultDebounceTime); + } + }; + + mapLoaded(); + } + + private handleGoogleMapsLoaded(): void { + this.mapsState.set(MapState.Loaded); + this.promises.forEach(({ res }) => res()); + } + + private handleGoogleMapsFailed(): void { + this.mapsState.set(MapState.Failed); + this.promises.forEach(({ rej }) => rej()); } } From d0f3fe08e57cd08d1e841def2ffaf3a174f09d9a Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Fri, 6 Dec 2024 15:22:13 +1000 Subject: [PATCH 04/27] Remove array of signals from maps service --- src/app/services/maps/maps.service.ts | 37 +++++++++++++++++---------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 504685d9e..7c83b3034 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -9,14 +9,17 @@ export enum MapState { Failed, } +interface WrappedPromise { + promise: Promise; + resolve: (...args: any) => void; + reject: (...args: any) => void; +} + @Injectable({ providedIn: "root" }) export class MapsService { // By embedding the google maps script in the services constructor, we can // start loading the script as soon as the service is injected, and we don't // have to wait for the underlying component to be created. - // - // The underlying component can subscribe to the mapsState signal to know - // when the google maps script has been loaded. public constructor(@Inject(IS_SERVER_PLATFORM) private isServer: boolean) { // while angular services are singletons, it is still possible to create // multiple instances of the service with hacky code @@ -26,6 +29,16 @@ export class MapsService { // in normal use, but is defensive programming against misuse. if (!MapsService.embeddedService) { MapsService.embeddedService = true; + + let resolver: (value: unknown) => void; + let rejector: (reason?: unknown) => void; + const promise = new Promise((res, rej) => { + resolver = res; + rejector = rej; + }); + + this.loadPromise = { promise, resolve: resolver, reject: rejector }; + this.embedGoogleMaps(); } else { console.warn("Google Maps Service already embedded."); @@ -35,21 +48,17 @@ export class MapsService { private static embeddedService = false; public mapsState = signal(MapState.NotLoaded); - private promises: { - res: (...args: any) => void; - rej: (...args: any) => void; - }[] = []; + private loadPromise: WrappedPromise; private googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; - public loadedAsync(): Promise { + public loadAsync(): Promise { if (this.mapsState() === MapState.Loaded) { return Promise.resolve(); + } else if (this.mapsState() === MapState.Failed) { + return Promise.reject(); } - const newPromise = new Promise((res, rej) => { - this.promises.push({ res, rej }); - }); - return newPromise; + return this.loadPromise.promise; } /** @@ -106,11 +115,11 @@ export class MapsService { private handleGoogleMapsLoaded(): void { this.mapsState.set(MapState.Loaded); - this.promises.forEach(({ res }) => res()); + this.loadPromise.resolve(); } private handleGoogleMapsFailed(): void { this.mapsState.set(MapState.Failed); - this.promises.forEach(({ rej }) => rej()); + this.loadPromise.reject(); } } From 90c3a0b4b81f22e948c5a1406653111dddb996e5 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Fri, 6 Dec 2024 15:31:37 +1000 Subject: [PATCH 05/27] Add comments to document shared promises --- src/app/services/maps/maps.service.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 7c83b3034..5314078cd 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -9,7 +9,7 @@ export enum MapState { Failed, } -interface WrappedPromise { +interface SharedPromise { promise: Promise; resolve: (...args: any) => void; reject: (...args: any) => void; @@ -47,11 +47,19 @@ export class MapsService { private static embeddedService = false; - public mapsState = signal(MapState.NotLoaded); - private loadPromise: WrappedPromise; - private googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; + public readonly mapsState = signal(MapState.NotLoaded); + private readonly loadPromise: SharedPromise ; + private readonly googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; public loadAsync(): Promise { + // if we have previously loaded the maps, return immediately with the + // correct resolved/rejected state + // + // we obviously can't return "this.loadPromise" directly as it'd never + // resolve because the res() and rej() functions would not be called because + // the maps loaded state has already settled + // (promises only resolve/reject once and do not emit their last value + // to new awaiters) if (this.mapsState() === MapState.Loaded) { return Promise.resolve(); } else if (this.mapsState() === MapState.Failed) { From deea6ffa75ab20e7d207c29143ea647acf269791 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 10:50:54 +1000 Subject: [PATCH 06/27] Staging state and test changes --- .../components/projects/projects.module.ts | 3 +- .../formly/location-input.component.spec.ts | 3 - .../components/shared/map/map.component.html | 48 ++++++------- .../components/shared/map/map.component.scss | 12 +++- .../shared/map/map.component.spec.ts | 70 ++++++++++--------- .../components/shared/map/map.component.ts | 34 ++++----- src/app/components/shared/map/map.module.ts | 3 +- .../components/shared/shared.components.ts | 2 + src/app/components/sites/sites.module.ts | 2 - src/app/services/maps/maps.service.spec.ts | 14 ++++ src/app/services/maps/maps.service.ts | 58 +++++++++------ src/app/test/helpers/googleMaps.ts | 7 ++ 12 files changed, 150 insertions(+), 106 deletions(-) create mode 100644 src/app/test/helpers/googleMaps.ts diff --git a/src/app/components/projects/projects.module.ts b/src/app/components/projects/projects.module.ts index a379ea475..8893e43d4 100644 --- a/src/app/components/projects/projects.module.ts +++ b/src/app/components/projects/projects.module.ts @@ -1,7 +1,6 @@ import { NgModule } from "@angular/core"; import { RouterModule } from "@angular/router"; import { getRouteConfigForPage } from "@helpers/page/pageRouting"; -import { MapModule } from "@shared/map/map.module"; import { SharedModule } from "@shared/shared.module"; import { SiteCardComponent } from "./components/site-card/site-card.component"; import { SiteMapComponent } from "./components/site-map/site-map.component"; @@ -30,7 +29,7 @@ const routes = projectsRoute.compileRoutes(getRouteConfigForPage); @NgModule({ declarations: components, - imports: [MapModule, SharedModule, RouterModule.forChild(routes)], + imports: [SharedModule, RouterModule.forChild(routes)], exports: [RouterModule, ...components], }) export class ProjectsModule {} diff --git a/src/app/components/shared/formly/location-input.component.spec.ts b/src/app/components/shared/formly/location-input.component.spec.ts index 33229a348..631b22c2f 100644 --- a/src/app/components/shared/formly/location-input.component.spec.ts +++ b/src/app/components/shared/formly/location-input.component.spec.ts @@ -6,7 +6,6 @@ import { ReactiveFormsModule, } from "@angular/forms"; import { GoogleMapsModule } from "@angular/google-maps"; -import { embedGoogleMaps } from "@helpers/embedScript/embedGoogleMaps"; import { createHostFactory, SpectatorHost } from "@ngneat/spectator"; import { FormlyBootstrapModule } from "@ngx-formly/bootstrap"; import { FormlyFieldProps, FormlyModule } from "@ngx-formly/core"; @@ -62,8 +61,6 @@ describe("FormlyLocationInput", () => { return spectator; } - beforeAll(async () => await embedGoogleMaps()); - beforeEach(() => setup()); const getLatitudeInput = () => diff --git a/src/app/components/shared/map/map.component.html b/src/app/components/shared/map/map.component.html index 4178cf569..b7eba5002 100644 --- a/src/app/components/shared/map/map.component.html +++ b/src/app/components/shared/map/map.component.html @@ -1,29 +1,29 @@ -@if (hasMarkers && googleMapsLoaded()) { - - - - - {{ infoContent }} - - -} - +@if (mapsLoaded && mapsLoaded && markersLoaded) { + + + {{ infoContent }} + +} @else {
- @if(googleMapsLoaded()) { -

Map loading

- } @else if (maps.mapsState() === mapStates.Failed) { -

Failure loading maps

+ @if (mapsFailed) { +

Failure loading map

+

+ Please ensure your ad-block is not blocking Google Maps +

+ } @else if(!mapsLoaded || !markersLoaded) { + } @else if (!hasMarkers) {

No locations specified

}
+} diff --git a/src/app/components/shared/map/map.component.scss b/src/app/components/shared/map/map.component.scss index 25b24c0c6..0e28e4451 100644 --- a/src/app/components/shared/map/map.component.scss +++ b/src/app/components/shared/map/map.component.scss @@ -1,8 +1,14 @@ .map-placeholder { - border: 1px solid black; display: flex; - height: 100%; - min-height: 400px; + flex-direction: column; justify-content: center; align-items: center; + + border: 1px solid black; + height: 100%; + min-height: 400px; +} + +.error-hint { + text-align: center; } diff --git a/src/app/components/shared/map/map.component.spec.ts b/src/app/components/shared/map/map.component.spec.ts index 40dca23b6..5b35148f5 100644 --- a/src/app/components/shared/map/map.component.spec.ts +++ b/src/app/components/shared/map/map.component.spec.ts @@ -1,17 +1,16 @@ import { GoogleMapsModule } from "@angular/google-maps"; -import { - embedGoogleMaps, - destroyGoogleMaps, -} from "@helpers/embedScript/embedGoogleMaps"; import { Site } from "@models/Site"; import { createComponentFactory, Spectator } from "@ngneat/spectator"; import { generateSite } from "@test/fakes/Site"; import { List } from "immutable"; -import { MapComponent } from "./map.component"; +import { destroyGoogleMaps } from "@test/helpers/googleMaps"; +import { modelData } from "@test/helpers/faker"; +import { MapMarkerOptions, MapComponent } from "./map.component"; // Disabled because google maps bundle interferes with other tests -xdescribe("MapComponent", () => { +describe("MapComponent", () => { let spectator: Spectator; + const createComponent = createComponentFactory({ component: MapComponent, imports: [GoogleMapsModule], @@ -29,61 +28,66 @@ xdescribe("MapComponent", () => { return spectator.queryAll("map-marker"); } - beforeAll(async () => await embedGoogleMaps()); - beforeEach(async () => (spectator = createComponent())); - afterAll(() => destroyGoogleMaps()); + function setup(markers: MapMarkerOptions[] = []): void { + spectator = createComponent({ detectChanges: false }); + spectator.setInput("markers", List(markers)); + } + + afterEach(() => { + destroyGoogleMaps(); + }); it("should create", () => { - spectator.setInput("markers", List([])); - spectator.detectChanges(); + setup(); expect(spectator.component).toBeTruthy(); }); - it("should display placeholder", () => { - spectator.setInput("markers", List([])); - spectator.detectChanges(); + it("should display a placeholder when loading the google maps bundle", () => { + setup(); + }); + + it("should display a placeholder when the markers are loading", () => { + setup(); + }); + + it("should display placeholder when there are no markers", () => { + setup(); + const label = spectator .query("div.map-placeholder") .innerText.trim(); + expect(label).toBe("No locations specified"); }); + it("should display an error if the google maps bundle fails to load", () => {}); + it("should display map", () => { - spectator.setInput( - "markers", - List([new Site(generateSite()).getMapMarker()]) - ); - spectator.detectChanges(); + const markers = [new Site(generateSite()).getMapMarker()]; + setup(markers); expect(getMap()).toBeTruthy(); }); it("should have info window", () => { - spectator.setInput( - "markers", - List([new Site(generateSite()).getMapMarker()]) - ); - spectator.detectChanges(); + const markers = [new Site(generateSite()).getMapMarker()]; + setup(markers); expect(getMap()).toBeTruthy(); }); it("should display single site", () => { - spectator.setInput( - "markers", - List([new Site(generateSite()).getMapMarker()]) - ); - spectator.detectChanges(); + const markers = [new Site(generateSite()).getMapMarker()]; + setup(markers); expect(getInfoWindow()).toBeTruthy(); }); it("should display multiple markers", () => { - const markers = List( - [1, 2, 3].map(() => new Site(generateSite()).getMapMarker()) + const markers = modelData.randomArray(3, 3, () => + new Site(generateSite()).getMapMarker() ); - spectator.setInput("markers", markers); - spectator.detectChanges(); + setup(markers); expect(getMarker().length).toBe(3); }); diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index d59471b71..c61089dbb 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -1,7 +1,6 @@ import { AfterViewChecked, Component, - computed, EventEmitter, Input, OnChanges, @@ -12,10 +11,13 @@ import { } from "@angular/core"; import { GoogleMap, MapInfoWindow, MapMarker } from "@angular/google-maps"; import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; -import { MapsService, MapState } from "@services/maps/maps.service"; +import { MapsService } from "@services/maps/maps.service"; import { List } from "immutable"; import { takeUntil } from "rxjs/operators"; +export type MapMarkerOptions = google.maps.MarkerOptions; +export type MapOptions = google.maps.MapOptions; + /** * Google Maps Wrapper Component * ! Manually test when editing, unit test coverage is poor @@ -31,6 +33,11 @@ export class MapComponent { public constructor(protected maps: MapsService) { super(); + + this.maps + .loadAsync() + .then(() => (this.mapsLoaded = true)) + .catch(() => (this.mapsFailed = true)); } @ViewChild(GoogleMap) public map: GoogleMap; @@ -48,26 +55,24 @@ export class MapComponent // Setting to "hybrid" can increase load times and looks like the map is bugged public mapOptions: MapOptions = { mapTypeId: "satellite" }; public bounds: google.maps.LatLngBounds; - protected googleMapsLoaded = computed( - () => this.maps.mapsState() === MapState.Loaded - ); - protected mapStates = MapState; + public markersLoaded = false; + protected mapsLoaded = false; + protected mapsFailed = false; private updateMap: boolean; public ngOnChanges(): void { - this.hasMarkers = false; - this.filteredMarkers = []; - - // Google global may not be declared - if (this.maps.mapsState() !== MapState.Loaded) { + if (!this.mapsLoaded) { return; } + this.hasMarkers = this.markers?.size > 0; + this.filteredMarkers = []; + // Calculate pin boundaries so that map can be auto-focused properly this.bounds = new google.maps.LatLngBounds(); this.markers?.forEach((marker) => { if (isMarkerValid(marker)) { - this.hasMarkers = true; + this.markersLoaded = true; this.filteredMarkers.push(marker); this.bounds.extend(marker.position); } @@ -76,7 +81,7 @@ export class MapComponent } public ngAfterViewChecked(): void { - if (!this.map || !this.hasMarkers || !this.updateMap) { + if (!this.map || !this.markersLoaded || !this.updateMap) { return; } @@ -132,6 +137,3 @@ export function sanitizeMapMarkers( return List(output); } - -export type MapMarkerOptions = google.maps.MarkerOptions; -export type MapOptions = google.maps.MapOptions; diff --git a/src/app/components/shared/map/map.module.ts b/src/app/components/shared/map/map.module.ts index 6489ba0ee..a03e767e7 100644 --- a/src/app/components/shared/map/map.module.ts +++ b/src/app/components/shared/map/map.module.ts @@ -2,11 +2,12 @@ import { CommonModule } from "@angular/common"; import { NgModule } from "@angular/core"; import { GoogleMapsModule } from "@angular/google-maps"; import { RouterModule } from "@angular/router"; +import { LoadingModule } from "@shared/loading/loading.module"; import { MapComponent } from "./map.component"; @NgModule({ declarations: [MapComponent], - imports: [CommonModule, RouterModule, GoogleMapsModule], + imports: [CommonModule, RouterModule, GoogleMapsModule, LoadingModule], exports: [MapComponent], }) export class MapModule {} diff --git a/src/app/components/shared/shared.components.ts b/src/app/components/shared/shared.components.ts index cfa698fa9..2de294143 100644 --- a/src/app/components/shared/shared.components.ts +++ b/src/app/components/shared/shared.components.ts @@ -58,6 +58,7 @@ import { ZonedDateTimeComponent } from "./datetime-formats/datetime/zoned-dateti import { DatetimeComponent } from "./datetime-formats/datetime/datetime/datetime.component"; import { AudioEventCardModule } from "./audio-event-card/annotation-event-card.module"; import { IfLoggedInComponent } from "./can/can.component"; +import { MapModule } from "./map/map.module"; export const sharedComponents = [ AnnotationDownloadComponent, @@ -115,6 +116,7 @@ export const sharedModules = [ InputModule, ItemsModule, LoadingModule, + MapModule, MenuModule, ModelCardsModule, AudioEventCardModule, diff --git a/src/app/components/sites/sites.module.ts b/src/app/components/sites/sites.module.ts index 6b90a8885..fc642cf2d 100644 --- a/src/app/components/sites/sites.module.ts +++ b/src/app/components/sites/sites.module.ts @@ -2,7 +2,6 @@ import { NgModule } from "@angular/core"; import { RouterModule } from "@angular/router"; import { RegionsModule } from "@components/regions/regions.module"; import { getRouteConfigForPage } from "@helpers/page/pageRouting"; -import { MapModule } from "@shared/map/map.module"; import { SharedModule } from "@shared/shared.module"; import { RecentAnnotationsComponent } from "./components/recent-annotations/recent-annotations.component"; import { SiteComponent } from "./components/site/site.component"; @@ -33,7 +32,6 @@ const pointRoutes = pointsRoute.compileRoutes(getRouteConfigForPage); @NgModule({ declarations: components, imports: [ - MapModule, RegionsModule, SharedModule, RouterModule.forChild([...siteRoutes, ...pointRoutes]), diff --git a/src/app/services/maps/maps.service.spec.ts b/src/app/services/maps/maps.service.spec.ts index 00d3d3875..619327445 100644 --- a/src/app/services/maps/maps.service.spec.ts +++ b/src/app/services/maps/maps.service.spec.ts @@ -15,4 +15,18 @@ describe("MapsService", () => { it("should create", () => { expect(spec.service).toBeInstanceOf(MapsService); }); + + it("should not embed the google maps script if the service is already embedded", () => {}); + + it("should embed the google maps script if the service is not already embedded", () => {}); + + it("should add the auth key to the google maps script", () => {}); + + it("should update the 'load' promise correctly when google maps loads", () => {}); + + it("should update the 'load' promise correctly when google maps fails to load", () => {}); + + it("should return a promise that immediate resolves if the maps are already loaded", () => {}); + + it("should return a promise that immediate rejects if the maps have failed to load", () => {}); }); diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 5314078cd..105a731ba 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -1,18 +1,19 @@ -import { Inject, Injectable, signal } from "@angular/core"; +import { Inject, Injectable } from "@angular/core"; +import { ConfigService } from "@services/config/config.service"; import { defaultDebounceTime, IS_SERVER_PLATFORM } from "src/app/app.helper"; import { environment } from "src/environments/environment"; -export enum MapState { +enum MapState { NotLoaded, Loading, Loaded, Failed, } -interface SharedPromise { +interface SharedPromise { promise: Promise; - resolve: (...args: any) => void; - reject: (...args: any) => void; + resolve: (...args: void[]) => void; + reject: (...args: void[]) => void; } @Injectable({ providedIn: "root" }) @@ -20,7 +21,10 @@ export class MapsService { // By embedding the google maps script in the services constructor, we can // start loading the script as soon as the service is injected, and we don't // have to wait for the underlying component to be created. - public constructor(@Inject(IS_SERVER_PLATFORM) private isServer: boolean) { + public constructor( + @Inject(IS_SERVER_PLATFORM) private isServer: boolean, + private config: ConfigService + ) { // while angular services are singletons, it is still possible to create // multiple instances of the service with hacky code // e.g. new MapsService() @@ -39,7 +43,7 @@ export class MapsService { this.loadPromise = { promise, resolve: resolver, reject: rejector }; - this.embedGoogleMaps(); + this.embedGoogleMaps(this.config.keys.googleMaps); } else { console.warn("Google Maps Service already embedded."); } @@ -47,9 +51,14 @@ export class MapsService { private static embeddedService = false; - public readonly mapsState = signal(MapState.NotLoaded); - private readonly loadPromise: SharedPromise ; - private readonly googleMapsBaseUrl = "https://maps.googleapis.com/maps/api/js"; + public mapsState: MapState = MapState.NotLoaded; + private readonly loadPromise: SharedPromise; + + // using loading=async requests a version of the google maps api that does not + // block the main thread while loading + // this can improve performance and removes a warning from the dev console + private readonly googleMapsBaseUrl = + "https://maps.googleapis.com/maps/api/js?loading=async"; public loadAsync(): Promise { // if we have previously loaded the maps, return immediately with the @@ -60,9 +69,9 @@ export class MapsService { // the maps loaded state has already settled // (promises only resolve/reject once and do not emit their last value // to new awaiters) - if (this.mapsState() === MapState.Loaded) { + if (this.mapsState === MapState.Loaded) { return Promise.resolve(); - } else if (this.mapsState() === MapState.Failed) { + } else if (this.mapsState === MapState.Failed) { return Promise.reject(); } @@ -75,7 +84,7 @@ export class MapsService { * * @param key Google maps API key */ - private async embedGoogleMaps(key?: string): Promise { + private async embedGoogleMaps(key: string): Promise { // TODO: during SSR we might be able to render a static image of the map // using the StaticMapService // https://developers.google.com/maps/documentation/maps-static/overview @@ -83,26 +92,31 @@ export class MapsService { return; } - this.mapsState.set(MapState.Loading); + this.mapsState = MapState.Loading; let googleMapsUrl = this.googleMapsBaseUrl; if (key) { - googleMapsUrl += `?key=${key}&callback=initMap`; + // TODO: migrate to google.maps.AdvancedMarkerElement once we bump the + // Angular version + // https://developers.google.com/maps/documentation/javascript/advanced-markers/migration + googleMapsUrl += `&key=${key}`; } const node: HTMLScriptElement = document.createElement("script"); + node.addEventListener("error", () => { + this.handleGoogleMapsFailed(); + }); + node.id = "google-maps"; node.type = "text/javascript"; node.async = true; node.src = googleMapsUrl; - node.addEventListener("error", () => { - this.handleGoogleMapsFailed(); - }); - document.head.appendChild(node); // Detect when google maps properly embeds + // TODO: This is a bit of a hack and we should find a better way to detect + // when the google namespace is available const instantiationRetries = 10; let count = 0; @@ -110,7 +124,6 @@ export class MapsService { if (typeof google !== "undefined") { this.handleGoogleMapsLoaded(); } else if (count > instantiationRetries) { - console.error("Failed to load google maps."); this.handleGoogleMapsFailed(); } else { count++; @@ -122,12 +135,13 @@ export class MapsService { } private handleGoogleMapsLoaded(): void { - this.mapsState.set(MapState.Loaded); + this.mapsState = MapState.Loaded; this.loadPromise.resolve(); } private handleGoogleMapsFailed(): void { - this.mapsState.set(MapState.Failed); + console.error("Failed to load google maps."); + this.mapsState = MapState.Failed; this.loadPromise.reject(); } } diff --git a/src/app/test/helpers/googleMaps.ts b/src/app/test/helpers/googleMaps.ts new file mode 100644 index 000000000..0a287c365 --- /dev/null +++ b/src/app/test/helpers/googleMaps.ts @@ -0,0 +1,7 @@ +/** + * Remove the google maps script from the document. This should + * only be accessed by unit tests. + */ +export function destroyGoogleMaps(): void { + document.getElementById("google-maps").remove(); +} From fa224d4a09437b10b6cda09617dd277302c29b38 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 11:04:03 +1000 Subject: [PATCH 07/27] Add better comments to code-workspace --- workbench-client.code-workspace | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/workbench-client.code-workspace b/workbench-client.code-workspace index c290ae5f4..df4a706d6 100644 --- a/workbench-client.code-workspace +++ b/workbench-client.code-workspace @@ -61,7 +61,8 @@ "cyrilletuzi.angular-schematics", "dbaeumer.vscode-eslint", "streetsidesoftware.code-spell-checker", - "xabikos.jasminesnippets" + "xabikos.jasminesnippets", + "aaron-bond.better-comments" ] } } From 6c436c7f3d54ef7f722b3321d19c11cb3bcc5468 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 11:12:04 +1000 Subject: [PATCH 08/27] Fix data request page title --- src/app/components/data-request/data-request.component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/components/data-request/data-request.component.ts b/src/app/components/data-request/data-request.component.ts index 0eb2bed74..c9213c7d3 100644 --- a/src/app/components/data-request/data-request.component.ts +++ b/src/app/components/data-request/data-request.component.ts @@ -12,7 +12,7 @@ import schema from "./data-request.schema.json"; @Component({ selector: "baw-data-request", template: ` -

Data Request

+

Custom Data Request

Annotations Download

From e474b02b4505c71e860b1357755b9c3368936995 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 11:28:25 +1000 Subject: [PATCH 09/27] de-duplicate state from maps service --- .../components/shared/map/map.component.html | 6 ++--- .../components/shared/map/map.component.ts | 23 +++++++++++++------ src/app/services/maps/maps.service.ts | 14 +++++------ 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/app/components/shared/map/map.component.html b/src/app/components/shared/map/map.component.html index b7eba5002..aa465b8d5 100644 --- a/src/app/components/shared/map/map.component.html +++ b/src/app/components/shared/map/map.component.html @@ -1,4 +1,4 @@ -@if (mapsLoaded && mapsLoaded && markersLoaded) { +@if (googleMapsLoaded && markersLoaded) { } @else {
- @if (mapsFailed) { + @if (googleMapsFailed) {

Failure loading map

Please ensure your ad-block is not blocking Google Maps

- } @else if(!mapsLoaded || !markersLoaded) { + } @else if(!googleMapsLoaded || !markersLoaded) { } @else if (!hasMarkers) {

No locations specified

diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index c61089dbb..f815ebe5e 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -1,5 +1,6 @@ import { AfterViewChecked, + ChangeDetectorRef, Component, EventEmitter, Input, @@ -11,7 +12,7 @@ import { } from "@angular/core"; import { GoogleMap, MapInfoWindow, MapMarker } from "@angular/google-maps"; import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; -import { MapsService } from "@services/maps/maps.service"; +import { GoogleMapsState, MapsService } from "@services/maps/maps.service"; import { List } from "immutable"; import { takeUntil } from "rxjs/operators"; @@ -31,13 +32,15 @@ export class MapComponent extends withUnsubscribe() implements OnChanges, AfterViewChecked { - public constructor(protected maps: MapsService) { + public constructor( + private maps: MapsService, + private cdr: ChangeDetectorRef + ) { super(); this.maps .loadAsync() - .then(() => (this.mapsLoaded = true)) - .catch(() => (this.mapsFailed = true)); + .finally(() => this.cdr.markForCheck()); } @ViewChild(GoogleMap) public map: GoogleMap; @@ -56,12 +59,18 @@ export class MapComponent public mapOptions: MapOptions = { mapTypeId: "satellite" }; public bounds: google.maps.LatLngBounds; public markersLoaded = false; - protected mapsLoaded = false; - protected mapsFailed = false; private updateMap: boolean; + public get googleMapsLoaded(): boolean { + return this.maps.mapsState === GoogleMapsState.Loaded; + } + + public get googleMapsFailed(): boolean { + return this.maps.mapsState === GoogleMapsState.Failed; + } + public ngOnChanges(): void { - if (!this.mapsLoaded) { + if (!this.googleMapsLoaded) { return; } diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 105a731ba..ef06caf44 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -3,7 +3,7 @@ import { ConfigService } from "@services/config/config.service"; import { defaultDebounceTime, IS_SERVER_PLATFORM } from "src/app/app.helper"; import { environment } from "src/environments/environment"; -enum MapState { +export enum GoogleMapsState { NotLoaded, Loading, Loaded, @@ -51,7 +51,7 @@ export class MapsService { private static embeddedService = false; - public mapsState: MapState = MapState.NotLoaded; + public mapsState: GoogleMapsState = GoogleMapsState.NotLoaded; private readonly loadPromise: SharedPromise; // using loading=async requests a version of the google maps api that does not @@ -69,9 +69,9 @@ export class MapsService { // the maps loaded state has already settled // (promises only resolve/reject once and do not emit their last value // to new awaiters) - if (this.mapsState === MapState.Loaded) { + if (this.mapsState === GoogleMapsState.Loaded) { return Promise.resolve(); - } else if (this.mapsState === MapState.Failed) { + } else if (this.mapsState === GoogleMapsState.Failed) { return Promise.reject(); } @@ -92,7 +92,7 @@ export class MapsService { return; } - this.mapsState = MapState.Loading; + this.mapsState = GoogleMapsState.Loading; let googleMapsUrl = this.googleMapsBaseUrl; if (key) { @@ -135,13 +135,13 @@ export class MapsService { } private handleGoogleMapsLoaded(): void { - this.mapsState = MapState.Loaded; + this.mapsState = GoogleMapsState.Loaded; this.loadPromise.resolve(); } private handleGoogleMapsFailed(): void { console.error("Failed to load google maps."); - this.mapsState = MapState.Failed; + this.mapsState = GoogleMapsState.Failed; this.loadPromise.reject(); } } From dc9084b193963d98d5e231fab8ddbe98617524b5 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 13:12:44 +1000 Subject: [PATCH 10/27] Fix unloaded state --- .../components/shared/map/map.component.html | 4 ++-- src/app/components/shared/map/map.component.ts | 18 +++++++++--------- .../sites/components/site/site.component.ts | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/app/components/shared/map/map.component.html b/src/app/components/shared/map/map.component.html index aa465b8d5..8b034ecc6 100644 --- a/src/app/components/shared/map/map.component.html +++ b/src/app/components/shared/map/map.component.html @@ -1,4 +1,4 @@ -@if (googleMapsLoaded && markersLoaded) { +@if (hasMarkers && googleMapsLoaded) { Please ensure your ad-block is not blocking Google Maps

- } @else if(!googleMapsLoaded || !markersLoaded) { + } @else if(!googleMapsLoaded) { } @else if (!hasMarkers) {

No locations specified

diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index f815ebe5e..49978de42 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -33,14 +33,14 @@ export class MapComponent implements OnChanges, AfterViewChecked { public constructor( - private maps: MapsService, + private mapService: MapsService, private cdr: ChangeDetectorRef ) { super(); - this.maps - .loadAsync() - .finally(() => this.cdr.markForCheck()); + // we use "finally" here so that we will trigger a change detection cycle + // if Google Maps successfully or unsuccessfully embeds + this.mapService.loadAsync().finally(() => this.cdr.markForCheck()); } @ViewChild(GoogleMap) public map: GoogleMap; @@ -58,15 +58,14 @@ export class MapComponent // Setting to "hybrid" can increase load times and looks like the map is bugged public mapOptions: MapOptions = { mapTypeId: "satellite" }; public bounds: google.maps.LatLngBounds; - public markersLoaded = false; - private updateMap: boolean; + private updateMap: boolean = false; public get googleMapsLoaded(): boolean { - return this.maps.mapsState === GoogleMapsState.Loaded; + return this.mapService.mapsState === GoogleMapsState.Loaded; } public get googleMapsFailed(): boolean { - return this.maps.mapsState === GoogleMapsState.Failed; + return this.mapService.mapsState === GoogleMapsState.Failed; } public ngOnChanges(): void { @@ -90,13 +89,14 @@ export class MapComponent } public ngAfterViewChecked(): void { - if (!this.map || !this.markersLoaded || !this.updateMap) { + if (!this.map || !this.updateMap) { return; } this.updateMap = false; this.map.fitBounds(this.bounds); this.map.panToBounds(this.bounds); + // Setup info windows for each marker this.mapMarkers?.forEach((marker, index) => { marker.mapMouseover.pipe(takeUntil(this.unsubscribe)).subscribe({ diff --git a/src/app/components/sites/components/site/site.component.ts b/src/app/components/sites/components/site/site.component.ts index a2960c2eb..14a0f3f03 100644 --- a/src/app/components/sites/components/site/site.component.ts +++ b/src/app/components/sites/components/site/site.component.ts @@ -21,7 +21,7 @@ import { takeUntil } from "rxjs/operators"; @Component({ selector: "baw-site", templateUrl: "./site.component.html", - styleUrls: ["./site.component.scss"], + styleUrl: "./site.component.scss", }) class SiteComponent extends PageComponent implements OnInit { @Input() public project: Project; From cf048174020f6726aba0eec17c85a6856e989c0e Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 13:35:02 +1000 Subject: [PATCH 11/27] Fix change detection issues --- .vscode/settings.json | 15 --------------- .../components/site-map/site-map.component.ts | 10 +++++----- src/app/components/shared/map/map.component.ts | 17 +++++++---------- workbench-client.code-workspace | 9 ++++++++- 4 files changed, 20 insertions(+), 31 deletions(-) delete mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index 43ebd3aad..000000000 --- a/.vscode/settings.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "typescript.tsdk": "node_modules\\typescript\\lib", - "cSpell.words": [ - "Datatable", - "datatable", - "luxon", - "toastr", - "taggings", - "typeahead", - "Ecoacoustics", - "datatable", - "datetime", - "formly" - ] -} diff --git a/src/app/components/projects/components/site-map/site-map.component.ts b/src/app/components/projects/components/site-map/site-map.component.ts index 450ed0d8e..8c32b719e 100644 --- a/src/app/components/projects/components/site-map/site-map.component.ts +++ b/src/app/components/projects/components/site-map/site-map.component.ts @@ -42,11 +42,11 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { if ((this.project || this.region) && !this.sitesSubset?.length) { this.getFilter(filters, this.project, this.region) .pipe( - switchMap((models) => this.getMarkers(models)), + switchMap((models: Site[]) => this.getMarkers(models)), takeUntil(this.unsubscribe) ) .subscribe({ - next: (sites) => this.pushMarkers(sites), + next: (sites: Site[]) => this.pushMarkers(sites), error: () => this.pushMarkers([]), }); } @@ -58,7 +58,7 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { filters: Filters, project: Project, region?: Region - ) { + ): Observable { return this.region ? this.sitesApi.filterByRegion(filters, project, region) : this.sitesApi.filter(filters, project); @@ -67,7 +67,7 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { /** * Retrieve map markers from api */ - private getMarkers(sites: Site[]) { + private getMarkers(sites: Site[]): Observable { const numPages = sites?.[0]?.getMetadata()?.paging?.maxPage || 1; const observables: Observable[] = []; @@ -85,7 +85,7 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { /** * Push new sites to markers list */ - private pushMarkers(sites: Site[]) { + private pushMarkers(sites: Site[]): void { this.markers = this.markers.concat( sanitizeMapMarkers(sites.map((site) => site.getMapMarker())) ); diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index 49978de42..f34df9008 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -32,15 +32,12 @@ export class MapComponent extends withUnsubscribe() implements OnChanges, AfterViewChecked { - public constructor( - private mapService: MapsService, - private cdr: ChangeDetectorRef - ) { + public constructor(private mapService: MapsService) { super(); // we use "finally" here so that we will trigger a change detection cycle // if Google Maps successfully or unsuccessfully embeds - this.mapService.loadAsync().finally(() => this.cdr.markForCheck()); + this.mapService.loadAsync().finally(() => this.ngOnChanges()); } @ViewChild(GoogleMap) public map: GoogleMap; @@ -69,18 +66,18 @@ export class MapComponent } public ngOnChanges(): void { + this.hasMarkers = false; + this.filteredMarkers = []; + if (!this.googleMapsLoaded) { return; } - this.hasMarkers = this.markers?.size > 0; - this.filteredMarkers = []; - // Calculate pin boundaries so that map can be auto-focused properly this.bounds = new google.maps.LatLngBounds(); this.markers?.forEach((marker) => { if (isMarkerValid(marker)) { - this.markersLoaded = true; + this.hasMarkers = true; this.filteredMarkers.push(marker); this.bounds.extend(marker.position); } @@ -89,7 +86,7 @@ export class MapComponent } public ngAfterViewChecked(): void { - if (!this.map || !this.updateMap) { + if (!this.map || !this.hasMarkers || !this.updateMap) { return; } diff --git a/workbench-client.code-workspace b/workbench-client.code-workspace index df4a706d6..ac5452b2f 100644 --- a/workbench-client.code-workspace +++ b/workbench-client.code-workspace @@ -7,6 +7,7 @@ }, "cSpell.words": [ "Ecosounds", + "Ecoacoustics", "Formly", "Mixins", "dropdown", @@ -20,7 +21,13 @@ "toastr", "tzinfo", "unprocessable", - "vocalisations" + "vocalisations", + "deleter", + "typeahead", + "datatable", + "datetime", + "luxon", + "datatable", ], "cSpell.ignorePaths": [ "**/.git/objects/**", From 5d1b4cca6edc6f0554b9f4f5f34ce05b25a105f2 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 14:23:49 +1000 Subject: [PATCH 12/27] Finish map load state tests --- .../shared/map/map.component.spec.ts | 125 ++++++++++++------ .../components/shared/map/map.component.ts | 1 - src/app/services/maps/maps.service.spec.ts | 2 + src/app/test/helpers/googleMaps.ts | 16 ++- 4 files changed, 104 insertions(+), 40 deletions(-) diff --git a/src/app/components/shared/map/map.component.spec.ts b/src/app/components/shared/map/map.component.spec.ts index 5b35148f5..511418abf 100644 --- a/src/app/components/shared/map/map.component.spec.ts +++ b/src/app/components/shared/map/map.component.spec.ts @@ -1,19 +1,31 @@ import { GoogleMapsModule } from "@angular/google-maps"; import { Site } from "@models/Site"; -import { createComponentFactory, Spectator } from "@ngneat/spectator"; +import { + createComponentFactory, + Spectator, + SpyObject, +} from "@ngneat/spectator"; import { generateSite } from "@test/fakes/Site"; import { List } from "immutable"; -import { destroyGoogleMaps } from "@test/helpers/googleMaps"; +import { + destroyGoogleMaps, + mockGoogleNamespace, +} from "@test/helpers/googleMaps"; import { modelData } from "@test/helpers/faker"; +import { MockConfigModule } from "@services/config/configMock.module"; +import { SharedModule } from "@shared/shared.module"; +import { LoadingComponent } from "@shared/loading/loading.component"; +import { MapsService } from "@services/maps/maps.service"; import { MapMarkerOptions, MapComponent } from "./map.component"; // Disabled because google maps bundle interferes with other tests describe("MapComponent", () => { let spectator: Spectator; + let mapsServiceSpy: SpyObject; const createComponent = createComponentFactory({ component: MapComponent, - imports: [GoogleMapsModule], + imports: [GoogleMapsModule, MockConfigModule, SharedModule], }); function getMap() { @@ -28,67 +40,104 @@ describe("MapComponent", () => { return spectator.queryAll("map-marker"); } + function getLoadingComponent(): LoadingComponent { + return spectator.query(LoadingComponent); + } + + function placeholderElement(): HTMLDivElement { + return spectator.query("div.map-placeholder"); + } + + /** Causes all pending 'loadAsync' promises to resolve */ + function triggerLoadSuccess(): void { + mapsServiceSpy["handleGoogleMapsLoaded"](); + spectator.detectChanges(); + } + + /** Causes all pending 'loadAsync' promises to reject */ + function triggerLoadFailure(): void { + mapsServiceSpy["handleGoogleMapsFailed"](); + spectator.detectChanges(); + } + function setup(markers: MapMarkerOptions[] = []): void { + mockGoogleNamespace(); + spectator = createComponent({ detectChanges: false }); + mapsServiceSpy = spectator.inject(MapsService); + + // when ng-spectator's setInput is used it will call detectChanges, meaning + // that this will be the first change detection cycle spectator.setInput("markers", List(markers)); } afterEach(() => { destroyGoogleMaps(); + MapsService["embeddedService"] = false; }); it("should create", () => { setup(); - expect(spectator.component).toBeTruthy(); + expect(spectator.component).toBeInstanceOf(MapComponent); }); - it("should display a placeholder when loading the google maps bundle", () => { - setup(); - }); + describe("loading/error messages", () => { + it("should display a placeholder when loading the google maps bundle", () => { + setup(); + expect(getLoadingComponent()).toExist(); + }); - it("should display a placeholder when the markers are loading", () => { - setup(); - }); + it("should display placeholder when there are no markers", () => { + const expectedText = "No locations specified"; - it("should display placeholder when there are no markers", () => { - setup(); + setup(); + triggerLoadSuccess(); - const label = spectator - .query("div.map-placeholder") - .innerText.trim(); + expect(placeholderElement()).toHaveExactTrimmedText(expectedText); + }); - expect(label).toBe("No locations specified"); + it("should display an error if the google maps bundle fails to load", () => { + const expectedText = + "Failure loading map Please ensure your ad-block is not blocking Google Maps"; + + setup(); + triggerLoadFailure(); + + expect(placeholderElement()).toHaveExactTrimmedText(expectedText); + }); }); - it("should display an error if the google maps bundle fails to load", () => {}); + xdescribe("markers", () => { + it("should display map", () => { + const markers = [new Site(generateSite()).getMapMarker()]; - it("should display map", () => { - const markers = [new Site(generateSite()).getMapMarker()]; - setup(markers); + setup(markers); + triggerLoadSuccess(); - expect(getMap()).toBeTruthy(); - }); + expect(getMap()).toBeTruthy(); + }); - it("should have info window", () => { - const markers = [new Site(generateSite()).getMapMarker()]; - setup(markers); + it("should have info window", () => { + const markers = [new Site(generateSite()).getMapMarker()]; + setup(markers); - expect(getMap()).toBeTruthy(); - }); + expect(getMap()).toBeTruthy(); + }); - it("should display single site", () => { - const markers = [new Site(generateSite()).getMapMarker()]; - setup(markers); + it("should display single site", () => { + const markers = [new Site(generateSite()).getMapMarker()]; + setup(markers); - expect(getInfoWindow()).toBeTruthy(); - }); + expect(getInfoWindow()).toBeTruthy(); + }); - it("should display multiple markers", () => { - const markers = modelData.randomArray(3, 3, () => - new Site(generateSite()).getMapMarker() - ); - setup(markers); + it("should display multiple markers", () => { + const markers = modelData.randomArray(3, 3, () => + new Site(generateSite()).getMapMarker() + ); + setup(markers); - expect(getMarker().length).toBe(3); + expect(getMarker().length).toBe(3); + }); }); }); diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index f34df9008..a209e92f8 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -1,6 +1,5 @@ import { AfterViewChecked, - ChangeDetectorRef, Component, EventEmitter, Input, diff --git a/src/app/services/maps/maps.service.spec.ts b/src/app/services/maps/maps.service.spec.ts index 619327445..ce960c76a 100644 --- a/src/app/services/maps/maps.service.spec.ts +++ b/src/app/services/maps/maps.service.spec.ts @@ -1,4 +1,5 @@ import { createServiceFactory, SpectatorService } from "@ngneat/spectator"; +import { MockConfigModule } from "@services/config/configMock.module"; import { MapsService } from "./maps.service"; describe("MapsService", () => { @@ -6,6 +7,7 @@ describe("MapsService", () => { const createService = createServiceFactory({ service: MapsService, + imports: [MockConfigModule], }); beforeEach(() => { diff --git a/src/app/test/helpers/googleMaps.ts b/src/app/test/helpers/googleMaps.ts index 0a287c365..dcc5129c8 100644 --- a/src/app/test/helpers/googleMaps.ts +++ b/src/app/test/helpers/googleMaps.ts @@ -3,5 +3,19 @@ * only be accessed by unit tests. */ export function destroyGoogleMaps(): void { - document.getElementById("google-maps").remove(); + document.getElementById("google-maps")?.remove(); +} + +export function mockGoogleNamespace(): void { + window.google = { + maps: { + LatLngBounds: jasmine.createSpy("LatLngBounds"), + Marker: jasmine.createSpy("Marker"), + Map: jasmine.createSpy("Map"), + InfoWindow: jasmine.createSpy("InfoWindow"), + event: { + addListener: jasmine.createSpy("addListener"), + }, + }, + } as any; } From ad2757f2097d1e2743a49b9b91833feb50f39645 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 15:30:19 +1000 Subject: [PATCH 13/27] Complete map service tests --- .../shared/map/map.component.spec.ts | 2 +- src/app/services/config/configMock.service.ts | 2 +- src/app/services/maps/maps.service.spec.ts | 76 +++++++++++++++++-- src/app/services/maps/maps.service.ts | 40 ++++++---- 4 files changed, 95 insertions(+), 25 deletions(-) diff --git a/src/app/components/shared/map/map.component.spec.ts b/src/app/components/shared/map/map.component.spec.ts index 511418abf..fa4be356d 100644 --- a/src/app/components/shared/map/map.component.spec.ts +++ b/src/app/components/shared/map/map.component.spec.ts @@ -44,7 +44,7 @@ describe("MapComponent", () => { return spectator.query(LoadingComponent); } - function placeholderElement(): HTMLDivElement { + function placeholderElement() { return spectator.query("div.map-placeholder"); } diff --git a/src/app/services/config/configMock.service.ts b/src/app/services/config/configMock.service.ts index 6ae221b13..e963807ee 100644 --- a/src/app/services/config/configMock.service.ts +++ b/src/app/services/config/configMock.service.ts @@ -42,7 +42,7 @@ export const testApiConfig = new Configuration({ oldClientBase: `${assetRoot}/old-client/index.html`, }, keys: { - googleMaps: "", + googleMaps: "mock-api-key", googleAnalytics: { domain: "", trackingId: "", diff --git a/src/app/services/maps/maps.service.spec.ts b/src/app/services/maps/maps.service.spec.ts index ce960c76a..65badcb82 100644 --- a/src/app/services/maps/maps.service.spec.ts +++ b/src/app/services/maps/maps.service.spec.ts @@ -1,5 +1,7 @@ import { createServiceFactory, SpectatorService } from "@ngneat/spectator"; import { MockConfigModule } from "@services/config/configMock.module"; +import { testApiConfig } from "@services/config/configMock.service"; +import { destroyGoogleMaps } from "@test/helpers/googleMaps"; import { MapsService } from "./maps.service"; describe("MapsService", () => { @@ -10,25 +12,85 @@ describe("MapsService", () => { imports: [MockConfigModule], }); + function scriptUrl(): string { + return spec.service["googleMapsBundleUrl"](); + } + + function triggerLoadSuccess(): void { + spec.service["handleGoogleMapsLoaded"](); + } + + function triggerLoadFailure(): void { + spec.service["handleGoogleMapsFailed"](); + } + beforeEach(() => { spec = createService(); }); + afterEach(() => { + destroyGoogleMaps(); + MapsService["embeddedService"] = false; + }); + it("should create", () => { expect(spec.service).toBeInstanceOf(MapsService); }); - it("should not embed the google maps script if the service is already embedded", () => {}); + it("should add the auth key to the google maps script", () => { + const expectedApiKey = testApiConfig.keys.googleMaps; + const expectedUrl = `https://maps.googleapis.com/maps/api/js?loading=async&key=${expectedApiKey}`; + const realizedUrl = scriptUrl(); - it("should embed the google maps script if the service is not already embedded", () => {}); + expect(realizedUrl).toEqual(expectedUrl); + }); - it("should add the auth key to the google maps script", () => {}); + it("should resolve all promises when google maps is successfully embedded", (done) => { + spec.service + .loadAsync() + .catch(() => fail("Promise should have resolved")) + .finally(() => done()); - it("should update the 'load' promise correctly when google maps loads", () => {}); + triggerLoadSuccess(); + }); - it("should update the 'load' promise correctly when google maps fails to load", () => {}); + it("should reject all promises when google maps fails to embed", (done) => { + spec.service + .loadAsync() + .then(() => fail("Promise should have rejected")) + .finally(() => done()); - it("should return a promise that immediate resolves if the maps are already loaded", () => {}); + triggerLoadFailure(); + }); + + it("should immediately resolve a 'loadAsync' after google maps has been embedded", (done) => { + spec.service + .loadAsync() + .then(async () => { + // because this await does not have a catch clause, causing this test to + // fail if the promise is rejected + await spec.service.loadAsync(); + done(); + }) + .catch(() => { + fail("Promise should have resolved"); + done(); + }); + + triggerLoadSuccess(); + }); + + it("should immediately reject a 'loadAsync' after google maps failed to embed", (done) => { + spec.service + .loadAsync() + .then(() => fail("Promise should have rejected")) + .catch(() => { + spec.service + .loadAsync() + .then(() => fail("Promise should have rejected")) + .catch(() => done()); + }); - it("should return a promise that immediate rejects if the maps have failed to load", () => {}); + triggerLoadFailure(); + }); }); diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index ef06caf44..83c08c424 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -23,7 +23,7 @@ export class MapsService { // have to wait for the underlying component to be created. public constructor( @Inject(IS_SERVER_PLATFORM) private isServer: boolean, - private config: ConfigService + private config: ConfigService, ) { // while angular services are singletons, it is still possible to create // multiple instances of the service with hacky code @@ -43,7 +43,7 @@ export class MapsService { this.loadPromise = { promise, resolve: resolver, reject: rejector }; - this.embedGoogleMaps(this.config.keys.googleMaps); + this.embedGoogleMaps(); } else { console.warn("Google Maps Service already embedded."); } @@ -54,12 +54,6 @@ export class MapsService { public mapsState: GoogleMapsState = GoogleMapsState.NotLoaded; private readonly loadPromise: SharedPromise; - // using loading=async requests a version of the google maps api that does not - // block the main thread while loading - // this can improve performance and removes a warning from the dev console - private readonly googleMapsBaseUrl = - "https://maps.googleapis.com/maps/api/js?loading=async"; - public loadAsync(): Promise { // if we have previously loaded the maps, return immediately with the // correct resolved/rejected state @@ -84,7 +78,7 @@ export class MapsService { * * @param key Google maps API key */ - private async embedGoogleMaps(key: string): Promise { + private async embedGoogleMaps(): Promise { // TODO: during SSR we might be able to render a static image of the map // using the StaticMapService // https://developers.google.com/maps/documentation/maps-static/overview @@ -94,13 +88,7 @@ export class MapsService { this.mapsState = GoogleMapsState.Loading; - let googleMapsUrl = this.googleMapsBaseUrl; - if (key) { - // TODO: migrate to google.maps.AdvancedMarkerElement once we bump the - // Angular version - // https://developers.google.com/maps/documentation/javascript/advanced-markers/migration - googleMapsUrl += `&key=${key}`; - } + const googleMapsUrl = this.googleMapsBundleUrl(); const node: HTMLScriptElement = document.createElement("script"); node.addEventListener("error", () => { @@ -134,6 +122,26 @@ export class MapsService { mapLoaded(); } + private googleMapsBundleUrl(): string { + // using loading=async requests a version of the google maps api that does not + // block the main thread while loading + // this can improve performance and removes a warning from the dev console + const googleMapsBaseUrl = + "https://maps.googleapis.com/maps/api/js?loading=async"; + + const mapsKey = this.config.keys.googleMaps; + + let googleMapsUrl = googleMapsBaseUrl; + if (mapsKey) { + // TODO: migrate to google.maps.AdvancedMarkerElement once we bump the + // Angular version + // https://developers.google.com/maps/documentation/javascript/advanced-markers/migration + googleMapsUrl += `&key=${mapsKey}`; + } + + return googleMapsUrl; + } + private handleGoogleMapsLoaded(): void { this.mapsState = GoogleMapsState.Loaded; this.loadPromise.resolve(); From bc127ea9c841c72e68987062372ab6e3a3534f61 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 15:58:00 +1000 Subject: [PATCH 14/27] Remove explicit call to ngOnUpdate --- .../components/shared/map/map.component.ts | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index a209e92f8..7ddd853c7 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -36,7 +36,7 @@ export class MapComponent // we use "finally" here so that we will trigger a change detection cycle // if Google Maps successfully or unsuccessfully embeds - this.mapService.loadAsync().finally(() => this.ngOnChanges()); + this.mapService.loadAsync().finally(() => this.updateMapMarkers()); } @ViewChild(GoogleMap) public map: GoogleMap; @@ -72,16 +72,7 @@ export class MapComponent return; } - // Calculate pin boundaries so that map can be auto-focused properly - this.bounds = new google.maps.LatLngBounds(); - this.markers?.forEach((marker) => { - if (isMarkerValid(marker)) { - this.hasMarkers = true; - this.filteredMarkers.push(marker); - this.bounds.extend(marker.position); - } - }); - this.updateMap = true; + this.updateMapMarkers(); } public ngAfterViewChecked(): void { @@ -106,6 +97,19 @@ export class MapComponent }); }); } + + private updateMapMarkers(): void { + // Calculate pin boundaries so that map can be auto-focused properly + this.bounds = new google.maps.LatLngBounds(); + this.markers?.forEach((marker) => { + if (isMarkerValid(marker)) { + this.hasMarkers = true; + this.filteredMarkers.push(marker); + this.bounds.extend(marker.position); + } + }); + this.updateMap = true; + } } /** @@ -124,7 +128,7 @@ function isMarkerValid(marker: MapMarkerOptions): boolean { * Handles sanitization of map markers so change detection will run properly */ export function sanitizeMapMarkers( - markers: MapMarkerOptions | MapMarkerOptions[] + markers: MapMarkerOptions | MapMarkerOptions[], ): List { const output: MapMarkerOptions[] = []; From 287fa33c21339879d48e13a7f181ce81c54463cf Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 16:19:17 +1000 Subject: [PATCH 15/27] Revert remove duplicate title on data request page --- src/app/components/data-request/data-request.component.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/app/components/data-request/data-request.component.ts b/src/app/components/data-request/data-request.component.ts index c9213c7d3..837f96d7f 100644 --- a/src/app/components/data-request/data-request.component.ts +++ b/src/app/components/data-request/data-request.component.ts @@ -12,7 +12,7 @@ import schema from "./data-request.schema.json"; @Component({ selector: "baw-data-request", template: ` -

Custom Data Request

+

Data Request

Annotations Download

@@ -26,6 +26,7 @@ import schema from "./data-request.schema.json"; implements OnInit { private api: DataRequestService, notifications: ToastrService, route: ActivatedRoute, - router: Router + router: Router, ) { super(notifications, route, router, { successMsg: () => From ecfb0bf3bb5cdf9da771024272a6d44e5410f3ce Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Mon, 9 Dec 2024 16:47:05 +1000 Subject: [PATCH 16/27] Move map types to maps service --- .../components/site-map/site-map.component.ts | 14 ++++++-------- .../shared/formly/location-input.component.ts | 8 +++----- .../components/shared/map/map.component.spec.ts | 8 ++++---- src/app/components/shared/map/map.component.ts | 10 ++++++---- src/app/components/shared/map/mapMock.component.ts | 3 ++- .../sites/components/site/site.component.ts | 10 ++++------ src/app/models/Site.ts | 2 +- src/app/services/maps/maps.service.ts | 3 +++ 8 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/app/components/projects/components/site-map/site-map.component.ts b/src/app/components/projects/components/site-map/site-map.component.ts index 8c32b719e..aca9037be 100644 --- a/src/app/components/projects/components/site-map/site-map.component.ts +++ b/src/app/components/projects/components/site-map/site-map.component.ts @@ -5,10 +5,8 @@ import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; import { Project } from "@models/Project"; import { Region } from "@models/Region"; import { ISite, Site } from "@models/Site"; -import { - MapMarkerOptions, - sanitizeMapMarkers, -} from "@shared/map/map.component"; +import { MapMarkerOptions } from "@services/maps/maps.service"; +import { sanitizeMapMarkers } from "@shared/map/map.component"; import { List } from "immutable"; import { merge, Observable } from "rxjs"; import { switchMap, takeUntil } from "rxjs/operators"; @@ -43,7 +41,7 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { this.getFilter(filters, this.project, this.region) .pipe( switchMap((models: Site[]) => this.getMarkers(models)), - takeUntil(this.unsubscribe) + takeUntil(this.unsubscribe), ) .subscribe({ next: (sites: Site[]) => this.pushMarkers(sites), @@ -57,7 +55,7 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { private getFilter( filters: Filters, project: Project, - region?: Region + region?: Region, ): Observable { return this.region ? this.sitesApi.filterByRegion(filters, project, region) @@ -74,7 +72,7 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { // Can skip first page because initial filter produces the results for (let page = 2; page <= numPages; page++) { observables.push( - this.getFilter({ paging: { page } }, this.project, this.region) + this.getFilter({ paging: { page } }, this.project, this.region), ); } @@ -87,7 +85,7 @@ export class SiteMapComponent extends withUnsubscribe() implements OnChanges { */ private pushMarkers(sites: Site[]): void { this.markers = this.markers.concat( - sanitizeMapMarkers(sites.map((site) => site.getMapMarker())) + sanitizeMapMarkers(sites.map((site) => site.getMapMarker())), ); } } diff --git a/src/app/components/shared/formly/location-input.component.ts b/src/app/components/shared/formly/location-input.component.ts index 9a238a9bf..3f3f8caf5 100644 --- a/src/app/components/shared/formly/location-input.component.ts +++ b/src/app/components/shared/formly/location-input.component.ts @@ -1,12 +1,10 @@ import { Component, OnInit } from "@angular/core"; import { isInstantiated } from "@helpers/isInstantiated/isInstantiated"; import { FieldType } from "@ngx-formly/core"; -import { - MapMarkerOptions, - sanitizeMapMarkers, -} from "@shared/map/map.component"; +import { sanitizeMapMarkers } from "@shared/map/map.component"; import { List } from "immutable"; import { asFormControl } from "./helper"; +import { MapMarkerOptions } from "@services/maps/maps.service"; /** * Location Input @@ -131,7 +129,7 @@ export class LocationInputComponent extends FieldType implements OnInit { position: { lat: latitude, lng: longitude }, label: `Position (${latitude},${longitude})`, } - : null + : null, ); } diff --git a/src/app/components/shared/map/map.component.spec.ts b/src/app/components/shared/map/map.component.spec.ts index fa4be356d..fafd9e948 100644 --- a/src/app/components/shared/map/map.component.spec.ts +++ b/src/app/components/shared/map/map.component.spec.ts @@ -15,8 +15,8 @@ import { modelData } from "@test/helpers/faker"; import { MockConfigModule } from "@services/config/configMock.module"; import { SharedModule } from "@shared/shared.module"; import { LoadingComponent } from "@shared/loading/loading.component"; -import { MapsService } from "@services/maps/maps.service"; -import { MapMarkerOptions, MapComponent } from "./map.component"; +import { MapMarkerOptions, MapsService } from "@services/maps/maps.service"; +import { MapComponent } from "./map.component"; // Disabled because google maps bundle interferes with other tests describe("MapComponent", () => { @@ -107,7 +107,7 @@ describe("MapComponent", () => { }); }); - xdescribe("markers", () => { + describe("markers", () => { it("should display map", () => { const markers = [new Site(generateSite()).getMapMarker()]; @@ -133,7 +133,7 @@ describe("MapComponent", () => { it("should display multiple markers", () => { const markers = modelData.randomArray(3, 3, () => - new Site(generateSite()).getMapMarker() + new Site(generateSite()).getMapMarker(), ); setup(markers); diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index 7ddd853c7..069af755f 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -11,13 +11,15 @@ import { } from "@angular/core"; import { GoogleMap, MapInfoWindow, MapMarker } from "@angular/google-maps"; import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; -import { GoogleMapsState, MapsService } from "@services/maps/maps.service"; +import { + GoogleMapsState, + MapMarkerOptions, + MapOptions, + MapsService, +} from "@services/maps/maps.service"; import { List } from "immutable"; import { takeUntil } from "rxjs/operators"; -export type MapMarkerOptions = google.maps.MarkerOptions; -export type MapOptions = google.maps.MapOptions; - /** * Google Maps Wrapper Component * ! Manually test when editing, unit test coverage is poor diff --git a/src/app/components/shared/map/mapMock.component.ts b/src/app/components/shared/map/mapMock.component.ts index 2cfea83f0..780341012 100644 --- a/src/app/components/shared/map/mapMock.component.ts +++ b/src/app/components/shared/map/mapMock.component.ts @@ -1,6 +1,7 @@ import { Component, Input, OnInit } from "@angular/core"; import { List } from "immutable"; -import { MapMarkerOptions, sanitizeMapMarkers } from "./map.component"; +import { MapMarkerOptions } from "@services/maps/maps.service"; +import { sanitizeMapMarkers } from "./map.component"; @Component({ selector: "baw-map", diff --git a/src/app/components/sites/components/site/site.component.ts b/src/app/components/sites/components/site/site.component.ts index 14a0f3f03..ce6fb6442 100644 --- a/src/app/components/sites/components/site/site.component.ts +++ b/src/app/components/sites/components/site/site.component.ts @@ -7,10 +7,8 @@ import { AudioRecording } from "@models/AudioRecording"; import { Project } from "@models/Project"; import { Region } from "@models/Region"; import { Site } from "@models/Site"; -import { - MapMarkerOptions, - sanitizeMapMarkers, -} from "@shared/map/map.component"; +import { MapMarkerOptions } from "@services/maps/maps.service"; +import { sanitizeMapMarkers } from "@shared/map/map.component"; import { List } from "immutable"; import { Observable } from "rxjs"; import { takeUntil } from "rxjs/operators"; @@ -73,11 +71,11 @@ class SiteComponent extends PageComponent implements OnInit { private filterByDates( direction: Direction, - filters: Filters = {} + filters: Filters = {}, ): Observable { return this.audioRecordingsApi.filterBySite( { sorting: { orderBy: "recordedDate", direction }, ...filters }, - this.site + this.site, ); } } diff --git a/src/app/models/Site.ts b/src/app/models/Site.ts index 6afc360d1..d097a9da8 100644 --- a/src/app/models/Site.ts +++ b/src/app/models/Site.ts @@ -7,7 +7,7 @@ import { siteRoute } from "@components/sites/sites.routes"; import { visualizeMenuItem } from "@components/visualize/visualize.menus"; import { isInstantiated } from "@helpers/isInstantiated/isInstantiated"; import { assetRoot } from "@services/config/config.service"; -import { MapMarkerOptions } from "@shared/map/map.component"; +import { MapMarkerOptions } from "@services/maps/maps.service"; import { PermissionLevel, DateTimeTimezone, diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 83c08c424..52524120b 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -10,6 +10,9 @@ export enum GoogleMapsState { Failed, } +export type MapMarkerOptions = google.maps.MarkerOptions; +export type MapOptions = google.maps.MapOptions; + interface SharedPromise { promise: Promise; resolve: (...args: void[]) => void; From c13ee722f924ae5cc6ac8c57652641d5ff704922 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Tue, 10 Dec 2024 10:59:27 +1000 Subject: [PATCH 17/27] Fix map tests and make maps.service more resiliant --- .../shared/map/map.component.spec.ts | 49 +++++++++++++------ src/app/services/maps/maps.service.ts | 34 ++++++++----- src/app/test/helpers/googleMaps.ts | 6 ++- 3 files changed, 61 insertions(+), 28 deletions(-) diff --git a/src/app/components/shared/map/map.component.spec.ts b/src/app/components/shared/map/map.component.spec.ts index fafd9e948..9f0385ee4 100644 --- a/src/app/components/shared/map/map.component.spec.ts +++ b/src/app/components/shared/map/map.component.spec.ts @@ -1,4 +1,4 @@ -import { GoogleMapsModule } from "@angular/google-maps"; +import { GoogleMap, GoogleMapsModule, MapInfoWindow, MapMarker } from "@angular/google-maps"; import { Site } from "@models/Site"; import { createComponentFactory, @@ -16,6 +16,8 @@ import { MockConfigModule } from "@services/config/configMock.module"; import { SharedModule } from "@shared/shared.module"; import { LoadingComponent } from "@shared/loading/loading.component"; import { MapMarkerOptions, MapsService } from "@services/maps/maps.service"; +import { MockBawApiModule } from "@baw-api/baw-apiMock.module"; +import { MockModule } from "ng-mocks"; import { MapComponent } from "./map.component"; // Disabled because google maps bundle interferes with other tests @@ -25,19 +27,24 @@ describe("MapComponent", () => { const createComponent = createComponentFactory({ component: MapComponent, - imports: [GoogleMapsModule, MockConfigModule, SharedModule], + imports: [ + MockBawApiModule, + MockConfigModule, + SharedModule, + MockModule(GoogleMapsModule), + ], }); function getMap() { - return spectator.query("google-map"); + return spectator.query(GoogleMap); } function getInfoWindow() { - return spectator.query("map-info-window"); + return spectator.query(MapInfoWindow); } - function getMarker() { - return spectator.queryAll("map-marker"); + function getMarkers() { + return spectator.queryAll(MapMarker); } function getLoadingComponent(): LoadingComponent { @@ -69,11 +76,15 @@ describe("MapComponent", () => { // when ng-spectator's setInput is used it will call detectChanges, meaning // that this will be the first change detection cycle spectator.setInput("markers", List(markers)); + + if (markers.length) { + spectator.component.hasMarkers = true; + spectator.detectChanges(); + } } afterEach(() => { destroyGoogleMaps(); - MapsService["embeddedService"] = false; }); it("should create", () => { @@ -114,30 +125,40 @@ describe("MapComponent", () => { setup(markers); triggerLoadSuccess(); - expect(getMap()).toBeTruthy(); + expect(getMap()).toExist(); }); it("should have info window", () => { const markers = [new Site(generateSite()).getMapMarker()]; + setup(markers); + triggerLoadSuccess(); - expect(getMap()).toBeTruthy(); + expect(getInfoWindow()).toExist(); }); - it("should display single site", () => { + // These tests are currently disabled because we don't want to/ actually + // load Google Maps in the tests, and mocking the Google Maps component is + // a maintenance burden + // TODO: we should find a way to mock these tests + xit("should display single site", () => { const markers = [new Site(generateSite()).getMapMarker()]; + setup(markers); + triggerLoadSuccess(); - expect(getInfoWindow()).toBeTruthy(); + expect(getMarkers().length).toBeTruthy(); }); - it("should display multiple markers", () => { + xit("should display multiple markers", () => { const markers = modelData.randomArray(3, 3, () => - new Site(generateSite()).getMapMarker(), + new Site(generateSite()).getMapMarker() ); + setup(markers); + triggerLoadSuccess(); - expect(getMarker().length).toBe(3); + expect(getMarkers()).toHaveLength(3); }); }); }); diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 52524120b..7898171b6 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -26,7 +26,7 @@ export class MapsService { // have to wait for the underlying component to be created. public constructor( @Inject(IS_SERVER_PLATFORM) private isServer: boolean, - private config: ConfigService, + private config: ConfigService ) { // while angular services are singletons, it is still possible to create // multiple instances of the service with hacky code @@ -36,16 +36,7 @@ export class MapsService { // in normal use, but is defensive programming against misuse. if (!MapsService.embeddedService) { MapsService.embeddedService = true; - - let resolver: (value: unknown) => void; - let rejector: (reason?: unknown) => void; - const promise = new Promise((res, rej) => { - resolver = res; - rejector = rej; - }); - - this.loadPromise = { promise, resolve: resolver, reject: rejector }; - + this.createLoadPromise(); this.embedGoogleMaps(); } else { console.warn("Google Maps Service already embedded."); @@ -55,7 +46,7 @@ export class MapsService { private static embeddedService = false; public mapsState: GoogleMapsState = GoogleMapsState.NotLoaded; - private readonly loadPromise: SharedPromise; + private loadPromise: SharedPromise; public loadAsync(): Promise { // if we have previously loaded the maps, return immediately with the @@ -72,7 +63,24 @@ export class MapsService { return Promise.reject(); } - return this.loadPromise.promise; + if (this.loadPromise) { + return this.loadPromise.promise; + } + + return this.createLoadPromise().promise; + } + + private createLoadPromise(): SharedPromise { + let resolver: (value: unknown) => void; + let rejector: (reason?: unknown) => void; + const promise = new Promise((res, rej) => { + resolver = res; + rejector = rej; + }); + + this.loadPromise = { promise, resolve: resolver, reject: rejector }; + + return this.loadPromise; } /** diff --git a/src/app/test/helpers/googleMaps.ts b/src/app/test/helpers/googleMaps.ts index dcc5129c8..01d8cf90e 100644 --- a/src/app/test/helpers/googleMaps.ts +++ b/src/app/test/helpers/googleMaps.ts @@ -9,7 +9,7 @@ export function destroyGoogleMaps(): void { export function mockGoogleNamespace(): void { window.google = { maps: { - LatLngBounds: jasmine.createSpy("LatLngBounds"), + LatLngBounds: MockLatLngBounds, Marker: jasmine.createSpy("Marker"), Map: jasmine.createSpy("Map"), InfoWindow: jasmine.createSpy("InfoWindow"), @@ -19,3 +19,7 @@ export function mockGoogleNamespace(): void { }, } as any; } + +class MockLatLngBounds { + public extend = jasmine.createSpy("extend"); +} From 3e8108f39d93f692b122d506cb855086e7f4bc0b Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Tue, 10 Dec 2024 11:10:39 +1000 Subject: [PATCH 18/27] Fix linting errors --- src/app/components/shared/formly/location-input.component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/components/shared/formly/location-input.component.ts b/src/app/components/shared/formly/location-input.component.ts index 3f3f8caf5..fb74a14d5 100644 --- a/src/app/components/shared/formly/location-input.component.ts +++ b/src/app/components/shared/formly/location-input.component.ts @@ -3,8 +3,8 @@ import { isInstantiated } from "@helpers/isInstantiated/isInstantiated"; import { FieldType } from "@ngx-formly/core"; import { sanitizeMapMarkers } from "@shared/map/map.component"; import { List } from "immutable"; -import { asFormControl } from "./helper"; import { MapMarkerOptions } from "@services/maps/maps.service"; +import { asFormControl } from "./helper"; /** * Location Input From c954f2153a3322d66b25774597913a6a451a7b64 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Tue, 10 Dec 2024 13:47:33 +1000 Subject: [PATCH 19/27] Refactor maps service promise --- src/app/helpers/timing/sleep.ts | 3 + src/app/services/maps/maps.service.ts | 84 ++++++--------------------- 2 files changed, 20 insertions(+), 67 deletions(-) create mode 100644 src/app/helpers/timing/sleep.ts diff --git a/src/app/helpers/timing/sleep.ts b/src/app/helpers/timing/sleep.ts new file mode 100644 index 000000000..7e78bac21 --- /dev/null +++ b/src/app/helpers/timing/sleep.ts @@ -0,0 +1,3 @@ +export function sleep(milliseconds: number): Promise { + return new Promise((resolve) => setTimeout(resolve, milliseconds)); +} diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 7898171b6..545d61d60 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -1,4 +1,5 @@ import { Inject, Injectable } from "@angular/core"; +import { sleep } from "@helpers/timing/sleep"; import { ConfigService } from "@services/config/config.service"; import { defaultDebounceTime, IS_SERVER_PLATFORM } from "src/app/app.helper"; import { environment } from "src/environments/environment"; @@ -13,12 +14,6 @@ export enum GoogleMapsState { export type MapMarkerOptions = google.maps.MarkerOptions; export type MapOptions = google.maps.MapOptions; -interface SharedPromise { - promise: Promise; - resolve: (...args: void[]) => void; - reject: (...args: void[]) => void; -} - @Injectable({ providedIn: "root" }) export class MapsService { // By embedding the google maps script in the services constructor, we can @@ -34,52 +29,21 @@ export class MapsService { // This is a safeguard to prevent multiple instances of the service // from embedding the google maps script. It should not be triggered // in normal use, but is defensive programming against misuse. - if (!MapsService.embeddedService) { - MapsService.embeddedService = true; - this.createLoadPromise(); - this.embedGoogleMaps(); + if (!MapsService.embedded) { + MapsService.embedded = true; + this.loadPromise = this.embedGoogleMaps(); } else { console.warn("Google Maps Service already embedded."); } } - private static embeddedService = false; + /** A static field to track if the Google Maps script has been embedded */ + private static embedded = false; public mapsState: GoogleMapsState = GoogleMapsState.NotLoaded; - private loadPromise: SharedPromise; - - public loadAsync(): Promise { - // if we have previously loaded the maps, return immediately with the - // correct resolved/rejected state - // - // we obviously can't return "this.loadPromise" directly as it'd never - // resolve because the res() and rej() functions would not be called because - // the maps loaded state has already settled - // (promises only resolve/reject once and do not emit their last value - // to new awaiters) - if (this.mapsState === GoogleMapsState.Loaded) { - return Promise.resolve(); - } else if (this.mapsState === GoogleMapsState.Failed) { - return Promise.reject(); - } - - if (this.loadPromise) { - return this.loadPromise.promise; - } - - return this.createLoadPromise().promise; - } - - private createLoadPromise(): SharedPromise { - let resolver: (value: unknown) => void; - let rejector: (reason?: unknown) => void; - const promise = new Promise((res, rej) => { - resolver = res; - rejector = rej; - }); - - this.loadPromise = { promise, resolve: resolver, reject: rejector }; + private loadPromise: Promise; + public loadAsync(): Promise { return this.loadPromise; } @@ -89,7 +53,7 @@ export class MapsService { * * @param key Google maps API key */ - private async embedGoogleMaps(): Promise { + private async embedGoogleMaps(): Promise { // TODO: during SSR we might be able to render a static image of the map // using the StaticMapService // https://developers.google.com/maps/documentation/maps-static/overview @@ -103,7 +67,7 @@ export class MapsService { const node: HTMLScriptElement = document.createElement("script"); node.addEventListener("error", () => { - this.handleGoogleMapsFailed(); + throw new Error("Failed to load Google Maps script."); }); node.id = "google-maps"; @@ -117,20 +81,17 @@ export class MapsService { // TODO: This is a bit of a hack and we should find a better way to detect // when the google namespace is available const instantiationRetries = 10; - let count = 0; - const mapLoaded = () => { + for (let retry = 0; retry < instantiationRetries; retry++) { + await sleep(defaultDebounceTime); if (typeof google !== "undefined") { - this.handleGoogleMapsLoaded(); - } else if (count > instantiationRetries) { - this.handleGoogleMapsFailed(); - } else { - count++; - setTimeout(() => mapLoaded(), defaultDebounceTime); + this.mapsState = GoogleMapsState.Loaded; + return true; } - }; + } - mapLoaded(); + this.mapsState = GoogleMapsState.Failed; + return false; } private googleMapsBundleUrl(): string { @@ -152,15 +113,4 @@ export class MapsService { return googleMapsUrl; } - - private handleGoogleMapsLoaded(): void { - this.mapsState = GoogleMapsState.Loaded; - this.loadPromise.resolve(); - } - - private handleGoogleMapsFailed(): void { - console.error("Failed to load google maps."); - this.mapsState = GoogleMapsState.Failed; - this.loadPromise.reject(); - } } From b008b89b64be48eaa86a4120a42c493bb1184ef7 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Tue, 10 Dec 2024 14:34:46 +1000 Subject: [PATCH 20/27] Update tests --- .../shared/map/map.component.spec.ts | 6 +- src/app/services/maps/maps.service.spec.ts | 62 +++++++++---------- src/app/services/maps/maps.service.ts | 16 +---- workbench-client.code-workspace | 6 +- 4 files changed, 37 insertions(+), 53 deletions(-) diff --git a/src/app/components/shared/map/map.component.spec.ts b/src/app/components/shared/map/map.component.spec.ts index 9f0385ee4..2c179ce7b 100644 --- a/src/app/components/shared/map/map.component.spec.ts +++ b/src/app/components/shared/map/map.component.spec.ts @@ -15,7 +15,7 @@ import { modelData } from "@test/helpers/faker"; import { MockConfigModule } from "@services/config/configMock.module"; import { SharedModule } from "@shared/shared.module"; import { LoadingComponent } from "@shared/loading/loading.component"; -import { MapMarkerOptions, MapsService } from "@services/maps/maps.service"; +import { GoogleMapsState, MapMarkerOptions, MapsService } from "@services/maps/maps.service"; import { MockBawApiModule } from "@baw-api/baw-apiMock.module"; import { MockModule } from "ng-mocks"; import { MapComponent } from "./map.component"; @@ -57,13 +57,13 @@ describe("MapComponent", () => { /** Causes all pending 'loadAsync' promises to resolve */ function triggerLoadSuccess(): void { - mapsServiceSpy["handleGoogleMapsLoaded"](); + mapsServiceSpy.mapsState = GoogleMapsState.Loaded; spectator.detectChanges(); } /** Causes all pending 'loadAsync' promises to reject */ function triggerLoadFailure(): void { - mapsServiceSpy["handleGoogleMapsFailed"](); + mapsServiceSpy.mapsState = GoogleMapsState.Failed; spectator.detectChanges(); } diff --git a/src/app/services/maps/maps.service.spec.ts b/src/app/services/maps/maps.service.spec.ts index 65badcb82..f38e27d08 100644 --- a/src/app/services/maps/maps.service.spec.ts +++ b/src/app/services/maps/maps.service.spec.ts @@ -2,7 +2,7 @@ import { createServiceFactory, SpectatorService } from "@ngneat/spectator"; import { MockConfigModule } from "@services/config/configMock.module"; import { testApiConfig } from "@services/config/configMock.service"; import { destroyGoogleMaps } from "@test/helpers/googleMaps"; -import { MapsService } from "./maps.service"; +import { GoogleMapsState, MapsService } from "./maps.service"; describe("MapsService", () => { let spec: SpectatorService; @@ -17,11 +17,13 @@ describe("MapsService", () => { } function triggerLoadSuccess(): void { - spec.service["handleGoogleMapsLoaded"](); + spec.service.mapsState = GoogleMapsState.Loaded; + spec.service.loadAsync = async () => true; } function triggerLoadFailure(): void { - spec.service["handleGoogleMapsFailed"](); + spec.service.mapsState = GoogleMapsState.Failed; + spec.service.loadAsync = async () => false; } beforeEach(() => { @@ -30,7 +32,6 @@ describe("MapsService", () => { afterEach(() => { destroyGoogleMaps(); - MapsService["embeddedService"] = false; }); it("should create", () => { @@ -45,52 +46,49 @@ describe("MapsService", () => { expect(realizedUrl).toEqual(expectedUrl); }); - it("should resolve all promises when google maps is successfully embedded", (done) => { - spec.service - .loadAsync() - .catch(() => fail("Promise should have resolved")) - .finally(() => done()); - + it("should resolve all promises with 'true' when google maps is successfully embedded", (done) => { triggerLoadSuccess(); - }); - it("should reject all promises when google maps fails to embed", (done) => { spec.service .loadAsync() - .then(() => fail("Promise should have rejected")) - .finally(() => done()); + .then((success: boolean) => { + expect(success).toBeTrue(); + done(); + }); + }); + it("should result all promises with 'false' when google maps fails to embed", (done) => { triggerLoadFailure(); - }); - it("should immediately resolve a 'loadAsync' after google maps has been embedded", (done) => { spec.service .loadAsync() - .then(async () => { - // because this await does not have a catch clause, causing this test to - // fail if the promise is rejected - await spec.service.loadAsync(); - done(); - }) - .catch(() => { - fail("Promise should have resolved"); + .then((success: boolean) => { + expect(success).toBeFalse(); done(); }); + }); + it("should immediately resolve a 'loadAsync' with 'true' after google maps has been embedded", (done) => { triggerLoadSuccess(); - }); - it("should immediately reject a 'loadAsync' after google maps failed to embed", (done) => { spec.service .loadAsync() - .then(() => fail("Promise should have rejected")) - .catch(() => { - spec.service - .loadAsync() - .then(() => fail("Promise should have rejected")) - .catch(() => done()); + .then(async () => { + const success = await spec.service.loadAsync(); + expect(success).toBeTrue(); + done(); }); + }); + it("should immediately resolve a 'loadAsync' with 'false' after google maps failed to embed", (done) => { triggerLoadFailure(); + + spec.service + .loadAsync() + .then(async () => { + const success = await spec.service.loadAsync(); + expect(success).toBeFalse(); + done(); + }); }); }); diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 545d61d60..6662baae8 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -23,23 +23,9 @@ export class MapsService { @Inject(IS_SERVER_PLATFORM) private isServer: boolean, private config: ConfigService ) { - // while angular services are singletons, it is still possible to create - // multiple instances of the service with hacky code - // e.g. new MapsService() - // This is a safeguard to prevent multiple instances of the service - // from embedding the google maps script. It should not be triggered - // in normal use, but is defensive programming against misuse. - if (!MapsService.embedded) { - MapsService.embedded = true; - this.loadPromise = this.embedGoogleMaps(); - } else { - console.warn("Google Maps Service already embedded."); - } + this.loadPromise = this.embedGoogleMaps(); } - /** A static field to track if the Google Maps script has been embedded */ - private static embedded = false; - public mapsState: GoogleMapsState = GoogleMapsState.NotLoaded; private loadPromise: Promise; diff --git a/workbench-client.code-workspace b/workbench-client.code-workspace index ac5452b2f..dbcb50a1c 100644 --- a/workbench-client.code-workspace +++ b/workbench-client.code-workspace @@ -6,9 +6,9 @@ "source.fixAll": "explicit" }, "cSpell.words": [ - "Ecosounds", - "Ecoacoustics", - "Formly", + "ecosounds", + "ecoacoustics", + "formly", "Mixins", "dropdown", "fontawesome", From 5ecb3916f4f179ed1b4734dcb288c10fc17b5bf9 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Tue, 10 Dec 2024 15:18:48 +1000 Subject: [PATCH 21/27] Fix up tests --- src/app/components/shared/map/map.component.ts | 12 +++++++++--- .../components/sites/pages/new/new.component.spec.ts | 4 +++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index 069af755f..d73aff466 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -36,9 +36,15 @@ export class MapComponent public constructor(private mapService: MapsService) { super(); - // we use "finally" here so that we will trigger a change detection cycle - // if Google Maps successfully or unsuccessfully embeds - this.mapService.loadAsync().finally(() => this.updateMapMarkers()); + this.mapService.loadAsync() + .then((success: boolean) => { + if (success) { + this.updateMapMarkers(); + } else { + console.warn("Failed to load Google Maps"); + } + }) + .catch(() => console.warn("Failed to load Google Maps")); } @ViewChild(GoogleMap) public map: GoogleMap; diff --git a/src/app/components/sites/pages/new/new.component.spec.ts b/src/app/components/sites/pages/new/new.component.spec.ts index 06ce1120b..e60ef2fb2 100644 --- a/src/app/components/sites/pages/new/new.component.spec.ts +++ b/src/app/components/sites/pages/new/new.component.spec.ts @@ -18,6 +18,8 @@ import { assertPageInfo } from "@test/helpers/pageRoute"; import { testFormImports } from "@test/helpers/testbed"; import { ToastrService } from "ngx-toastr"; import { BehaviorSubject, Subject } from "rxjs"; +import { MockComponent } from "ng-mocks"; +import { MapComponent } from "@shared/map/map.component"; import pointSchema from "../../point.base.json"; import siteSchema from "../../site.base.json"; import { SiteNewComponent } from "./new.component"; @@ -27,7 +29,7 @@ describe("SiteNewComponent", () => { const createComponent = createRoutingFactory({ component: SiteNewComponent, imports: [...testFormImports, MockBawApiModule], - declarations: [FormComponent], + declarations: [FormComponent, MockComponent(MapComponent)], mocks: [ToastrService], }); From 1da7b9cb01200613c374cc230459b05dfd54a905 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Tue, 10 Dec 2024 15:51:51 +1000 Subject: [PATCH 22/27] Improve handling of error event --- src/app/services/maps/maps.service.spec.ts | 44 +++++++++------------- src/app/services/maps/maps.service.ts | 18 ++++++++- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/src/app/services/maps/maps.service.spec.ts b/src/app/services/maps/maps.service.spec.ts index f38e27d08..b1411503d 100644 --- a/src/app/services/maps/maps.service.spec.ts +++ b/src/app/services/maps/maps.service.spec.ts @@ -49,46 +49,38 @@ describe("MapsService", () => { it("should resolve all promises with 'true' when google maps is successfully embedded", (done) => { triggerLoadSuccess(); - spec.service - .loadAsync() - .then((success: boolean) => { - expect(success).toBeTrue(); - done(); - }); + spec.service.loadAsync().then((success: boolean) => { + expect(success).toBeTrue(); + done(); + }); }); it("should result all promises with 'false' when google maps fails to embed", (done) => { triggerLoadFailure(); - spec.service - .loadAsync() - .then((success: boolean) => { - expect(success).toBeFalse(); - done(); - }); + spec.service.loadAsync().then((success: boolean) => { + expect(success).toBeFalse(); + done(); + }); }); it("should immediately resolve a 'loadAsync' with 'true' after google maps has been embedded", (done) => { triggerLoadSuccess(); - spec.service - .loadAsync() - .then(async () => { - const success = await spec.service.loadAsync(); - expect(success).toBeTrue(); - done(); - }); + spec.service.loadAsync().then(async () => { + const success = await spec.service.loadAsync(); + expect(success).toBeTrue(); + done(); + }); }); it("should immediately resolve a 'loadAsync' with 'false' after google maps failed to embed", (done) => { triggerLoadFailure(); - spec.service - .loadAsync() - .then(async () => { - const success = await spec.service.loadAsync(); - expect(success).toBeFalse(); - done(); - }); + spec.service.loadAsync().then(async () => { + const success = await spec.service.loadAsync(); + expect(success).toBeFalse(); + done(); + }); }); }); diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 6662baae8..63c960a47 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -53,7 +53,7 @@ export class MapsService { const node: HTMLScriptElement = document.createElement("script"); node.addEventListener("error", () => { - throw new Error("Failed to load Google Maps script."); + this.mapsState = GoogleMapsState.Failed; }); node.id = "google-maps"; @@ -69,13 +69,27 @@ export class MapsService { const instantiationRetries = 10; for (let retry = 0; retry < instantiationRetries; retry++) { - await sleep(defaultDebounceTime); + // because the "failed" state can be asynchronously updated by the script + // elements error event listener, we check if the state has changed to + // a "failed" state so that we can return false from this functions + // promise + if ((this.mapsState as GoogleMapsState) === GoogleMapsState.Failed) { + return false; + } + + // because the "google" global namespace is loaded by the google maps + // script, we can check if it is defined to determine if the script has + // been successfully loaded if (typeof google !== "undefined") { this.mapsState = GoogleMapsState.Loaded; return true; } + + await sleep(defaultDebounceTime); } + // if we reach this point, the "google" namespace was never defined in the + // global scope, so we assume the script failed to load this.mapsState = GoogleMapsState.Failed; return false; } From 7343188ffa5bd1238cb0c89a5483e5e6c77ed3b2 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Tue, 10 Dec 2024 17:22:31 +1000 Subject: [PATCH 23/27] Fix race condition in maps.component --- .../components/shared/map/map.component.html | 2 +- .../components/shared/map/map.component.ts | 102 +++++++++++------- 2 files changed, 64 insertions(+), 40 deletions(-) diff --git a/src/app/components/shared/map/map.component.html b/src/app/components/shared/map/map.component.html index 8b034ecc6..1f0da5a3c 100644 --- a/src/app/components/shared/map/map.component.html +++ b/src/app/components/shared/map/map.component.html @@ -6,7 +6,7 @@ (mapClick)="markerOptions?.draggable && newLocation.emit($event)" > { - if (success) { - this.updateMapMarkers(); - } else { - console.warn("Failed to load Google Maps"); - } - }) + this.mapService + .loadAsync() .catch(() => console.warn("Failed to load Google Maps")); } - @ViewChild(GoogleMap) public map: GoogleMap; - @ViewChild(MapInfoWindow) public info: MapInfoWindow; - @ViewChildren(MapMarker) public mapMarkers: QueryList; + // TODO: These ViewChild decorators are making the ngAfterViewChecked hook + // continuously run. We should refactor this component to improve performance + @ViewChild(GoogleMap) public map?: GoogleMap; + @ViewChild(MapInfoWindow) public info?: MapInfoWindow; + @ViewChildren(MapMarker) public mapMarkers?: QueryList; @Input() public markers: List; @Input() public markerOptions: MapMarkerOptions; @Output() public newLocation = new EventEmitter(); - public filteredMarkers: MapMarkerOptions[]; + public validMarkers: MapMarkerOptions[]; public hasMarkers = false; public infoContent = ""; // Setting to "hybrid" can increase load times and looks like the map is bugged public mapOptions: MapOptions = { mapTypeId: "satellite" }; public bounds: google.maps.LatLngBounds; - private updateMap: boolean = false; + + /** + * By setting this flag, the next change detection cycle will cause the + * map bounds and marker information to be updated + */ + private shouldUpdateMapElement: boolean = false; public get googleMapsLoaded(): boolean { return this.mapService.mapsState === GoogleMapsState.Loaded; @@ -72,31 +74,66 @@ export class MapComponent return this.mapService.mapsState === GoogleMapsState.Failed; } - public ngOnChanges(): void { - this.hasMarkers = false; - this.filteredMarkers = []; - - if (!this.googleMapsLoaded) { - return; + /** + * Runs when new markers are added/removed + * This is possible because the markers are an immutable list + */ + public ngOnChanges(changes: SimpleChanges): void { + if ("markers" in changes) { + this.updateFilteredMarkers(); + this.shouldUpdateMapElement = true; } - - this.updateMapMarkers(); } public ngAfterViewChecked(): void { - if (!this.map || !this.hasMarkers || !this.updateMap) { + if (!this.map || !this.hasMarkers || !this.shouldUpdateMapElement) { return; } + this.shouldUpdateMapElement = false; + + this.focusMapMarkers(); + this.addMarkerInformation(); + } + + /** + * Extracts valid markers into `validMarkers` + */ + private updateFilteredMarkers(): void { + this.hasMarkers = false; + this.validMarkers = []; + + this.markers?.forEach((marker) => { + if (isMarkerValid(marker)) { + this.hasMarkers = true; + this.validMarkers.push(marker); + } + }); + } + + /** + * Moves the maps viewport to fit all `filteredMarkers` by calculating marker + * boundaries so that the map has all markers in focus + */ + private focusMapMarkers(): void { + this.bounds = new google.maps.LatLngBounds(); + this.validMarkers.forEach((marker) => { + this.bounds.extend(marker.position); + }); - this.updateMap = false; this.map.fitBounds(this.bounds); this.map.panToBounds(this.bounds); + } - // Setup info windows for each marker + /** + * Adds a map-info-window to each map marker + * This is done to provide a label for each marker when the user hovers over + * the marker + */ + private addMarkerInformation(): void { this.mapMarkers?.forEach((marker, index) => { marker.mapMouseover.pipe(takeUntil(this.unsubscribe)).subscribe({ next: () => { - this.infoContent = this.filteredMarkers[index].label as string; + this.infoContent = this.validMarkers[index].label as string; this.info.open(marker); }, error: () => { @@ -105,19 +142,6 @@ export class MapComponent }); }); } - - private updateMapMarkers(): void { - // Calculate pin boundaries so that map can be auto-focused properly - this.bounds = new google.maps.LatLngBounds(); - this.markers?.forEach((marker) => { - if (isMarkerValid(marker)) { - this.hasMarkers = true; - this.filteredMarkers.push(marker); - this.bounds.extend(marker.position); - } - }); - this.updateMap = true; - } } /** @@ -136,7 +160,7 @@ function isMarkerValid(marker: MapMarkerOptions): boolean { * Handles sanitization of map markers so change detection will run properly */ export function sanitizeMapMarkers( - markers: MapMarkerOptions | MapMarkerOptions[], + markers: MapMarkerOptions | MapMarkerOptions[] ): List { const output: MapMarkerOptions[] = []; From 530c35e264a1cbf708eef8fda74bd6e516b950bc Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Wed, 11 Dec 2024 10:23:31 +1000 Subject: [PATCH 24/27] Remove ngAfterViewChecked from map.componenet --- .../components/shared/map/map.component.html | 16 +-- .../components/shared/map/map.component.ts | 100 ++++++------------ 2 files changed, 42 insertions(+), 74 deletions(-) diff --git a/src/app/components/shared/map/map.component.html b/src/app/components/shared/map/map.component.html index 1f0da5a3c..6137f3384 100644 --- a/src/app/components/shared/map/map.component.html +++ b/src/app/components/shared/map/map.component.html @@ -1,4 +1,4 @@ -@if (hasMarkers && googleMapsLoaded) { +@if (hasMarkers && googleMapsLoaded === true) { {{ infoContent }} } @else {
- @if (googleMapsFailed) { + @if (googleMapsLoaded === false) {

Failure loading map

Please ensure your ad-block is not blocking Google Maps

- } @else if(!googleMapsLoaded) { - } @else if (!hasMarkers) {

No locations specified

+ } @else { + }
} diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index 72b3defdb..3c654fe5e 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -1,29 +1,23 @@ import { - AfterViewChecked, Component, EventEmitter, Input, OnChanges, Output, - QueryList, SimpleChanges, ViewChild, - ViewChildren, } from "@angular/core"; -import { GoogleMap, MapInfoWindow, MapMarker } from "@angular/google-maps"; +import { GoogleMap, MapInfoWindow } from "@angular/google-maps"; import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; import { - GoogleMapsState, MapMarkerOptions, MapOptions, MapsService, } from "@services/maps/maps.service"; import { List } from "immutable"; -import { takeUntil } from "rxjs/operators"; /** * Google Maps Wrapper Component - * ! Manually test when editing, unit test coverage is poor */ @Component({ selector: "baw-map", @@ -32,47 +26,38 @@ import { takeUntil } from "rxjs/operators"; }) export class MapComponent extends withUnsubscribe() - implements OnChanges, AfterViewChecked + implements OnChanges { public constructor(private mapService: MapsService) { super(); this.mapService .loadAsync() + .then((success: boolean) => (this.googleMapsLoaded = success)) .catch(() => console.warn("Failed to load Google Maps")); } - // TODO: These ViewChild decorators are making the ngAfterViewChecked hook - // continuously run. We should refactor this component to improve performance - @ViewChild(GoogleMap) public map?: GoogleMap; @ViewChild(MapInfoWindow) public info?: MapInfoWindow; - @ViewChildren(MapMarker) public mapMarkers?: QueryList; + + @ViewChild(GoogleMap) + private set map(value: GoogleMap) { + this._map = value; + this.focusMarkers(); + } @Input() public markers: List; @Input() public markerOptions: MapMarkerOptions; @Output() public newLocation = new EventEmitter(); - public validMarkers: MapMarkerOptions[]; + public validMarkersOptions: MapMarkerOptions[]; public hasMarkers = false; public infoContent = ""; + private _map: GoogleMap; // Setting to "hybrid" can increase load times and looks like the map is bugged public mapOptions: MapOptions = { mapTypeId: "satellite" }; public bounds: google.maps.LatLngBounds; - - /** - * By setting this flag, the next change detection cycle will cause the - * map bounds and marker information to be updated - */ - private shouldUpdateMapElement: boolean = false; - - public get googleMapsLoaded(): boolean { - return this.mapService.mapsState === GoogleMapsState.Loaded; - } - - public get googleMapsFailed(): boolean { - return this.mapService.mapsState === GoogleMapsState.Failed; - } + protected googleMapsLoaded: boolean | null = null; /** * Runs when new markers are added/removed @@ -81,18 +66,30 @@ export class MapComponent public ngOnChanges(changes: SimpleChanges): void { if ("markers" in changes) { this.updateFilteredMarkers(); - this.shouldUpdateMapElement = true; } } - public ngAfterViewChecked(): void { - if (!this.map || !this.hasMarkers || !this.shouldUpdateMapElement) { + protected addMapMarkerInfo(options: MapMarkerOptions, marker: any): void { + this.infoContent = options.label as string; + this.info.open(marker); + } + + /** + * Moves the maps viewport to fit all `filteredMarkers` by calculating marker + * boundaries so that the map has all markers in focus + */ + protected focusMarkers(): void { + if (!this._map || !this.hasMarkers) { return; } - this.shouldUpdateMapElement = false; - this.focusMapMarkers(); - this.addMarkerInformation(); + this.bounds = new google.maps.LatLngBounds(); + this.validMarkersOptions.forEach((marker) => { + this.bounds.extend(marker.position); + }); + + this._map.fitBounds(this.bounds); + this._map.panToBounds(this.bounds); } /** @@ -100,48 +97,15 @@ export class MapComponent */ private updateFilteredMarkers(): void { this.hasMarkers = false; - this.validMarkers = []; + this.validMarkersOptions = []; this.markers?.forEach((marker) => { if (isMarkerValid(marker)) { this.hasMarkers = true; - this.validMarkers.push(marker); + this.validMarkersOptions.push(marker); } }); } - - /** - * Moves the maps viewport to fit all `filteredMarkers` by calculating marker - * boundaries so that the map has all markers in focus - */ - private focusMapMarkers(): void { - this.bounds = new google.maps.LatLngBounds(); - this.validMarkers.forEach((marker) => { - this.bounds.extend(marker.position); - }); - - this.map.fitBounds(this.bounds); - this.map.panToBounds(this.bounds); - } - - /** - * Adds a map-info-window to each map marker - * This is done to provide a label for each marker when the user hovers over - * the marker - */ - private addMarkerInformation(): void { - this.mapMarkers?.forEach((marker, index) => { - marker.mapMouseover.pipe(takeUntil(this.unsubscribe)).subscribe({ - next: () => { - this.infoContent = this.validMarkers[index].label as string; - this.info.open(marker); - }, - error: () => { - console.error("Failed to create info content for map marker"); - }, - }); - }); - } } /** From 73fe4f2ca6b65d2772ce2e24ea1fca0040e2618d Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Wed, 11 Dec 2024 10:46:44 +1000 Subject: [PATCH 25/27] Clean up map service promises --- .../components/shared/map/map.component.ts | 5 +-- src/app/services/maps/maps.service.ts | 42 +++++++++++-------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index 3c654fe5e..528460c6b 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -24,10 +24,7 @@ import { List } from "immutable"; templateUrl: "./map.component.html", styleUrl: "./map.component.scss", }) -export class MapComponent - extends withUnsubscribe() - implements OnChanges -{ +export class MapComponent extends withUnsubscribe() implements OnChanges { public constructor(private mapService: MapsService) { super(); diff --git a/src/app/services/maps/maps.service.ts b/src/app/services/maps/maps.service.ts index 63c960a47..e937a3460 100644 --- a/src/app/services/maps/maps.service.ts +++ b/src/app/services/maps/maps.service.ts @@ -52,8 +52,13 @@ export class MapsService { const googleMapsUrl = this.googleMapsBundleUrl(); const node: HTMLScriptElement = document.createElement("script"); - node.addEventListener("error", () => { - this.mapsState = GoogleMapsState.Failed; + + const scriptErrorPromise = new Promise((res) => { + node.addEventListener("error", () => { + this.logWarning("Error thrown in external script"); + this.mapsState = GoogleMapsState.Failed; + res(false); + }); }); node.id = "google-maps"; @@ -63,24 +68,24 @@ export class MapsService { document.head.appendChild(node); - // Detect when google maps properly embeds - // TODO: This is a bit of a hack and we should find a better way to detect - // when the google namespace is available + return Promise.race([ + scriptErrorPromise, + this.waitForGoogleNamespace(), + ]) as Promise; + } + + private async waitForGoogleNamespace(): Promise { const instantiationRetries = 10; for (let retry = 0; retry < instantiationRetries; retry++) { - // because the "failed" state can be asynchronously updated by the script - // elements error event listener, we check if the state has changed to - // a "failed" state so that we can return false from this functions - // promise - if ((this.mapsState as GoogleMapsState) === GoogleMapsState.Failed) { - return false; - } - // because the "google" global namespace is loaded by the google maps // script, we can check if it is defined to determine if the script has // been successfully loaded - if (typeof google !== "undefined") { + if ( + typeof google !== "undefined" && + typeof google.maps !== "undefined" && + typeof google.maps.importLibrary !== "undefined" + ) { this.mapsState = GoogleMapsState.Loaded; return true; } @@ -88,9 +93,8 @@ export class MapsService { await sleep(defaultDebounceTime); } - // if we reach this point, the "google" namespace was never defined in the - // global scope, so we assume the script failed to load - this.mapsState = GoogleMapsState.Failed; + this.logWarning("Unable to find 'google' namespace"); + return false; } @@ -113,4 +117,8 @@ export class MapsService { return googleMapsUrl; } + + private logWarning(message: string): void { + console.warn(`Maps: ${message}`); + } } From fccc7bd03a53be30daa93cc2961352eefb967b0b Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Wed, 11 Dec 2024 10:59:38 +1000 Subject: [PATCH 26/27] Fix variable overloading --- src/app/components/shared/map/map.component.html | 8 ++++---- src/app/components/shared/map/map.component.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/app/components/shared/map/map.component.html b/src/app/components/shared/map/map.component.html index 6137f3384..d93016bc6 100644 --- a/src/app/components/shared/map/map.component.html +++ b/src/app/components/shared/map/map.component.html @@ -6,12 +6,12 @@ (mapClick)="markerOptions?.draggable && newLocation.emit($event)" > diff --git a/src/app/components/shared/map/map.component.ts b/src/app/components/shared/map/map.component.ts index 528460c6b..b23e09cad 100644 --- a/src/app/components/shared/map/map.component.ts +++ b/src/app/components/shared/map/map.component.ts @@ -7,7 +7,7 @@ import { SimpleChanges, ViewChild, } from "@angular/core"; -import { GoogleMap, MapInfoWindow } from "@angular/google-maps"; +import { GoogleMap, MapAnchorPoint, MapInfoWindow } from "@angular/google-maps"; import { withUnsubscribe } from "@helpers/unsubscribe/unsubscribe"; import { MapMarkerOptions, @@ -66,7 +66,7 @@ export class MapComponent extends withUnsubscribe() implements OnChanges { } } - protected addMapMarkerInfo(options: MapMarkerOptions, marker: any): void { + protected addMapMarkerInfo(options: MapMarkerOptions, marker: MapAnchorPoint): void { this.infoContent = options.label as string; this.info.open(marker); } From 533b6ee48b1b8db707d38daebe8a16db7b3ef989 Mon Sep 17 00:00:00 2001 From: hudson-newey Date: Wed, 11 Dec 2024 11:33:44 +1000 Subject: [PATCH 27/27] Fix testing map loading state --- src/app/components/shared/map/map.component.html | 4 ++-- src/app/components/shared/map/map.component.spec.ts | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/app/components/shared/map/map.component.html b/src/app/components/shared/map/map.component.html index d93016bc6..8955dab34 100644 --- a/src/app/components/shared/map/map.component.html +++ b/src/app/components/shared/map/map.component.html @@ -24,10 +24,10 @@

Please ensure your ad-block is not blocking Google Maps

+ } @else if (googleMapsLoaded === null) { + } @else if (!hasMarkers) {

No locations specified

- } @else { - }
} diff --git a/src/app/components/shared/map/map.component.spec.ts b/src/app/components/shared/map/map.component.spec.ts index 2c179ce7b..95bcb5fff 100644 --- a/src/app/components/shared/map/map.component.spec.ts +++ b/src/app/components/shared/map/map.component.spec.ts @@ -58,12 +58,14 @@ describe("MapComponent", () => { /** Causes all pending 'loadAsync' promises to resolve */ function triggerLoadSuccess(): void { mapsServiceSpy.mapsState = GoogleMapsState.Loaded; + spectator.component["googleMapsLoaded"] = true; spectator.detectChanges(); } /** Causes all pending 'loadAsync' promises to reject */ function triggerLoadFailure(): void { mapsServiceSpy.mapsState = GoogleMapsState.Failed; + spectator.component["googleMapsLoaded"] = false; spectator.detectChanges(); }