feat(maps): add Leaflet and MapLibre integrations#833
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
commit: |
|
CI note: both test jobs now fail only the two existing jQuery bundling snapshots in |
|
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:
📝 WalkthroughWalkthroughAdds Leaflet and MapLibre GL JS integrations to Nuxt Scripts with lazy loading, bundled styling, typed registry metadata, declarative map components, resource lifecycle management, tests, licensing, documentation, and playground examples. Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (2)
packages/script/src/runtime/components/Leaflet/useLeafletResource.ts (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
bindLeafletEvents'sany-typedemitforces unsafe casts at every call site. The shared helper's signature(target, emit: (event: any, ...args: any[]) => void, events)discards the type safety of each component'sdefineEmits, which is why every caller must castemit as any.
packages/script/src/runtime/components/Leaflet/useLeafletResource.ts#L23-L30: makebindLeafletEventsgeneric over an event-map type (e.g.<E extends Record<string, any[]>>(target, emit: <K extends keyof E & string>(event: K, ...args: E[K]) => void, events: readonly (keyof E & string)[])) so it can accept a component's actual emit function without casting.packages/script/src/runtime/components/Leaflet/ScriptLeafletTileLayer.vue#L28-L28: drop theemit as anycast once the helper is generic.packages/script/src/runtime/components/Leaflet/ScriptLeafletMarker.vue#L48-L48: drop theemit as anycast once the helper is generic.packages/script/src/runtime/components/Leaflet/ScriptLeafletGeoJson.vue#L30-L30: drop theemit as anycast once the helper is generic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/script/src/runtime/components/Leaflet/useLeafletResource.ts` around lines 23 - 30, Make bindLeafletEvents generic over an event-map type so its emit callback and events list use the component’s typed event names and argument tuples instead of any. In packages/script/src/runtime/components/Leaflet/useLeafletResource.ts:23-30, update bindLeafletEvents accordingly; in packages/script/src/runtime/components/Leaflet/ScriptLeafletTileLayer.vue:28, packages/script/src/runtime/components/Leaflet/ScriptLeafletMarker.vue:48, and packages/script/src/runtime/components/Leaflet/ScriptLeafletGeoJson.vue:30, remove the emit as any casts and pass each component’s typed emit directly.packages/script/src/runtime/components/Leaflet/ScriptLeafletMap.vue (1)
143-148: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueVerify whether the underlying script loader exposes richer error detail than this generic message.
watch(status, ...)fabricates a fresh, genericError('Leaflet failed to load')rather than surfacing whateveruseScriptLeaflet/useScriptactually reports on failure. Unhead'suseScript()exposes a dedicatedonErrorcallback for this purpose — worth confirming whether using it here would let you forward a more specific/original error instead of overwriting it, since the underlying composable's exact error signature isn't visible in this file set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/script/src/runtime/components/Leaflet/ScriptLeafletMap.vue` around lines 143 - 148, Update the Leaflet script error handling around useScriptLeaflet and the status watcher to use the loader’s onError callback or exposed error value when available, forwarding the original/richer error through loadError and emit('error'). Avoid replacing the underlying failure with the generic Error('Leaflet failed to load'); preserve a fallback only when no loader error detail is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/script/src/runtime/components/Leaflet/ScriptLeafletMarker.vue`:
- Line 24: Update the dragstart event type in the Leaflet marker event
definitions to use LeafletEvent instead of Leaflet.DragEndEvent. Keep
Leaflet.DragEndEvent for dragend, where distance is available.
---
Nitpick comments:
In `@packages/script/src/runtime/components/Leaflet/ScriptLeafletMap.vue`:
- Around line 143-148: Update the Leaflet script error handling around
useScriptLeaflet and the status watcher to use the loader’s onError callback or
exposed error value when available, forwarding the original/richer error through
loadError and emit('error'). Avoid replacing the underlying failure with the
generic Error('Leaflet failed to load'); preserve a fallback only when no loader
error detail is provided.
In `@packages/script/src/runtime/components/Leaflet/useLeafletResource.ts`:
- Around line 23-30: Make bindLeafletEvents generic over an event-map type so
its emit callback and events list use the component’s typed event names and
argument tuples instead of any. In
packages/script/src/runtime/components/Leaflet/useLeafletResource.ts:23-30,
update bindLeafletEvents accordingly; in
packages/script/src/runtime/components/Leaflet/ScriptLeafletTileLayer.vue:28,
packages/script/src/runtime/components/Leaflet/ScriptLeafletMarker.vue:48, and
packages/script/src/runtime/components/Leaflet/ScriptLeafletGeoJson.vue:30,
remove the emit as any casts and pass each component’s typed emit directly.
🪄 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: 8728dada-c052-428e-8a69-59f1c7941cab
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
docs/content/scripts/leaflet/2.api/1.script-leaflet-map.mddocs/content/scripts/leaflet/2.api/2.tile-layer.mddocs/content/scripts/leaflet/2.api/3.marker.mddocs/content/scripts/leaflet/2.api/4.popup.mddocs/content/scripts/leaflet/2.api/5.geojson.mddocs/content/scripts/leaflet/2.api/6.use-script-leaflet.mddocs/content/scripts/leaflet/index.mdpackage.jsonpackages/script/THIRD_PARTY_LICENSES.mdpackages/script/package.jsonpackages/script/src/registry-logos.tspackages/script/src/registry-types.jsonpackages/script/src/registry.tspackages/script/src/runtime/components/Leaflet/ScriptLeafletGeoJson.vuepackages/script/src/runtime/components/Leaflet/ScriptLeafletMap.vuepackages/script/src/runtime/components/Leaflet/ScriptLeafletMarker.vuepackages/script/src/runtime/components/Leaflet/ScriptLeafletPopup.vuepackages/script/src/runtime/components/Leaflet/ScriptLeafletTileLayer.vuepackages/script/src/runtime/components/Leaflet/useLeafletResource.tspackages/script/src/runtime/leaflet-styles.tspackages/script/src/runtime/registry/leaflet.tspackages/script/src/runtime/registry/schemas.tspackages/script/src/runtime/types.tspackages/script/src/script-meta.tsplayground/nuxt.config.tsplayground/pages/index.vueplayground/pages/third-parties/leaflet.vuepnpm-workspace.yamlscripts/generate-registry-types.tstest/nuxt-runtime/leaflet-components.nuxt.test.tstest/nuxt-runtime/leaflet-map.nuxt.test.tstest/types/types.test-d.tstest/unit/leaflet-lifecycle.test.tstest/unit/leaflet-registry.test.tstest/unit/leaflet-styles.test.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
harlan-zw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/script/src/runtime/components/MapLibre/ScriptMapLibrePopup.vue (1)
83-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReactive
optionsupdates only take effect formaxWidth/offset; other fields are silently ignored.
Object.assign(popup.value.options, options)(Line 86) merges into the popup's internal options object, but onlymaxWidthandoffsetare re-applied via their public setters afterward. Fields likeclassName,closeButton,closeOnClick, oranchorare mutated in the internal bag but have no setter invoked, so changing them reactively has no visible effect — inconsistent with the apparent full reactivity of theoptionsprop.Consider calling the relevant public methods for fields that have one (e.g.
addClassName/removeClassNameforclassName), or documenting that onlymaxWidth/offsetare supported for reactive updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/script/src/runtime/components/MapLibre/ScriptMapLibrePopup.vue` around lines 83 - 91, Update the options watcher in the MapLibre popup component so every supported reactive option is applied through its corresponding public popup API, rather than only mutating popup.value.options; specifically handle fields such as className, closeButton, closeOnClick, and anchor using their available methods, while preserving the existing maxWidth and offset updates. Do not silently claim full reactivity for fields that lack a public update API; document or explicitly limit those fields if they cannot be applied.scripts/generate-registry-types.ts (1)
382-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging when a
TSTypeReferencecan't be resolved.If
namedTypes.get(typeNode.typeName.name)misses (e.g., a future Leaflet/MapLibre-style component imports its Props/Events/Slots type from another file instead of declaring it in the SFC's own normal<script>block),resolveTypeNodesilently falls back to the unresolved reference, and downstream extraction quietly produces empty fields/events/slots with no error — the generated docs would silently regress with no signal to catch it in CI.♻️ Suggested defensive logging
const declaration = namedTypes.get(typeNode.typeName.name) - if (!declaration) + if (!declaration) { + console.warn(`[generate-registry-types] Could not resolve type "${typeNode.typeName.name}" in ${fileName}; declare it in the component's normal <script> block.`) return { node: typeNode, source: scriptSource } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate-registry-types.ts` around lines 382 - 393, Add defensive logging in resolveTypeNode when a TSTypeReference with an Identifier typeName has no matching declaration from namedTypes.get. Include the unresolved type name and enough context to identify the source, then preserve the existing fallback return so downstream behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/script/src/runtime/components/MapLibre/ScriptMapLibreGeoJson.vue`:
- Around line 34-89: Make sourceId changes reactive in the resource
synchronization flow: update the watcher around props.layers,
props.sourceOptions, and props.beforeId to include props.sourceId, ensuring the
previous source and owned layers are removed before recreating resources with
the new ID. Update removeOwnedResources or the surrounding sync lifecycle to
retain and remove the actually-created source ID during cleanup, including
unmount cleanup.
In `@packages/script/src/runtime/components/MapLibre/ScriptMapLibreMarker.vue`:
- Around line 82-93: Update the watcher for props.options to call setDraggable,
setRotation, setRotationAlignment, and setPitchAlignment only when their
corresponding option fields are defined, preserving existing marker state during
partial updates. Replace the direct element style opacity assignment with
marker.value.setOpacity(options.opacity, options.opacityWhenCovered), while
retaining the existing opacity guard.
In `@packages/script/src/runtime/maplibre-styles.ts`:
- Around line 7-23: Update ensureMapLibreStyles so deduplication compares the
requested stylesheetUrl with the URL already injected, rather than returning
solely when MAPLIBRE_STYLE_ID exists. Allow a different URL to create its own
stylesheet link while continuing to avoid duplicate injections of the same URL;
preserve the existing integrity and crossOrigin handling for the default URL.
In `@playground/pages/third-parties/maplibre.vue`:
- Line 5: Relax the center ref type used with ScriptMapLibreMap so it accepts
the component’s emitted MapLibre.LngLat camera-state object instead of only a
numeric tuple. Update both playground/pages/third-parties/maplibre.vue:5-5 and
docs/content/scripts/maplibre/index.md:46-46, using a compatible broad type such
as MapLibre.LngLatLike or any.
- Line 114: Add role="group" to the div with class "controls" in the map camera
controls markup so its existing aria-label is announced by screen readers.
---
Nitpick comments:
In `@packages/script/src/runtime/components/MapLibre/ScriptMapLibrePopup.vue`:
- Around line 83-91: Update the options watcher in the MapLibre popup component
so every supported reactive option is applied through its corresponding public
popup API, rather than only mutating popup.value.options; specifically handle
fields such as className, closeButton, closeOnClick, and anchor using their
available methods, while preserving the existing maxWidth and offset updates. Do
not silently claim full reactivity for fields that lack a public update API;
document or explicitly limit those fields if they cannot be applied.
In `@scripts/generate-registry-types.ts`:
- Around line 382-393: Add defensive logging in resolveTypeNode when a
TSTypeReference with an Identifier typeName has no matching declaration from
namedTypes.get. Include the unresolved type name and enough context to identify
the source, then preserve the existing fallback return so downstream behavior
remains unchanged.
🪄 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: 0d3fcdcf-6ef1-47c7-b77f-691380fde560
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
docs/content/scripts/maplibre/2.api/1.script-maplibre-map.mddocs/content/scripts/maplibre/2.api/2.marker.mddocs/content/scripts/maplibre/2.api/3.popup.mddocs/content/scripts/maplibre/2.api/4.navigation-control.mddocs/content/scripts/maplibre/2.api/5.geojson.mddocs/content/scripts/maplibre/2.api/6.use-script-maplibre.mddocs/content/scripts/maplibre/index.mdpackage.jsonpackages/script/THIRD_PARTY_LICENSES.mdpackages/script/package.jsonpackages/script/src/registry-logos.tspackages/script/src/registry-types.jsonpackages/script/src/registry.tspackages/script/src/runtime/components/Leaflet/ScriptLeafletMap.vuepackages/script/src/runtime/components/MapLibre/ScriptMapLibreGeoJson.vuepackages/script/src/runtime/components/MapLibre/ScriptMapLibreMap.vuepackages/script/src/runtime/components/MapLibre/ScriptMapLibreMarker.vuepackages/script/src/runtime/components/MapLibre/ScriptMapLibreNavigationControl.vuepackages/script/src/runtime/components/MapLibre/ScriptMapLibrePopup.vuepackages/script/src/runtime/components/MapLibre/useMapLibreResource.tspackages/script/src/runtime/maplibre-styles.tspackages/script/src/runtime/registry/maplibre.tspackages/script/src/runtime/registry/schemas.tspackages/script/src/runtime/types.tspackages/script/src/script-meta.tsplayground/nuxt.config.tsplayground/pages/index.vueplayground/pages/third-parties/maplibre.vuepnpm-workspace.yamlscripts/generate-registry-types.tstest/nuxt-runtime/maplibre-components.nuxt.test.tstest/nuxt-runtime/maplibre-map.nuxt.test.tstest/types/types.test-d.tstest/unit/maplibre-lifecycle.test.tstest/unit/maplibre-registry.test.tstest/unit/maplibre-styles.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- playground/nuxt.config.ts
- pnpm-workspace.yaml
- test/types/types.test-d.ts
- playground/pages/index.vue
- packages/script/src/script-meta.ts
- package.json
- packages/script/src/runtime/types.ts
- packages/script/package.json
- packages/script/src/runtime/components/Leaflet/ScriptLeafletMap.vue
2 participants
🔗 Linked issue
No linked issue.
❓ Type of change
📚 Description
Adds Leaflet 1.9.4 and MapLibre GL JS 5.24 as lazy-loaded, provider-agnostic alternatives to Google Maps.
Both integrations include Nuxt components for maps, markers, popups, and GeoJSON. Leaflet supports raster tile providers, while MapLibre adds vector styles, reactive camera controls, navigation controls, and CSP worker configuration.
Both load near the viewport and reserve layout space during SSR. Child resources are removed before the map is torn down.
The docs follow the existing Google Maps structure and cover setup, providers, accessibility, and real-world examples. The playground includes a Leaflet pickup locator using OpenStreetMap and a MapLibre delivery tracker using OpenFreeMap.
📸 Screenshots
Leaflet pickup locator
MapLibre delivery tracker