From 98fda25715cd2a3b410b860a288e5f9eca3cb47c Mon Sep 17 00:00:00 2001 From: Dori Maloney Date: Mon, 4 Nov 2024 16:29:40 -0700 Subject: [PATCH 1/5] Added fix to allow latitude or longitude of 0 --- src/marker-utils.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/marker-utils.ts b/src/marker-utils.ts index a28f3b5b..31917d3f 100644 --- a/src/marker-utils.ts +++ b/src/marker-utils.ts @@ -58,7 +58,12 @@ export class MarkerUtils { return marker.position; } // since we can't cast to LatLngLiteral for reasons =( - if (marker.position.lat && marker.position.lng) { + if ( + marker.position.lat !== null && + marker.position.lat !== undefined && + marker.position.lng !== null && + marker.position.lng !== undefined + ) { return new google.maps.LatLng( marker.position.lat, marker.position.lng From e14e21cda0970afac557618aafa727a7faf76b48 Mon Sep 17 00:00:00 2001 From: Dori Maloney Date: Tue, 5 Nov 2024 12:38:38 -0700 Subject: [PATCH 2/5] Added unit tests --- src/marker-utils.test.ts | 46 +++++++++++++++++++++++++++++----------- src/marker-utils.ts | 17 +++++++++++++++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/marker-utils.test.ts b/src/marker-utils.test.ts index 72d060be..75736620 100644 --- a/src/marker-utils.test.ts +++ b/src/marker-utils.test.ts @@ -14,8 +14,8 @@ * limitations under the License. */ -import { MarkerUtils } from "./marker-utils"; -import { initialize } from "@googlemaps/jest-mocks"; +import { MarkerUtils } from './marker-utils'; +import { initialize } from '@googlemaps/jest-mocks'; initialize(); const markerClasses = [ @@ -24,15 +24,15 @@ const markerClasses = [ ]; describe.each(markerClasses)( - "MarkerUtils works with legacy and Advanced Markers", + 'MarkerUtils works with legacy and Advanced Markers', (markerClass) => { let map: google.maps.Map; beforeEach(() => { - map = new google.maps.Map(document.createElement("div")); + map = new google.maps.Map(document.createElement('div')); }); - test("identifies AdvancedMarker instances", () => { + test('identifies AdvancedMarker instances', () => { const isAdvancedMarker = MarkerUtils.isAdvancedMarker(new markerClass()); if (markerClass === google.maps.marker.AdvancedMarkerElement) { expect(isAdvancedMarker).toBeTruthy; @@ -41,7 +41,7 @@ describe.each(markerClasses)( expect(isAdvancedMarker).toBeFalsy; }); - test("sets the map", () => { + test('sets the map', () => { const marker = new markerClass(); MarkerUtils.setMap(marker, map); if (markerClass === google.maps.marker.AdvancedMarkerElement) { @@ -53,21 +53,43 @@ describe.each(markerClasses)( expect((marker as google.maps.Marker).setMap).toHaveBeenCalled; }); - test("gets the marker position and returns a LatLng", () => { + test('gets the marker position and returns a LatLng', () => { // test markers created with LatLng and LatLngLiteral - [new google.maps.LatLng(1, 1), { lat: 1, lng: 1 }].forEach((position) => { + [ + new google.maps.LatLng(1, 1), + { lat: 1, lng: 1 }, + new google.maps.LatLng(0, 0), + { lat: 0, lng: 0 }, + new google.maps.LatLng(0, 1), + { lat: 0, lng: 1 }, + new google.maps.LatLng(1, 0), + { lat: 1, lng: 0 }, + ].forEach((position) => { const marker = new markerClass({ position: position }); if (markerClass === google.maps.marker.AdvancedMarkerElement) { (marker as google.maps.marker.AdvancedMarkerElement).position = position; } - expect(MarkerUtils.getPosition(marker)).toBeInstanceOf( - google.maps.LatLng - ); + + const markerPosition = MarkerUtils.getPosition(marker); + + expect(markerPosition).toBeInstanceOf(google.maps.LatLng); + + if (typeof position.lat === 'number') { + expect(markerPosition.lat()).toEqual(position.lat); + } else if (typeof position.lat === 'function') { + expect(markerPosition.lat()).toEqual(position.lat()); + } + + if (typeof position.lng === 'number') { + expect(markerPosition.lng()).toEqual(position.lng); + } else if (typeof position.lng === 'function') { + expect(markerPosition.lng()).toEqual(position.lng()); + } }); }); - test("", () => { + test('detects the visibility of a marker', () => { const marker = new markerClass(); expect(MarkerUtils.getVisible(marker)).toBeTruthy; }); diff --git a/src/marker-utils.ts b/src/marker-utils.ts index 31917d3f..3614e783 100644 --- a/src/marker-utils.ts +++ b/src/marker-utils.ts @@ -54,6 +54,23 @@ export class MarkerUtils { // SuperClusterAlgorithm.calculate expects a LatLng instance so we fake it for Adv Markers if (this.isAdvancedMarker(marker)) { if (marker.position) { + /* + TODO: This check doesn't work when a `loader` is used to create the position object. + The type check sees `marker.position` as a generic `object`, but I'm not sure how to + fix this. Example `LatLng` object creation to reproduce: + ```ts + import {Loader} from '@googlemaps/js-api-loader'; + + const loader = new Loader(...); + const {LatLng} = await loader.importLibrary('core'); + const latLng = new LatLng({lat: latitude, lng: longitude}); + + const newAdvancedMarker = new AdvancedMarkerElement({ + position: latLng, + ... + }); + ``` + */ if (marker.position instanceof google.maps.LatLng) { return marker.position; } From 3a235f69a384ee8380654c27470af26fa1b92d5e Mon Sep 17 00:00:00 2001 From: Dori Maloney Date: Tue, 5 Nov 2024 13:30:35 -0700 Subject: [PATCH 3/5] Reorganized unit tests to target marker scenarios --- src/marker-utils.test.ts | 107 ++++++++++++++++++++++++++++++--------- src/marker-utils.ts | 20 ++++++-- 2 files changed, 99 insertions(+), 28 deletions(-) diff --git a/src/marker-utils.test.ts b/src/marker-utils.test.ts index 75736620..809a7670 100644 --- a/src/marker-utils.test.ts +++ b/src/marker-utils.test.ts @@ -23,6 +23,20 @@ const markerClasses = [ google.maps.marker.AdvancedMarkerElement, ]; +const latLngLiteralTestScenarios = [ + { lat: 1, lng: 1 }, + { lat: 0, lng: 0 }, + { lat: 0, lng: 1 }, + { lat: 1, lng: 0 }, +]; + +const latLngTestScenarios = [ + new google.maps.LatLng(1, 1), + new google.maps.LatLng(0, 0), + new google.maps.LatLng(0, 1), + new google.maps.LatLng(1, 0), +]; + describe.each(markerClasses)( 'MarkerUtils works with legacy and Advanced Markers', (markerClass) => { @@ -53,40 +67,85 @@ describe.each(markerClasses)( expect((marker as google.maps.Marker).setMap).toHaveBeenCalled; }); - test('gets the marker position and returns a LatLng', () => { - // test markers created with LatLng and LatLngLiteral - [ - new google.maps.LatLng(1, 1), - { lat: 1, lng: 1 }, - new google.maps.LatLng(0, 0), - { lat: 0, lng: 0 }, - new google.maps.LatLng(0, 1), - { lat: 0, lng: 1 }, - new google.maps.LatLng(1, 0), - { lat: 1, lng: 0 }, - ].forEach((position) => { - const marker = new markerClass({ position: position }); - if (markerClass === google.maps.marker.AdvancedMarkerElement) { - (marker as google.maps.marker.AdvancedMarkerElement).position = - position; - } + test('gets the marker position from an AdvancedMarkerElement with a LatLng position', () => { + latLngTestScenarios.forEach((position: google.maps.LatLng) => { + const marker = new google.maps.marker.AdvancedMarkerElement({ + position: position, + }); const markerPosition = MarkerUtils.getPosition(marker); expect(markerPosition).toBeInstanceOf(google.maps.LatLng); + expect(markerPosition.lat()).toEqual(position.lat()); + expect(markerPosition.lng()).toEqual(position.lng()); + }); + }); + + test.only('gets the marker position from an AdvancedMarkerElement with a LatLngLiteral position', () => { + latLngLiteralTestScenarios.forEach( + (position: google.maps.LatLngLiteral) => { + const marker = new google.maps.marker.AdvancedMarkerElement(); + // const marker = new google.maps.marker.AdvancedMarkerElement({ + // position: position, + // }); + + marker.position = position; + + console.log( + ` + Advanced marker position: ${JSON.stringify(marker.position)} + ${typeof marker.position.lat} + ${typeof marker.position.lng} + ` + ); + + const markerPosition = MarkerUtils.getPosition(marker); + + // console.log(` + // Original: [${JSON.stringify(position)}] + // markerClass data: [${JSON.stringify(markerClass)}] + // markerClass type: [${typeof markerClass}] + // Converted: [${markerPosition.lat()}, ${markerPosition.lng()}] + // `); + // console.log( + // `Converted: [${markerPosition.lat()}, ${markerPosition.lng()}]` + // ); + + console.log( + `Received [${markerPosition.lat()}, ${markerPosition.lng()}]` + ); - if (typeof position.lat === 'number') { + expect(markerPosition).toBeInstanceOf(google.maps.LatLng); expect(markerPosition.lat()).toEqual(position.lat); - } else if (typeof position.lat === 'function') { - expect(markerPosition.lat()).toEqual(position.lat()); + expect(markerPosition.lng()).toEqual(position.lng); } + ); + }); - if (typeof position.lng === 'number') { + test('gets the marker position from a Marker with a LatLng position', () => { + latLngTestScenarios.forEach((position: google.maps.LatLng) => { + const marker = new google.maps.Marker({ position: position }); + + const markerPosition = MarkerUtils.getPosition(marker); + + expect(markerPosition).toBeInstanceOf(google.maps.LatLng); + expect(markerPosition.lat()).toEqual(position.lat()); + expect(markerPosition.lng()).toEqual(position.lng()); + }); + }); + + test('gets the marker position from a Marker with a LatLngLiteral position', () => { + latLngLiteralTestScenarios.forEach( + (position: google.maps.LatLngLiteral) => { + const marker = new google.maps.Marker({ position: position }); + + const markerPosition = MarkerUtils.getPosition(marker); + + expect(markerPosition).toBeInstanceOf(google.maps.LatLng); + expect(markerPosition.lat()).toEqual(position.lat); expect(markerPosition.lng()).toEqual(position.lng); - } else if (typeof position.lng === 'function') { - expect(markerPosition.lng()).toEqual(position.lng()); } - }); + ); }); test('detects the visibility of a marker', () => { diff --git a/src/marker-utils.ts b/src/marker-utils.ts index 3614e783..9005953b 100644 --- a/src/marker-utils.ts +++ b/src/marker-utils.ts @@ -76,19 +76,31 @@ export class MarkerUtils { } // since we can't cast to LatLngLiteral for reasons =( if ( - marker.position.lat !== null && - marker.position.lat !== undefined && - marker.position.lng !== null && - marker.position.lng !== undefined + // marker.position.lat !== null && + // marker.position.lat !== undefined && + // marker.position.lng !== null && + // marker.position.lng !== undefined + typeof marker.position.lat === 'number' && + typeof marker.position.lng === 'number' ) { + console.log('Inside LatLngLiteral check...'); return new google.maps.LatLng( marker.position.lat, marker.position.lng ); } } + console.log( + `Returning empty LatLng for Advanced marker... Position was ${JSON.stringify( + marker.position + )} with type ${typeof marker.position}` + ); + return new google.maps.LatLng(null); } + + console.log('Returning regular Marker position...'); + return marker.getPosition(); } From 582e945d8edfe073217ab3923cbce0a7ffe31076 Mon Sep 17 00:00:00 2001 From: Dori Maloney Date: Tue, 5 Nov 2024 13:35:06 -0700 Subject: [PATCH 4/5] Cleaned up code for review --- src/marker-utils.test.ts | 33 ++++----------------------------- src/marker-utils.ts | 12 ------------ 2 files changed, 4 insertions(+), 41 deletions(-) diff --git a/src/marker-utils.test.ts b/src/marker-utils.test.ts index 809a7670..870bf5ec 100644 --- a/src/marker-utils.test.ts +++ b/src/marker-utils.test.ts @@ -69,9 +69,9 @@ describe.each(markerClasses)( test('gets the marker position from an AdvancedMarkerElement with a LatLng position', () => { latLngTestScenarios.forEach((position: google.maps.LatLng) => { - const marker = new google.maps.marker.AdvancedMarkerElement({ - position: position, - }); + const marker = new google.maps.marker.AdvancedMarkerElement(); + + marker.position = position; const markerPosition = MarkerUtils.getPosition(marker); @@ -81,40 +81,15 @@ describe.each(markerClasses)( }); }); - test.only('gets the marker position from an AdvancedMarkerElement with a LatLngLiteral position', () => { + test('gets the marker position from an AdvancedMarkerElement with a LatLngLiteral position', () => { latLngLiteralTestScenarios.forEach( (position: google.maps.LatLngLiteral) => { const marker = new google.maps.marker.AdvancedMarkerElement(); - // const marker = new google.maps.marker.AdvancedMarkerElement({ - // position: position, - // }); marker.position = position; - console.log( - ` - Advanced marker position: ${JSON.stringify(marker.position)} - ${typeof marker.position.lat} - ${typeof marker.position.lng} - ` - ); - const markerPosition = MarkerUtils.getPosition(marker); - // console.log(` - // Original: [${JSON.stringify(position)}] - // markerClass data: [${JSON.stringify(markerClass)}] - // markerClass type: [${typeof markerClass}] - // Converted: [${markerPosition.lat()}, ${markerPosition.lng()}] - // `); - // console.log( - // `Converted: [${markerPosition.lat()}, ${markerPosition.lng()}]` - // ); - - console.log( - `Received [${markerPosition.lat()}, ${markerPosition.lng()}]` - ); - expect(markerPosition).toBeInstanceOf(google.maps.LatLng); expect(markerPosition.lat()).toEqual(position.lat); expect(markerPosition.lng()).toEqual(position.lng); diff --git a/src/marker-utils.ts b/src/marker-utils.ts index 9005953b..8d17021c 100644 --- a/src/marker-utils.ts +++ b/src/marker-utils.ts @@ -76,31 +76,19 @@ export class MarkerUtils { } // since we can't cast to LatLngLiteral for reasons =( if ( - // marker.position.lat !== null && - // marker.position.lat !== undefined && - // marker.position.lng !== null && - // marker.position.lng !== undefined typeof marker.position.lat === 'number' && typeof marker.position.lng === 'number' ) { - console.log('Inside LatLngLiteral check...'); return new google.maps.LatLng( marker.position.lat, marker.position.lng ); } } - console.log( - `Returning empty LatLng for Advanced marker... Position was ${JSON.stringify( - marker.position - )} with type ${typeof marker.position}` - ); return new google.maps.LatLng(null); } - console.log('Returning regular Marker position...'); - return marker.getPosition(); } From a08954ee3975d25790fbde7021602e70a5a7b91b Mon Sep 17 00:00:00 2001 From: Dori Maloney Date: Tue, 5 Nov 2024 14:18:56 -0700 Subject: [PATCH 5/5] Fixed linting errors --- src/marker-utils.test.ts | 22 +++++++++++----------- src/marker-utils.ts | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/marker-utils.test.ts b/src/marker-utils.test.ts index 870bf5ec..15461787 100644 --- a/src/marker-utils.test.ts +++ b/src/marker-utils.test.ts @@ -14,8 +14,8 @@ * limitations under the License. */ -import { MarkerUtils } from './marker-utils'; -import { initialize } from '@googlemaps/jest-mocks'; +import { MarkerUtils } from "./marker-utils"; +import { initialize } from "@googlemaps/jest-mocks"; initialize(); const markerClasses = [ @@ -38,15 +38,15 @@ const latLngTestScenarios = [ ]; describe.each(markerClasses)( - 'MarkerUtils works with legacy and Advanced Markers', + "MarkerUtils works with legacy and Advanced Markers", (markerClass) => { let map: google.maps.Map; beforeEach(() => { - map = new google.maps.Map(document.createElement('div')); + map = new google.maps.Map(document.createElement("div")); }); - test('identifies AdvancedMarker instances', () => { + test("identifies AdvancedMarker instances", () => { const isAdvancedMarker = MarkerUtils.isAdvancedMarker(new markerClass()); if (markerClass === google.maps.marker.AdvancedMarkerElement) { expect(isAdvancedMarker).toBeTruthy; @@ -55,7 +55,7 @@ describe.each(markerClasses)( expect(isAdvancedMarker).toBeFalsy; }); - test('sets the map', () => { + test("sets the map", () => { const marker = new markerClass(); MarkerUtils.setMap(marker, map); if (markerClass === google.maps.marker.AdvancedMarkerElement) { @@ -67,7 +67,7 @@ describe.each(markerClasses)( expect((marker as google.maps.Marker).setMap).toHaveBeenCalled; }); - test('gets the marker position from an AdvancedMarkerElement with a LatLng position', () => { + test("gets the marker position from an AdvancedMarkerElement with a LatLng position", () => { latLngTestScenarios.forEach((position: google.maps.LatLng) => { const marker = new google.maps.marker.AdvancedMarkerElement(); @@ -81,7 +81,7 @@ describe.each(markerClasses)( }); }); - test('gets the marker position from an AdvancedMarkerElement with a LatLngLiteral position', () => { + test("gets the marker position from an AdvancedMarkerElement with a LatLngLiteral position", () => { latLngLiteralTestScenarios.forEach( (position: google.maps.LatLngLiteral) => { const marker = new google.maps.marker.AdvancedMarkerElement(); @@ -97,7 +97,7 @@ describe.each(markerClasses)( ); }); - test('gets the marker position from a Marker with a LatLng position', () => { + test("gets the marker position from a Marker with a LatLng position", () => { latLngTestScenarios.forEach((position: google.maps.LatLng) => { const marker = new google.maps.Marker({ position: position }); @@ -109,7 +109,7 @@ describe.each(markerClasses)( }); }); - test('gets the marker position from a Marker with a LatLngLiteral position', () => { + test("gets the marker position from a Marker with a LatLngLiteral position", () => { latLngLiteralTestScenarios.forEach( (position: google.maps.LatLngLiteral) => { const marker = new google.maps.Marker({ position: position }); @@ -123,7 +123,7 @@ describe.each(markerClasses)( ); }); - test('detects the visibility of a marker', () => { + test("detects the visibility of a marker", () => { const marker = new markerClass(); expect(MarkerUtils.getVisible(marker)).toBeTruthy; }); diff --git a/src/marker-utils.ts b/src/marker-utils.ts index 8d17021c..125f733b 100644 --- a/src/marker-utils.ts +++ b/src/marker-utils.ts @@ -76,8 +76,8 @@ export class MarkerUtils { } // since we can't cast to LatLngLiteral for reasons =( if ( - typeof marker.position.lat === 'number' && - typeof marker.position.lng === 'number' + typeof marker.position.lat === "number" && + typeof marker.position.lng === "number" ) { return new google.maps.LatLng( marker.position.lat,