From cdf1624f1ed5554de68ece860c5e39e7adc47ed2 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Thu, 9 Apr 2026 22:57:34 +1000 Subject: [PATCH 1/2] fix(google-maps): close races in resolveQueryToLatLng resolveQueryToLatLng could hang forever or throw, depending on timing: 1. The wait pattern after `await load()` used a non-immediate watcher. When load() populated mapsApi synchronously the watcher missed the change and the promise never resolved. 2. PlacesService construction dereferenced `map.value!`, but the function is publicly exposed and could be called before onLoaded populated the map ref, throwing on the bang. Extract a `waitForMapsReady` helper into useGoogleMapsResource that mirrors importLibrary's correct pattern: short-circuit when both refs are already set, re-check after `await load()`, then install an immediate watcher inside a detached effect scope. The watcher waits for both mapsApi and map and rejects if status enters 'error'. --- .../GoogleMaps/ScriptGoogleMaps.vue | 18 +-- .../GoogleMaps/useGoogleMapsResource.ts | 63 +++++++++- test/unit/google-maps-lifecycle.test.ts | 113 +++++++++++++++++- 3 files changed, 180 insertions(+), 14 deletions(-) diff --git a/packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue b/packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue index 8a37d4ef..d0db9b3d 100644 --- a/packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue +++ b/packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue @@ -138,7 +138,7 @@ import { defu } from 'defu' import { tryUseNuxtApp, useHead, useRuntimeConfig } from 'nuxt/app' import { computed, onBeforeUnmount, onMounted, provide, ref, shallowRef, toRaw, useAttrs, useTemplateRef, watch } from 'vue' import ScriptAriaLoadingIndicator from '../ScriptAriaLoadingIndicator.vue' -import { MAP_INJECTION_KEY } from './useGoogleMapsResource' +import { MAP_INJECTION_KEY, waitForMapsReady } from './useGoogleMapsResource' const props = withDefaults(defineProps(), { // @ts-expect-error untyped @@ -241,17 +241,11 @@ async function resolveQueryToLatLng(query: string) { throw new Error(`No location found for ${query}`) } - // Fallback: use Places API client-side - if (!mapsApi.value) { - await load() - // await new promise, watch until mapsApi is set - await new Promise((resolve) => { - const _ = watch(mapsApi, () => { - _() - resolve() - }) - }) - } + // Fallback: use Places API client-side. Wait for both the maps API and a + // Map instance: resolveQueryToLatLng is publicly exposed and may be called + // before onLoaded has populated map.value, so constructing PlacesService + // without map would throw. + await waitForMapsReady({ mapsApi, map, status, load }) const placesService = new mapsApi.value!.places.PlacesService(map.value!) const result = await new Promise((resolve, reject) => { diff --git a/packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts b/packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts index 081df7d2..3a302998 100644 --- a/packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts +++ b/packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts @@ -1,6 +1,6 @@ import type { InjectionKey, Ref, ShallowRef } from 'vue' import { whenever } from '@vueuse/core' -import { inject, onUnmounted, ref, shallowRef } from 'vue' +import { effectScope, inject, onUnmounted, ref, shallowRef, watch } from 'vue' export const MAP_INJECTION_KEY = Symbol('map') as InjectionKey<{ map: ShallowRef @@ -41,6 +41,67 @@ export interface GoogleMapsResourceContext { mapsApi: typeof google.maps } +/** + * Wait until the Google Maps API and a Map instance are both available. + * + * Triggers script loading via `load()` if not already loaded. Uses an + * immediate watcher (matching `importLibrary`'s pattern) to avoid the race + * where `load()` resolves synchronously: a non-immediate watcher would miss + * the change and the promise would hang forever. + * + * Rejects if `status` enters an `'error'` state before both refs are populated. + * Runs the watcher inside a detached effect scope so it is safe to call from + * any context (component setup, exposed methods, tests). + */ +export async function waitForMapsReady({ + mapsApi, + map, + status, + load, +}: { + mapsApi: ShallowRef + map: ShallowRef + status: Ref + load: () => Promise | unknown +}): Promise { + if (mapsApi.value && map.value) + return + if (status.value === 'error') + throw new Error('Google Maps script failed to load') + + await load() + + // load() may have populated both refs synchronously — re-check before + // installing a watcher to avoid the race that hangs the promise forever. + if (mapsApi.value && map.value) + return + if (status.value === 'error') + throw new Error('Google Maps script failed to load') + + const scope = effectScope(true) + try { + await new Promise((resolve, reject) => { + scope.run(() => { + watch( + [mapsApi, map, status], + ([api, m, s]) => { + if (api && m) { + resolve() + return + } + if (s === 'error') + reject(new Error('Google Maps script failed to load')) + }, + { immediate: true }, + ) + }) + }) + } + finally { + scope.stop() + } +} + /** * Composable for safely managing Google Maps resource lifecycle. * diff --git a/test/unit/google-maps-lifecycle.test.ts b/test/unit/google-maps-lifecycle.test.ts index 48fd5533..729e6273 100644 --- a/test/unit/google-maps-lifecycle.test.ts +++ b/test/unit/google-maps-lifecycle.test.ts @@ -4,7 +4,7 @@ import { mount } from '@vue/test-utils' import { beforeEach, describe, expect, it, vi } from 'vitest' import { defineComponent, h, nextTick, onUnmounted, provide, ref, shallowRef } from 'vue' -import { MAP_INJECTION_KEY, useGoogleMapsResource } from '../../packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource' +import { MAP_INJECTION_KEY, useGoogleMapsResource, waitForMapsReady } from '../../packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource' import { createMockGoogleMapsAPI } from './__mocks__/google-maps-api' // Helper to create a wrapper component that provides mock map context @@ -367,3 +367,114 @@ describe('google Maps component lifecycle - memory leak prevention', () => { ) }) }) + +describe('waitForMapsReady', () => { + // Regression: the previous resolveQueryToLatLng wait pattern used a + // non-immediate watch after `await load()`. If load() populated mapsApi + // synchronously, the watcher missed the change and the promise hung + // forever. The fix re-checks state after load() and uses an immediate + // watcher (matching importLibrary's pattern). It also waits for `map` + // to be set, since PlacesService construction requires it. + + it('returns immediately when both refs are already populated', async () => { + const mapsApi = shallowRef({ places: {} }) + const map = shallowRef({}) + const status = ref('loaded') + const load = vi.fn(() => Promise.resolve()) + + await waitForMapsReady({ mapsApi, map, status, load }) + + // Should not have called load() at all + expect(load).not.toHaveBeenCalled() + }) + + it('does not hang when load() resolves synchronously', async () => { + // Reproduces the original race: load() populates mapsApi synchronously, + // so a non-immediate watcher would never fire. + const mapsApi = shallowRef(undefined) + const map = shallowRef(undefined) + const status = ref('loading') + const load = vi.fn(() => { + // Synchronously populate both refs before returning the resolved promise + mapsApi.value = { places: {} } + map.value = {} + return Promise.resolve() + }) + + // Race against a timeout to detect the hang + await expect( + Promise.race([ + waitForMapsReady({ mapsApi, map, status, load }), + new Promise((_, reject) => setTimeout(() => reject(new Error('hang')), 100)), + ]), + ).resolves.toBeUndefined() + + expect(load).toHaveBeenCalledOnce() + }) + + it('resolves when refs are populated asynchronously after load()', async () => { + const mapsApi = shallowRef(undefined) + const map = shallowRef(undefined) + const status = ref('loading') + const load = vi.fn(() => Promise.resolve()) + + const promise = waitForMapsReady({ mapsApi, map, status, load }) + + // Populate refs after a tick — simulates the normal onLoaded flow + await nextTick() + mapsApi.value = { places: {} } + await nextTick() + map.value = {} + + await expect(promise).resolves.toBeUndefined() + }) + + it('rejects synchronously when status is already error', async () => { + const mapsApi = shallowRef(undefined) + const map = shallowRef(undefined) + const status = ref('error') + const load = vi.fn(() => Promise.resolve()) + + await expect( + waitForMapsReady({ mapsApi, map, status, load }), + ).rejects.toThrow('Google Maps script failed to load') + + // Should bail before calling load() + expect(load).not.toHaveBeenCalled() + }) + + it('rejects when status transitions to error during the wait', async () => { + const mapsApi = shallowRef(undefined) + const map = shallowRef(undefined) + const status = ref('loading') + const load = vi.fn(() => Promise.resolve()) + + const promise = waitForMapsReady({ mapsApi, map, status, load }) + + await nextTick() + status.value = 'error' + + await expect(promise).rejects.toThrow('Google Maps script failed to load') + }) + + it('waits for map even when mapsApi is set first', async () => { + // Guards against the second bug: PlacesService construction needs + // map.value, not just mapsApi.value. + const mapsApi = shallowRef({ places: {} }) + const map = shallowRef(undefined) + const status = ref('loading') + const load = vi.fn(() => Promise.resolve()) + + const promise = waitForMapsReady({ mapsApi, map, status, load }) + + // Race against a short timeout: should NOT resolve yet + const racedEarly = await Promise.race([ + promise.then(() => 'resolved'), + new Promise(resolve => setTimeout(resolve, 30, 'pending')), + ]) + expect(racedEarly).toBe('pending') + + map.value = {} + await expect(promise).resolves.toBeUndefined() + }) +}) From e0ca92a83a4d31e35ad2d07a6bf277c2898e78c2 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Thu, 9 Apr 2026 23:07:17 +1000 Subject: [PATCH 2/2] test(google-maps): drop wall-clock timeouts in waitForMapsReady tests CodeRabbit flagged the setTimeout-based race patterns as potentially flaky on slow CI. Replace with deterministic alternatives: - "does not hang" test now relies on vitest's per-test timeout as the backstop. The fix prevents the hang, so the await resolves immediately on the happy path. - "waits for map" test uses a settled flag plus nextTick flushes instead of a wall-clock setTimeout race. --- test/unit/google-maps-lifecycle.test.ts | 26 ++++++++++++------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/test/unit/google-maps-lifecycle.test.ts b/test/unit/google-maps-lifecycle.test.ts index 729e6273..dfbd93ed 100644 --- a/test/unit/google-maps-lifecycle.test.ts +++ b/test/unit/google-maps-lifecycle.test.ts @@ -401,13 +401,9 @@ describe('waitForMapsReady', () => { return Promise.resolve() }) - // Race against a timeout to detect the hang - await expect( - Promise.race([ - waitForMapsReady({ mapsApi, map, status, load }), - new Promise((_, reject) => setTimeout(() => reject(new Error('hang')), 100)), - ]), - ).resolves.toBeUndefined() + // The original race would hang forever — vitest's per-test timeout + // is the deterministic backstop, no wall-clock setTimeout needed. + await expect(waitForMapsReady({ mapsApi, map, status, load })).resolves.toBeUndefined() expect(load).toHaveBeenCalledOnce() }) @@ -465,16 +461,18 @@ describe('waitForMapsReady', () => { const status = ref('loading') const load = vi.fn(() => Promise.resolve()) - const promise = waitForMapsReady({ mapsApi, map, status, load }) + let settled = false + const promise = waitForMapsReady({ mapsApi, map, status, load }).then(() => { + settled = true + }) - // Race against a short timeout: should NOT resolve yet - const racedEarly = await Promise.race([ - promise.then(() => 'resolved'), - new Promise(resolve => setTimeout(resolve, 30, 'pending')), - ]) - expect(racedEarly).toBe('pending') + // Flush microtasks: settlement should not happen while map is undefined + await nextTick() + await nextTick() + expect(settled).toBe(false) map.value = {} await expect(promise).resolves.toBeUndefined() + expect(settled).toBe(true) }) })