fix: prevent memory leaks in all Google Maps sub-components#651
Merged
fix: prevent memory leaks in all Google Maps sub-components#651
Conversation
The previous fix (#649) only handled one race condition — unmounting during `await importLibrary`. The actual leak was caused by orphaned Vue watchers: watchers created after an `await` lose component instance context and are never auto-stopped on unmount, retaining the entire reactive scope. - Create `useGoogleMapsResource` composable encoding lifecycle safety - Move all options watchers to synchronous setup scope (Vue auto-stops) - Add unmount guards to all async resource creation callbacks - Null all resource refs on unmount to release Google Maps objects - Convert `let` variables to `shallowRef` for proper GC - Make parent `onBeforeUnmount` synchronous (Vue doesn't await async hooks) - Clear `libraries` and `queryToLatLngCache` on parent unmount - Extract injection keys to shared `injectionKeys.ts` Closes #646
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
Replace verbose AI-generated best practices, error handling, and reactive data pattern sections with a concise component reference table and hierarchy diagram.
The .vue component imports fail typecheck in CI because google.maps types are not available in the vitest typecheck context.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors the Google Maps component library to centralize lifecycle management patterns. A new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/runtime/components/GoogleMaps/injectionKeys.ts (1)
8-14: Consider using distinct Symbol descriptions for debugging clarity.Both
ADVANCED_MARKER_ELEMENT_INJECTION_KEY(line 8) andMARKER_INJECTION_KEY(line 12) useSymbol('marker'). While functionally correct (eachSymbol()call creates a unique symbol), using the same description string can make debugging harder when inspecting injection keys in Vue DevTools or error messages.♻️ Suggested improvement
-export const ADVANCED_MARKER_ELEMENT_INJECTION_KEY = Symbol('marker') as InjectionKey<{ +export const ADVANCED_MARKER_ELEMENT_INJECTION_KEY = Symbol('advancedMarkerElement') as InjectionKey<{ advancedMarkerElement: ShallowRef<google.maps.marker.AdvancedMarkerElement | undefined> }> export const MARKER_INJECTION_KEY = Symbol('marker') as InjectionKey<{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/GoogleMaps/injectionKeys.ts` around lines 8 - 14, The two injection keys currently both call Symbol('marker'), which makes debugging harder; update ADVANCED_MARKER_ELEMENT_INJECTION_KEY to use a distinct description (e.g., Symbol('advanced-marker-element') or similar) and keep MARKER_INJECTION_KEY with a clear description (e.g., Symbol('marker')), so replace the Symbol('marker') in the ADVANCED_MARKER_ELEMENT_INJECTION_KEY declaration with a unique description string to make the symbols distinguishable in logs and devtools; ensure you only change the description text and keep the InjectionKey types (advancedMarkerElement and marker ShallowRef types) intact.src/runtime/components/GoogleMaps/useGoogleMapsResource.ts (1)
57-63: Consider wrapping cleanup in try-finally.If
cleanupthrows an exception,resource.valuewould not be set toundefined, potentially preventing garbage collection. Wrapping in try-finally ensures the ref is always nulled.🛡️ Proposed improvement
onUnmounted(() => { isUnmounted.value = true - if (resource.value && cleanup && mapContext?.mapsApi.value) { - cleanup(resource.value, { mapsApi: mapContext.mapsApi.value }) + try { + if (resource.value && cleanup && mapContext?.mapsApi.value) { + cleanup(resource.value, { mapsApi: mapContext.mapsApi.value }) + } + } + finally { + resource.value = undefined } - resource.value = undefined })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/GoogleMaps/useGoogleMapsResource.ts` around lines 57 - 63, In onUnmounted callback, ensure resource.value is always cleared even if cleanup throws: wrap the cleanup(resource.value, { mapsApi: mapContext.mapsApi.value }) call in a try-finally block so that resource.value = undefined executes in the finally; keep the existing isUnmounted update and the conditional (resource.value && cleanup && mapContext?.mapsApi.value) but perform cleanup inside try and nulling of resource.value in finally to guarantee GC-safe teardown.test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts (1)
33-37: Consider adding a comment explaining the magic number.The
flushAsync(ticks = 4)helper uses a default of 4 ticks. Adding a brief comment explaining why 4 ticks are typically needed (e.g., for async resource creation to complete) would improve maintainability.📝 Suggested improvement
+// Multiple ticks needed: watcher trigger → async create → promise resolve → ref assignment async function flushAsync(ticks = 4) { for (let i = 0; i < ticks; i++) { await nextTick() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts` around lines 33 - 37, The helper function flushAsync uses a default magic number of 4 ticks; add a brief inline comment above flushAsync explaining why 4 ticks is chosen (e.g., to allow multiple microtask/macrotask cycles for async resource/component creation and event propagation to complete when testing lifecycle), referencing the function name flushAsync and its use of nextTick so future readers understand the required delay and can adjust the default if test timing changes.src/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vue (1)
54-56: InconsistentnoDrawflag usage compared to regular Marker.In
ScriptGoogleMapsMarker.vue(line 54-55),addMarkeris called withnoDraw: truefollowed byrequestRerender()for batch efficiency. Here,addMarkeris called without the flag. Consider aligning the behavior for consistency and potential performance benefits with many markers.♻️ Suggested alignment with ScriptGoogleMapsMarker
if (markerClustererContext?.markerClusterer.value) { - markerClustererContext.markerClusterer.value.addMarker(marker) + markerClustererContext.markerClusterer.value.addMarker(marker, true) + markerClustererContext.requestRerender() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vue` around lines 54 - 56, The marker is added to the clusterer without the noDraw optimization; change the call on markerClustererContext.markerClusterer.value.addMarker to pass the noDraw flag (e.g., addMarker(marker, { noDraw: true })) and then invoke the marker clusterer's requestRerender() (markerClustererContext.markerClusterer.value.requestRerender()) to match the batching behavior used in ScriptGoogleMapsMarker.vue for performance consistency when adding many markers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vue`:
- Around line 62-70: Align the defensive checks in cleanup with create: update
the cleanup function (cleanup) to verify
markerClustererContext?.markerClusterer.value before calling
removeMarker(marker) so it mirrors the create() check; if that value is absent,
fall back to clearing marker.map as before. Ensure you reference
markerClustererContext, markerClusterer.value, removeMarker, and marker.map when
making the conditional so the else branch executes when the clusterer value is
undefined.
In `@src/runtime/components/GoogleMaps/ScriptGoogleMapsInfoWindow.vue`:
- Around line 41-67: The click listeners added to markerContext.marker.value and
advancedMarkerElementContext.advancedMarkerElement.value are never removed, so
update the resource to store the listener handles when calling addListener (or
store the parent marker reference) inside the function that creates the
InfoWindow (where iw is created) and then update cleanup(iw, { mapsApi }) to
remove those parent listeners using mapsApi.event.removeListener or
mapsApi.event.clearInstanceListeners on the stored marker/handle; adjust
references to infoWindow.value usage accordingly so the created listener handles
/ parent marker are accessible in cleanup (e.g., add a property on the resource
returned with markerListener or parentMarkerRef and remove it in cleanup).
In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts`:
- Line 39: The test suite title has a typo: the describe call uses 'scri
ptGoogleMapsAdvancedMarkerElement' with an extra space; update the describe
string to the correct identifier 'scriptGoogleMapsAdvancedMarkerElement' (i.e.,
remove the space) so the suite name is spelled correctly in the test for the
Google Maps lifecycle.
In `@test/unit/google-maps-lifecycle.test.ts`:
- Around line 1-5: Move the `@vitest-environment` directive to the very top of the
test file so Vitest recognizes it: place the line `/** `@vitest-environment`
happy-dom */` above all imports (i.e., before the `import { mount } from
'@vue/test-utils'` and other imports) in google-maps-lifecycle.test.ts so the
environment comment is the first thing in the file.
---
Nitpick comments:
In `@src/runtime/components/GoogleMaps/injectionKeys.ts`:
- Around line 8-14: The two injection keys currently both call Symbol('marker'),
which makes debugging harder; update ADVANCED_MARKER_ELEMENT_INJECTION_KEY to
use a distinct description (e.g., Symbol('advanced-marker-element') or similar)
and keep MARKER_INJECTION_KEY with a clear description (e.g., Symbol('marker')),
so replace the Symbol('marker') in the ADVANCED_MARKER_ELEMENT_INJECTION_KEY
declaration with a unique description string to make the symbols distinguishable
in logs and devtools; ensure you only change the description text and keep the
InjectionKey types (advancedMarkerElement and marker ShallowRef types) intact.
In `@src/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vue`:
- Around line 54-56: The marker is added to the clusterer without the noDraw
optimization; change the call on
markerClustererContext.markerClusterer.value.addMarker to pass the noDraw flag
(e.g., addMarker(marker, { noDraw: true })) and then invoke the marker
clusterer's requestRerender()
(markerClustererContext.markerClusterer.value.requestRerender()) to match the
batching behavior used in ScriptGoogleMapsMarker.vue for performance consistency
when adding many markers.
In `@src/runtime/components/GoogleMaps/useGoogleMapsResource.ts`:
- Around line 57-63: In onUnmounted callback, ensure resource.value is always
cleared even if cleanup throws: wrap the cleanup(resource.value, { mapsApi:
mapContext.mapsApi.value }) call in a try-finally block so that resource.value =
undefined executes in the finally; keep the existing isUnmounted update and the
conditional (resource.value && cleanup && mapContext?.mapsApi.value) but perform
cleanup inside try and nulling of resource.value in finally to guarantee GC-safe
teardown.
In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts`:
- Around line 33-37: The helper function flushAsync uses a default magic number
of 4 ticks; add a brief inline comment above flushAsync explaining why 4 ticks
is chosen (e.g., to allow multiple microtask/macrotask cycles for async
resource/component creation and event propagation to complete when testing
lifecycle), referencing the function name flushAsync and its use of nextTick so
future readers understand the required delay and can adjust the default if test
timing changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96dd3ed2-caf9-4efd-80c3-613c4b6f6ee5
📒 Files selected for processing (16)
src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsCircle.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsHeatmapLayer.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsInfoWindow.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsMarker.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsMarkerClusterer.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsPinElement.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsPolygon.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsPolyline.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsRectangle.vuesrc/runtime/components/GoogleMaps/injectionKeys.tssrc/runtime/components/GoogleMaps/useGoogleMapsResource.tstest/nuxt-runtime/google-maps-lifecycle.nuxt.test.tstest/unit/__mocks__/google-maps-api.tstest/unit/google-maps-lifecycle.test.ts
src/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vue
Show resolved
Hide resolved
Static .vue imports fail CI typecheck because the generated nuxt tsconfig has empty types array — google.maps types aren't included. Dynamic imports bypass the typecheck phase.
- Align clusterer cleanup check with create check in AdvancedMarkerElement - Track and remove parent marker click listener in InfoWindow cleanup - Fix typo in test describe block name
Use triple-slash reference directive to make google.maps ambient types available to the nuxt-runtime test file, fixing the CI typecheck that couldn't resolve .vue component module types.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/content/scripts/google-maps.md (1)
537-537: Clarify “basic Google Maps object” wording.“basic Google Maps object” is ambiguous; “underlying Google Maps object” is clearer and matches the implementation intent.
Suggested wording
-All SFC components accept an `options` prop matching their Google Maps API options type (excluding `map`, which is injected automatically). Options are reactive - changes update the basic Google Maps object. Components clean up automatically on unmount. +All SFC components accept an `options` prop matching their Google Maps API options type (excluding `map`, injected automatically). Options are reactive—changes update the underlying Google Maps object. Components clean up automatically on unmount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/scripts/google-maps.md` at line 537, Update the ambiguous phrase "basic Google Maps object" to "underlying Google Maps object" in the sentence describing SFC component behavior so it reads: "Options are reactive - changes update the underlying Google Maps object." Search for the exact phrase "basic Google Maps object" in the docs/content/scripts/google-maps.md and replace it (and any exact duplicates nearby) to ensure the wording matches implementation intent.test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts (1)
41-45: Don’t rely on an arbitrary 4×nextTick()flush here.These components create resources after awaited library imports. A fixed tick count couples the suite to the current scheduler depth and can make the lifecycle assertions flaky as soon as that async chain changes. Prefer flushing promises and then a Vue tick instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts` around lines 41 - 45, The current flushAsync implementation in the test (function flushAsync using a fixed 4× nextTick loop) is flaky; change it to first await promise microtasks to drain pending library imports and then await a single Vue tick. Concretely, replace the fixed loop in flushAsync with: await flushPromises() (import flushPromises from 'flush-promises' or use awaiting Promise.resolve() repeatedly until microtasks are drained) followed by await nextTick(); update any tests that call flushAsync to use this new behavior. Ensure you keep the function name flushAsync and the nextTick await so lifecycle assertions run after microtasks and one Vue render tick.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/scripts/google-maps.md`:
- Line 529: Update the hierarchy line so component names are consistently
prefixed: replace the mixed "ScriptGoogleMapsMarker / AdvancedMarkerElement"
with a prefixed form such as "ScriptGoogleMapsAdvancedMarkerElement /
AdvancedMarkerElement" so readers won't mistake it for a different component
(reference the component name ScriptGoogleMapsAdvancedMarkerElement and the
child AdvancedMarkerElement when making the change).
- Around line 526-535: The fenced hierarchy block starting with
"ScriptGoogleMaps (root)" is missing a language tag which triggers markdownlint
MD040; update the opening fence from ``` to ```text so the block becomes a text
code fence (keep the content and closing ``` unchanged) to satisfy the linter.
In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts`:
- Around line 580-611: The test currently unmounts the entire Provider (via
wrapper.unmount()) which destroys the marker instance and masks watcher leaks;
instead mount the Provider and render the Child conditionally with a v-if flag
(e.g., showChild ref) so you only unmount the
Child/ScriptGoogleMapsAdvancedMarkerElement, then mutate the same reactive
optionsRef object (e.g., change optionsRef.value.position.lat/lng) rather than
replacing it to trigger the watcher; after flipping showChild to false and
awaiting flushAsync(), change the optionsRef properties and assert the existing
marker instance did not get updated (verifying the watcher attached in the Child
was cleaned up). Ensure you reference Provider, Child, optionsRef,
ScriptGoogleMapsAdvancedMarkerElement, mountSuspended, and flushAsync when
locating the test code to modify.
---
Nitpick comments:
In `@docs/content/scripts/google-maps.md`:
- Line 537: Update the ambiguous phrase "basic Google Maps object" to
"underlying Google Maps object" in the sentence describing SFC component
behavior so it reads: "Options are reactive - changes update the underlying
Google Maps object." Search for the exact phrase "basic Google Maps object" in
the docs/content/scripts/google-maps.md and replace it (and any exact duplicates
nearby) to ensure the wording matches implementation intent.
In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts`:
- Around line 41-45: The current flushAsync implementation in the test (function
flushAsync using a fixed 4× nextTick loop) is flaky; change it to first await
promise microtasks to drain pending library imports and then await a single Vue
tick. Concretely, replace the fixed loop in flushAsync with: await
flushPromises() (import flushPromises from 'flush-promises' or use awaiting
Promise.resolve() repeatedly until microtasks are drained) followed by await
nextTick(); update any tests that call flushAsync to use this new behavior.
Ensure you keep the function name flushAsync and the nextTick await so lifecycle
assertions run after microtasks and one Vue render tick.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f05daa3-02af-48c4-9fee-3ddca340926d
📒 Files selected for processing (2)
docs/content/scripts/google-maps.mdtest/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts
- Improve options watcher test to unmount only child (not whole tree) - Move @vitest-environment directive to top of file - Add language tag to docs hierarchy code block - Use full component names in hierarchy diagram
The nuxt-runtime vitest project's generated tsconfig cannot resolve .vue module imports, causing TypeCheckError for component imports. Runtime tests pass — typecheck is handled by the dedicated typecheck project.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/content/scripts/google-maps.md (1)
541-544: Consider noting the actual prop type in the Options Type column.The actual component props use
Omit<google.maps.MarkerOptions, 'map'>andOmit<google.maps.marker.AdvancedMarkerElementOptions, 'map'>(seeScriptGoogleMapsMarker.vue:11andScriptGoogleMapsAdvancedMarkerElement.vue:12), but the table shows the base types. While line 537 does clarify thatmapis excluded, explicitly showingOmit<..., 'map'>in the table would be more precise for developers checking type compatibility.📖 Suggested table update
-| `ScriptGoogleMapsMarker` | `google.maps.MarkerOptions` | Classic marker | -| `ScriptGoogleMapsAdvancedMarkerElement` | `google.maps.marker.AdvancedMarkerElementOptions` | Recommended | +| `ScriptGoogleMapsMarker` | `Omit<google.maps.MarkerOptions, 'map'>` | Classic marker | +| `ScriptGoogleMapsAdvancedMarkerElement` | `Omit<google.maps.marker.AdvancedMarkerElementOptions, 'map'>` | Recommended |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/scripts/google-maps.md` around lines 541 - 544, Update the Options Type column to reflect the actual prop types used by the components: change the entries for ScriptGoogleMapsMarker and ScriptGoogleMapsAdvancedMarkerElement to use Omit types (Omit<google.maps.MarkerOptions, 'map'> and Omit<google.maps.marker.AdvancedMarkerElementOptions, 'map'> respectively) so the table matches the prop definitions in ScriptGoogleMapsMarker (uses Omit<google.maps.MarkerOptions, 'map'>) and ScriptGoogleMapsAdvancedMarkerElement (uses Omit<google.maps.marker.AdvancedMarkerElementOptions, 'map'>) and makes the exclusion of 'map' explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts`:
- Around line 6-13: Direct imports of .vue components cause TypeScript errors in
CI; replace the static imports of ScriptGoogleMapsAdvancedMarkerElement,
ScriptGoogleMapsCircle, ScriptGoogleMapsHeatmapLayer, ScriptGoogleMapsMarker,
ScriptGoogleMapsPinElement, ScriptGoogleMapsPolygon, ScriptGoogleMapsPolyline,
and ScriptGoogleMapsRectangle with dynamic imports wrapped in
Vue.defineAsyncComponent (i.e. import them via () => import('...') and pass to
defineAsyncComponent) and add defineAsyncComponent to the Vue import statement
so tests use async component resolution and avoid the missing type declarations
error.
---
Nitpick comments:
In `@docs/content/scripts/google-maps.md`:
- Around line 541-544: Update the Options Type column to reflect the actual prop
types used by the components: change the entries for ScriptGoogleMapsMarker and
ScriptGoogleMapsAdvancedMarkerElement to use Omit types
(Omit<google.maps.MarkerOptions, 'map'> and
Omit<google.maps.marker.AdvancedMarkerElementOptions, 'map'> respectively) so
the table matches the prop definitions in ScriptGoogleMapsMarker (uses
Omit<google.maps.MarkerOptions, 'map'>) and
ScriptGoogleMapsAdvancedMarkerElement (uses
Omit<google.maps.marker.AdvancedMarkerElementOptions, 'map'>) and makes the
exclusion of 'map' explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3501a7f9-4577-46dc-bdfc-50df5f084466
📒 Files selected for processing (5)
docs/content/scripts/google-maps.mdsrc/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vuesrc/runtime/components/GoogleMaps/ScriptGoogleMapsInfoWindow.vuetest/nuxt-runtime/google-maps-lifecycle.nuxt.test.tstest/unit/google-maps-lifecycle.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/unit/google-maps-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/components/GoogleMaps/ScriptGoogleMapsAdvancedMarkerElement.vue
The root tsconfig included test/nuxt-runtime in its scope, causing vue-tsc to attempt resolving .vue module imports during the typecheck vitest project. Excluding it matches the existing pattern for test/unit. Also moved SFC component tests back to composable-only tests in the nuxt-runtime file (component tests stay in the unit file where @vue/test-utils handles .vue parsing).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts (1)
196-241: Consider consolidating with the earlier race condition test.This test exercises the same code path as
'should handle async resource creation with unmount race condition'(lines 75-115) — both verify that resources created during an async gap after unmount are immediately cleaned up. The only difference is the framing ("import" delay vs "create" delay).If the intent is to document multiple real-world scenarios, the current approach is valid. Otherwise, these could be consolidated into a single parameterized test to reduce redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts` around lines 196 - 241, The two tests 'should cleanup resources created during async gap after unmount' and 'should handle async resource creation with unmount race condition' exercise the same code path and should be consolidated to avoid redundancy; refactor the tests into one parameterized case (or remove the duplicate) that covers both "import" delay and "create" delay scenarios by reusing the same test logic around createMapProvider/Provider, the Child component using useGoogleMapsResource, and the cleanupFn assertion, or convert them into a single test with a parameter controlling whether the delay is introduced in the importPromise or inside create to verify cleanup behavior in both real-world variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/nuxt-runtime/google-maps-lifecycle.nuxt.test.ts`:
- Around line 196-241: The two tests 'should cleanup resources created during
async gap after unmount' and 'should handle async resource creation with unmount
race condition' exercise the same code path and should be consolidated to avoid
redundancy; refactor the tests into one parameterized case (or remove the
duplicate) that covers both "import" delay and "create" delay scenarios by
reusing the same test logic around createMapProvider/Provider, the Child
component using useGoogleMapsResource, and the cleanupFn assertion, or convert
them into a single test with a parameter controlling whether the delay is
introduced in the importPromise or inside create to verify cleanup behavior in
both real-world variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56d251d1-02eb-4420-9394-579c1fb1f57d
📒 Files selected for processing (2)
test/nuxt-runtime/google-maps-lifecycle.nuxt.test.tstsconfig.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Closes #646
Type of change
Description
The previous fix (#649) only handled one race condition (unmounting during
await importLibrary). The reporter confirmed the leak persisted in beta.25. The root cause was orphaned Vue watchers: every Google Maps sub-component created a nestedwhenever/watchinside an async callback. In Vue 3, watchers created after anawaitlose component instance context, so they're never auto-stopped on unmount — retaining the entire reactive scope indefinitely.Changes:
useGoogleMapsResourcecomposable encoding lifecycle safety (unmount guards, post-unmount cleanup, automatic ref nulling)ScriptGoogleMapscleanup made synchronous (Vue doesn't await async hooks), caches cleared on unmountletvariables →shallowReffor proper GCinjectionKeys.ts(re-exported for backwards compat)27 new tests covering composable lifecycle, actual SFC components, v-for list changes (#646 scenario), v-if toggling, deferred map readiness, options reactivity after unmount, nested trees, and mount/unmount cycles.