refactor(google-maps): export typed interfaces, internal cleanups#692
refactor(google-maps): export typed interfaces, internal cleanups#692
Conversation
Splits the safe, additive changes out of #689 so they can land independently of the larger breaking refactor. - Export `ScriptGoogleMapsProps`/`Emits`/`Slots`/`Expose` and the matching `ScriptGoogleMapsOverlayView*` interfaces so consumers can type their template refs and event handlers. - Modernise internal vars: `useTemplateRef` for `rootEl`/`mapEl`, `mapsApi` switched to `shallowRef`, internal `ready` ref renamed to `isMapReady` for clearer intent. - Tidy `resolveQueryToLatLng` so the cache always returns a consistent `LatLngLiteral` shape. - Collapse the color-mode logic into a single computed. - Use `instanceof google.maps.LatLng` to normalise marker positions in the overlay component. - Drop the `styled.vue` playground page; it relied on the deprecated `styles` map option which conflicts with `mapId`. No public API changes — `center`/`zoom` props, `googleMaps` expose, `ready` event payload, location-query auto-resolution, and the `setCenter` skip-if-equal optimisation are all preserved.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
The rule misfires on SFCs that use two <script> blocks (one for exported types, one for setup): it concatenates the blocks and flags the setup-block imports as appearing after the exports.
📝 WalkthroughWalkthroughAdds an ESLint config block disabling Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
eslint.config.mjs (1)
57-65: Scope theimport/firstoverride more narrowly.This disables the rule for every Vue SFC, including the single-script ones that are not affected by the dual-
<script>parsing issue. Prefer targeting just the affected Google Maps components, or using file-local disables there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.mjs` around lines 57 - 65, The current override blanket-disables the 'import/first' rule for all Vue SFCs (files: ['**/*.vue']); narrow this to only the affected Google Maps components by changing the files glob to the specific component paths (e.g. files: ['components/google-maps/**', 'src/components/GoogleMap*.vue'] or the exact filenames) or remove the global override entirely and add file-local /* eslint-disable import/first */ comments in the problematic SFCs; update the override entry that references 'import/first' so it targets only those component files instead of '**/*.vue'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 223-227: The cache check currently uses "instanceof
google.maps.LatLng" which throws if the Google API isn't loaded; instead, change
the branch that reads from queryToLatLngCache (symbol: queryToLatLngCache and
variable: cached) to detect a LatLng-like object without referencing the global
google (e.g., check if cached.lat is a function to call
cached.lat()/cached.lng(), otherwise treat it as a plain object with numeric
lat/lng properties). Update the conditional so it never accesses google or
google.maps.LatLng and returns { lat: ..., lng: ... } based on the presence/type
of cached.lat.
- Around line 246-255: In resolveQueryToLatLng(), the Promise waits on a
non-immediate watcher of mapsApi which can hang if load() sets mapsApi before
the watcher is created; fix by re-checking mapsApi.value immediately after await
load() and resolving if set, or replace the watcher with watch(mapsApi,
callback, { immediate: true }) (and ensure the watcher is properly torn down) so
the promise always resolves when mapsApi is already populated or changes.
---
Nitpick comments:
In `@eslint.config.mjs`:
- Around line 57-65: The current override blanket-disables the 'import/first'
rule for all Vue SFCs (files: ['**/*.vue']); narrow this to only the affected
Google Maps components by changing the files glob to the specific component
paths (e.g. files: ['components/google-maps/**',
'src/components/GoogleMap*.vue'] or the exact filenames) or remove the global
override entirely and add file-local /* eslint-disable import/first */ comments
in the problematic SFCs; update the override entry that references
'import/first' so it targets only those component files instead of '**/*.vue'.
🪄 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: 5b1bba14-bd88-4e8c-a6b6-da45047c9c34
📒 Files selected for processing (5)
eslint.config.mjspackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuepackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vueplayground/pages/index.vueplayground/pages/third-parties/google-maps/styled.vue
💤 Files with no reviewable changes (2)
- playground/pages/index.vue
- playground/pages/third-parties/google-maps/styled.vue
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue
Outdated
Show resolved
Hide resolved
| if (!mapsApi.value) { | ||
| await load() | ||
| // await new promise, watch until mapsApi is set | ||
| await new Promise<void>((resolve) => { | ||
| const _ = watch(mapsApi, () => { | ||
| _() | ||
| resolve() | ||
| }) | ||
| } | ||
| const placesService = new mapsApi.value!.places.PlacesService(map.value!) | ||
| placesService.findPlaceFromQuery({ | ||
| query, | ||
| fields: ['name', 'geometry'], | ||
| }, (results, status) => { | ||
| if (status === 'OK' && results?.[0]?.geometry?.location) | ||
| return resolve(results[0].geometry.location) | ||
| return reject(new Error(`No location found for ${query}`)) | ||
| }) | ||
| }).then((res) => { | ||
| queryToLatLngCache.set(query, res) | ||
| return res | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and locate the file
fd ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 126
🏁 Script executed:
# Read the file to understand the context around lines 246-255
wc -l packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 130
🏁 Script executed:
# Get the code context around the problematic lines
cat -n packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | sed -n '230,270p'Repository: nuxt/scripts
Length of output: 1823
🏁 Script executed:
# Find the load() function definition
rg -A 20 "^\s*(?:async\s+)?load\s*\(" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 38
🏁 Script executed:
# Search for resolveQueryToLatLng to understand its usage
rg -B 5 -A 10 "resolveQueryToLatLng" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 3155
🏁 Script executed:
# Search for mapsApi ref definition and where it gets assigned
rg -B 2 -A 2 "mapsApi\s*=" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | head -40Repository: nuxt/scripts
Length of output: 144
🏁 Script executed:
# Search for load() function definition
rg -B 5 -A 20 "const load\s*=|function load|async load" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 38
🏁 Script executed:
# Search for where mapsApi.value is assigned
rg "mapsApi\.value\s*=" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 95
🏁 Script executed:
# Get more context around the mapsApi initialization
cat -n packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | sed -n '1,100p'Repository: nuxt/scripts
Length of output: 3997
🏁 Script executed:
# Search for status variable that's used in proposed fix
rg "status\s*=" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | head -20Repository: nuxt/scripts
Length of output: 318
🏁 Script executed:
# Search for useScript usage which likely defines load()
rg "useScript|from.*script" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 494
🏁 Script executed:
# Get more context around the mapsApi.value assignment
rg -B 30 "mapsApi\.value\s*=" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 1193
🏁 Script executed:
# Get more lines around the beginning to find setup
cat -n packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | sed -n '100,200p'Repository: nuxt/scripts
Length of output: 4822
🏁 Script executed:
# Search for status ref definition
rg "const status|status\s*=\s*ref|status\s*=\s*shallowRef" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueRepository: nuxt/scripts
Length of output: 38
🏁 Script executed:
# Get more context around the onLoaded callback
cat -n packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | sed -n '375,395p'Repository: nuxt/scripts
Length of output: 907
🏁 Script executed:
# Look for useScriptGoogleMaps implementation
fd useScriptGoogleMapsRepository: nuxt/scripts
Length of output: 38
🏁 Script executed:
# Get the full context of the problematic code and surrounding logic
cat -n packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | sed -n '240,290p'Repository: nuxt/scripts
Length of output: 2016
🏁 Script executed:
# Let's verify the exact timing by checking if there's synchronous behavior in useScriptGoogleMaps
fd useScriptGoogleMaps -type fRepository: nuxt/scripts
Length of output: 226
🏁 Script executed:
# Check if there's documentation or if load() might complete before watcher registration
cat packages/script/src/runtime/composables/useScriptGoogleMaps.ts | head -100Repository: nuxt/scripts
Length of output: 150
🏁 Script executed:
# Verify the watch behavior - is it really non-immediate and could miss value changes?
rg "const mapsApi" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue -A 5Repository: nuxt/scripts
Length of output: 528
🏁 Script executed:
# Check if there are any other similar patterns with immediate watchers in the codebase
rg "watch\(" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | grep -c immediateRepository: nuxt/scripts
Length of output: 57
🏁 Script executed:
# Find the useScriptGoogleMaps file
fd useScriptGoogleMapsRepository: nuxt/scripts
Length of output: 38
🏁 Script executed:
# Search for the google-maps registry file
fd "google-maps" --type f | head -20Repository: nuxt/scripts
Length of output: 656
🏁 Script executed:
# Look for where useScriptGoogleMaps is defined
rg "export.*useScriptGoogleMaps|function useScriptGoogleMaps" packages/Repository: nuxt/scripts
Length of output: 198
🏁 Script executed:
# Get the full implementation of useScriptGoogleMaps
cat packages/script/src/runtime/registry/google-maps.tsRepository: nuxt/scripts
Length of output: 2136
🏁 Script executed:
# Let's verify the timing issue by checking if the watcher could miss the value change
# Check the full context where resolveQueryToLatLng is defined and see if status is in scope
cat -n packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | sed -n '190,230p'Repository: nuxt/scripts
Length of output: 1687
🏁 Script executed:
# Verify if status variable is accessible within the resolveQueryToLatLng function scope
rg -B 50 "async function resolveQueryToLatLng" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue | grep -E "status|const \{" | head -20Repository: nuxt/scripts
Length of output: 112
Close the load() / watcher race condition in resolveQueryToLatLng().
If load() completes and triggers onLoaded before the watcher at line 250 is created, mapsApi.value will already be set. Since the watcher is non-immediate, it only fires on value changes, not on existing values—causing it to never fire and the Promise to hang indefinitely.
Recheck mapsApi.value after await load(), or use an immediate watcher with error handling via the status variable:
Suggested fix
if (!mapsApi.value) {
await load()
- // await new promise, watch until mapsApi is set
- await new Promise<void>((resolve) => {
- const _ = watch(mapsApi, () => {
- _()
- resolve()
- })
- })
+ if (!mapsApi.value) {
+ await new Promise<void>((resolve, reject) => {
+ const stop = watch([mapsApi, status], ([api, nextStatus]) => {
+ if (api) {
+ stop()
+ resolve()
+ }
+ else if (nextStatus === 'error') {
+ stop()
+ reject(new Error('Failed to load Google Maps'))
+ }
+ }, { immediate: true })
+ })
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!mapsApi.value) { | |
| await load() | |
| // await new promise, watch until mapsApi is set | |
| await new Promise<void>((resolve) => { | |
| const _ = watch(mapsApi, () => { | |
| _() | |
| resolve() | |
| }) | |
| } | |
| const placesService = new mapsApi.value!.places.PlacesService(map.value!) | |
| placesService.findPlaceFromQuery({ | |
| query, | |
| fields: ['name', 'geometry'], | |
| }, (results, status) => { | |
| if (status === 'OK' && results?.[0]?.geometry?.location) | |
| return resolve(results[0].geometry.location) | |
| return reject(new Error(`No location found for ${query}`)) | |
| }) | |
| }).then((res) => { | |
| queryToLatLngCache.set(query, res) | |
| return res | |
| } | |
| if (!mapsApi.value) { | |
| await load() | |
| if (!mapsApi.value) { | |
| await new Promise<void>((resolve, reject) => { | |
| const stop = watch([mapsApi, status], ([api, nextStatus]) => { | |
| if (api) { | |
| stop() | |
| resolve() | |
| } | |
| else if (nextStatus === 'error') { | |
| stop() | |
| reject(new Error('Failed to load Google Maps')) | |
| } | |
| }, { immediate: true }) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`
around lines 246 - 255, In resolveQueryToLatLng(), the Promise waits on a
non-immediate watcher of mapsApi which can hang if load() sets mapsApi before
the watcher is created; fix by re-checking mapsApi.value immediately after await
load() and resolving if set, or replace the watcher with watch(mapsApi,
callback, { immediate: true }) (and ensure the watcher is properly torn down) so
the promise always resolves when mapsApi is already populated or changes.
There was a problem hiding this comment.
Pre-existing on main — the watcher race in resolveQueryToLatLng is identical to the version this PR replaces (same non-immediate watch(mapsApi, ...) pattern). Out of scope for this internal-cleanup PR; will get its own fix(google-maps): close races in resolveQueryToLatLng follow-up alongside the map.value guard you flagged below.
There was a problem hiding this comment.
@harlan-zw, understood — the non-immediate watch(mapsApi, ...) pattern is identical to what was on main before this PR, so it's not a regression introduced here. I'll treat this as pre-existing and out of scope for this cleanup PR.
Would you like me to open a GitHub issue to track the fix(google-maps): close races in resolveQueryToLatLng follow-up (covering both the mapsApi watcher race and the map.value guard)?
Reverts the cache-hit normalisation introduced in the previous commit. The original code stored whatever shape the resolver path returned (LatLng for Places, LatLngLiteral for proxy) and the cache-hit path returned the same shape — internally consistent per path. The normalisation only touched cache hits, which would have meant the same query returns LatLng on first call but LatLngLiteral on second call when using the Places fallback. If we want consistent shapes across both paths, that's a separate intentional change that should land with a dedicated test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue (1)
458-465: Minor: loading and error slots can render simultaneously.When
status === 'error'and!isMapReady, both the loading slot (line 460) and error slot (line 464) will render. The loading condition doesn't exclude the error state.This appears to be existing behavior preserved by the
ready→isMapReadyrename, but worth noting for a future cleanup.Potential fix
- <slot v-if="status !== 'awaitingLoad' && !isMapReady" name="loading"> + <slot v-if="status === 'loading' && !isMapReady" name="loading">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` around lines 458 - 465, The loading and error slots can both render when status === 'error' and !isMapReady; update the template logic around the loading slot in ScriptGoogleMaps.vue so the loading slot excludes the error state (e.g., change the loading slot's condition to also require status !== 'error' or convert the slot conditions into an if / else-if chain) so only one of loading or error renders; target the nodes using isMapReady and status to implement the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Line 256: The PlacesService is created with new
mapsApi.value!.places.PlacesService(map.value!) which can run before map.value
is set, causing a null reference when resolveQueryToLatLng is called; change to
lazily create or defer creating placesService until the map is initialized
(e.g., move instantiation into resolveQueryToLatLng or check map.value and await
the onLoaded/map-ready promise before calling new
mapsApi.value!.places.PlacesService), and add a defensive guard in
resolveQueryToLatLng to wait for or throw a clear error if map.value is not yet
available; reference symbols: placesService creation line, resolveQueryToLatLng,
mapsApi.value, map.value, onLoaded.
---
Nitpick comments:
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 458-465: The loading and error slots can both render when status
=== 'error' and !isMapReady; update the template logic around the loading slot
in ScriptGoogleMaps.vue so the loading slot excludes the error state (e.g.,
change the loading slot's condition to also require status !== 'error' or
convert the slot conditions into an if / else-if chain) so only one of loading
or error renders; target the nodes using isMapReady and status to implement the
fix.
🪄 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: 4b85e5cb-e020-4dab-bf88-f33aa5bf310a
📒 Files selected for processing (1)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue
| return res | ||
| } | ||
|
|
||
| const placesService = new mapsApi.value!.places.PlacesService(map.value!) |
There was a problem hiding this comment.
Potential null reference: map.value may be undefined when PlacesService is instantiated.
resolveQueryToLatLng is exposed publicly and can be called before the map is initialized. After mapsApi.value becomes available, there's a window before map.value is assigned in onLoaded (line 384). Calling this function during that window will throw because PlacesService requires a valid map/element.
Suggested fix: wait for map initialization
- const placesService = new mapsApi.value!.places.PlacesService(map.value!)
+ // Wait for map to be initialized if not already
+ if (!map.value) {
+ await new Promise<void>((resolve) => {
+ const stop = watch(map, (m) => {
+ if (m) {
+ stop()
+ resolve()
+ }
+ }, { immediate: true })
+ })
+ }
+ const placesService = new mapsApi.value!.places.PlacesService(map.value!)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const placesService = new mapsApi.value!.places.PlacesService(map.value!) | |
| // Wait for map to be initialized if not already | |
| if (!map.value) { | |
| await new Promise<void>((resolve) => { | |
| const stop = watch(map, (m) => { | |
| if (m) { | |
| stop() | |
| resolve() | |
| } | |
| }, { immediate: true }) | |
| }) | |
| } | |
| const placesService = new mapsApi.value!.places.PlacesService(map.value!) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` at
line 256, The PlacesService is created with new
mapsApi.value!.places.PlacesService(map.value!) which can run before map.value
is set, causing a null reference when resolveQueryToLatLng is called; change to
lazily create or defer creating placesService until the map is initialized
(e.g., move instantiation into resolveQueryToLatLng or check map.value and await
the onLoaded/map-ready promise before calling new
mapsApi.value!.places.PlacesService), and add a defensive guard in
resolveQueryToLatLng to wait for or throw a clear error if map.value is not yet
available; reference symbols: placesService creation line, resolveQueryToLatLng,
mapsApi.value, map.value, onLoaded.
There was a problem hiding this comment.
Pre-existing on main — same new mapsApi.value!.places.PlacesService(map.value!) line existed before this refactor. Real bug, just out of scope for this PR. Tracking it together with the watcher race above for a dedicated fix PR.
There was a problem hiding this comment.
@harlan-zw, understood — thanks for the context. The new mapsApi.value!.places.PlacesService(map.value!) null-ref issue predates this PR and is tracked for a dedicated fix PR alongside the watcher race.
Would you like me to open a GitHub issue to track both of these (the map.value null-ref in PlacesService instantiation and the watcher race in resolveQueryToLatLng) so they don't get lost?
1 participant
🔗 Linked issue
Splits the safe, additive portion out of #689 so it can land first while the breaking changes get their own targeted PRs.
❓ Type of change
📚 Description
#689 bundles a lot of unrelated work into one diff. This PR extracts the changes that have no public API impact so reviewing the breaking pieces becomes simpler.
ScriptGoogleMapsScriptGoogleMapsProps,…Emits,…Slots,…Exposefrom a non-setup script block so consumers can type their template refs and event handlers.useTemplateRefforrootEl/mapEl(latest Vue API).mapsApiswitched fromreftoshallowRef(correct for non-deeply-reactive data).readyref renamed toisMapReadyfor clearer intent.resolveQueryToLatLngcache normalisation: callers now always get a consistentLatLngLiteralshape on cache hits.ScriptGoogleMapsOverlayViewScriptGoogleMapsOverlayAnchor,ScriptGoogleMapsOverlayPane, and the…OverlayViewProps/Emits/Slots/Exposeinterfaces.instanceof google.maps.LatLnginstead of duck typing.defineEmits<…>()declaration.Playground
playground/pages/third-parties/google-maps/styled.vue. It relied on the deprecatedstylesmap option which silently conflicts withmapId, and isn't a useful demo on the v1 component.Out of scope (deferred to follow-up PRs)
These will be split off as separate PRs from #689 with proper deprecations, docs and tests:
center/zoomprops in favour ofmapOptions.googleMapsexpose →mapsApi(with deprecation shim).What this preserves
No behavioural or API changes, including:
center/zoomprops still work as top-level propsgoogleMapsexpose key unchangedreadyevent payload shape unchangedcenterstill workssetCenterskip-if-equal optimisation (from 6fdd533 / 0e4ae9e) preserveddefineModel('open')andoverlayexpose unchanged