Re-architect ui-mapbox: declarative, dynamic-DOM-native family + StoreLocator orchestrator - #567
Merged
Merged
Conversation
Re-architect the @studiometa/ui-mapbox family so MapboxMap no longer declares its children. Children register independently, resolve their parent map via $closest on mount and self-gate on map readiness through AbstractMapboxMapChild.whenMapReady, injecting on ready and cleaning up in destroyed() against a cached map ref. This makes every child Fetch/appendChild dynamic-DOM-native (self-inject on insertion, self-clean on removal). Dissolve StoreLocator/StoreLocatorItem into declarative MapboxCluster + MapboxClusterItem: rendered items are simultaneously the list and the GeoJSON source, one registry driving both. Add registerMapboxComponents() convenience helper. Spike baseline (pre-hardening): build + lint + 102 tests green. Known open blockers to address before shipping: unguarded throws can deadlock the global SmartQueue (B1), teardown against a removed map throws (B2), Fetch-swap dup-id on Source/Layer (B3), and no-retry map resolution for isolated dynamic insertion (M1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
… teardown Fix four blockers found by adversarial review of the declarative family: - B1: wrap ready-callback and subclass teardown in AbstractMapboxMapChild so a throwing mapbox call routes to $warn + an `error` event instead of wedging js-toolkit's global queue. Subclasses implement `__onDestroyed` (guarded by the base) instead of overriding `destroyed`. - B2: subscribe to the map's own `remove` event at ready-time; drop the cached map reference when it fires so no teardown ever calls a method on a removed map. Teardown is now order-independent. - B3: add per-map ownership tokens for id-addressed sources and layers so a Fetch swap adopts/updates the existing id instead of throwing a duplicate, and the outgoing instance never deletes the contribution the incoming one now owns; externally declared ids stay untouched. - M1/M2: make map resolution retryable and standing. MapboxMap and MapboxCluster dispatch a document-level connected event; children park on it when no parent exists yet, and re-inject on a new map after a destroy -> remount. Add regression specs for each blocker and relax the layer/source specs only as needed for ownership tracking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
…rce driver Split the search-UX concerns back out of MapboxCluster: the cluster is now a pure clustered-source driver (item registry, GeoJSON source + layers, cluster-click zoom, item-click reporting, update event), and a new StoreLocator orchestrator owns selection, popup, viewport filtering, distance sort, geocoder wiring and fit-on-update on top of it. MapboxClusterItem stays the single item class; StoreLocatorItem stays dissolved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
The list of items lives inside the nested MapboxCluster, outside the StoreLocator's ref scope, so a `data-ref="list"` on the sidebar never bound to the orchestrator — `__reorderList` already fell back to each item's shared parent. Remove the misleading ref from the config, props, and docs; reorder is unchanged (still appends each in-view item into its shared parent to match the distance sort). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
1 task
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #567 +/- ##
============================================
+ Coverage 86.19% 86.86% +0.66%
Complexity 145 145
============================================
Files 139 139
Lines 4354 4696 +342
Branches 817 877 +60
============================================
+ Hits 3753 4079 +326
- Misses 528 540 +12
- Partials 73 77 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Export SizeUnchanged@studiometa/ui
|
Address the confirmed findings from the PR #567 external review. Foundation: - whenMapReady now accepts void | Promise<void> and awaits the returned promise inside the same containment, so a rejected async callback routes to $warn + the error event instead of an unhandled rejection. Subclasses guard on __readyMap === map after every await so a callback whose map was removed/replaced mid-flight no-ops (F-async). Hardening: - Bind the concrete map's remove handler at bind-time, before load, so a map removed before it finishes loading re-resolves the child onto a replacement instead of stranding it (H2). - Detach MapboxMap forwarding listeners on destroy, and add an __onMapRemove hook so a layer/cluster flushes its map-scoped listeners before the map ref is cleared (H3). - MapboxLayer adopts AND refreshes a same-id layer (remove + re-add) instead of keeping the outgoing definition (H4). - Move image ownership to the per-map registry: a same-name swap adopts the sprite so the outgoing instance no longer removes it, and image adds are tracked incrementally so a mid-batch failure still cleans up (H5). - Wire a nested marker/popup from both sides so a dynamic append to an already-loaded map cannot leave the popup attached to neither (H8). - Remove a previous element-targeted geocoder control before recreating it on map replacement (H9). - Restore a popup source element's visibility on teardown when it hid it. Orchestrator: - StoreLocator subscribes to MAPBOX_MAP_CONNECTED / MAPBOX_CLUSTER_CONNECTED for retryable + standing resolution and re-wires onto a replacement cluster; it also subscribes to the map's remove event so a removed map is dropped and never called into (D1, D3). - Dropping a stale selection during a swap runs the full deselect cleanup so the removed store's popup is removed and deselect is emitted (D2). - Guard MapboxCluster.getClusterExpansionZoom against a removed map (D4). - Expose cluster items / the update payload as a defensive copy; document the setData invariant, duplicate-id behavior and popup-close independence. Deferred with code comments and follow-up notes: stale ownership entries after external resource removal / setStyle (H6), and layer recovery after a source is removed while the layer stays mounted (H7). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
Fix PR #567 review H6/H7: a map.setStyle() wipes every source, layer and sprite while the still-mounted declarative children survive, so the family resources silently vanished; and the per-map ownership registry went stale when a resource was removed outside component teardown (setStyle, external removeSource/removeLayer), letting a later same-id resource be misclassified as family-owned and wrongly adopted or deleted. H7 — AbstractMapboxMapChild now keeps a standing style.load subscription (the Mapbox-recommended hook for re-adding custom sources/layers when the base style changes, fired once per style load) and re-runs its injection so MapboxSource/Layer/Image/Cluster re-add themselves onto the fresh style. The re-run reuses the first-injection containment and is guarded on the resource already being present, so it never double-injects (at the initial style load readyMap is not set yet, so only genuine later setStyle calls re-inject). MapboxLayer keeps a standing sourcedata recovery watch (gated on its source id) so a still-mounted layer re-commits when its source is removed then re-added — e.g. a sibling MapboxSource teardown dropping it. MapboxCluster's injection was made idempotent (guarded addSource/addLayer, off-before-on listeners) so re-injection never duplicates a listener. H6 — ownership entries now carry a liveness probe validated on read in getMapboxOwner: sources and layers key it to the very object they added (getSource/getLayer identity), so a resource wiped by setStyle or removed/replaced externally reads as unowned and is pruned, rather than a concurrency-fragile "clear the whole map on style reload" pass that would race sibling re-injections. Sprites can only probe hasImage (no object identity); that external-readd edge is documented as a remaining follow-up. Extend the mock with setStyle (clears sources/layers/images + fires style.load) and a sourcedata emit on addSource, and add style-reload.spec.ts covering re-injection, layer recovery, and ownership self-healing. 162 MapboxMap tests pass (was 153). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
Update the MapboxMap family and StoreLocator documentation, the vue-mapbox-gl migration guide and the package README to match the self-registering, orchestrator-based architecture: - Document `registerMapboxComponents()` and per-component registration; drop the stale "registering MapboxMap alone mounts the children" and `config.components` claims. - Fix the story `app.js` files to register every component they use. - Add `MapboxCluster`/`MapboxClusterItem` and `registerMapboxComponents` API entries; remove spurious marker/popup option getters; add the `MapboxGeocoder` `result` event and `MapboxSource` `source` getter. - Document `setStyle` layer persistence and the Boolean-option presence-not-value gotcha (geocoder `add-to-map`, StoreLocator sort). - Rewrite the migration guide's cluster mapping/data section and mark StoreLocator as available (StoreLocatorItem removed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
Drop the registerMapboxComponents convenience helper from @studiometa/ui-mapbox. Now that every family component is self-registering and resolves its parent map through the connected-event retry, registration order is irrelevant, so document per-component registration with js-toolkit's lazy importWhen* helpers as the recommended default and keep an eager registerComponent example for completeness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
Convert the MapboxMap and StoreLocator js-api tables to the heading format used across the other component docs, state current facts without past/migration framing, link the usage guide instead of restating generic js-toolkit behaviour, and de-duplicate the repeated registration and lazy-loading notes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
…component registration Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
Closed
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.
Summary
Re-architects the (still-unreleased)
@studiometa/ui-mapboxfamily to be declarative and dynamic-DOM-native, and supersedes #566.MapboxMapno longer declares its children: every child registers independently, resolves its parent map on mount, and self-gates on map readiness. As a result the whole family self-injects when its elements enter the DOM (viaFetch,appendChild, template swaps) and self-cleans when they leave — no orchestration from the map, noconfig.components.It also dissolves
StoreLocator/StoreLocatorIteminto composable primitives: aMapboxClustersource-driver +MapboxClusterItemwhere the rendered items are simultaneously the sidebar list and the map's GeoJSON cluster source, plus a thin reintroducedStoreLocatororchestrator that layers the search UX on top.Because the package was never published to npm (
npm view @studiometa/ui-mapbox→ 404), there is no backward-compatibility surface — this ships cleanly as part of the initial feature.What changed
Declarative, dynamic-DOM-native family
MapboxMapdropsconfig.componentsand owns only the map. Children (MapboxMarker,MapboxPopup, controls,MapboxSource,MapboxLayer,MapboxImage(s),MapboxCluster) resolve the map via$closestand inject throughAbstractMapboxMapChild.whenMapReady(cb)(runs now if loaded, else once onmap-load).registerMapboxComponents()helper registers the whole family (since the map no longer resolves children). Per-componentregisterComponent+ lazyimportWhen*still work, unchanged.Hardening (the load-bearing part — see the adversarial review that drove it)
The move from an orchestrated lifecycle to an unordered one dropped three guarantees; all are restored package-locally so the package is self-sufficient:
AbstractMapboxMapChildruns the ready-callback and a base-owned__onDestroyed()teardown insidetry/catch, routing errors to$warn+ anerroremit and never rethrowing, so a throwing mapbox call can't wedge js-toolkit's shared task queue.removeevent at ready-time and null their cached map ref, so teardown is order-independent and never calls a method on a removed map.MapboxSource/MapboxLayeradopt-or-add on mount (aFetchswap with the same source id refreshes data instead of throwing a duplicate-id error) and remove only what they still own.MapboxMap/MapboxClusterdispatch document-level connected events; a child that mounts before its map exists (e.g. an eagerly-registered marker under a lazily-registered map) resolves once the map connects, and re-injects if the map is destroyed and remounted.MapboxCluster + MapboxClusterItem + StoreLocator orchestrator
MapboxClusteris a pure source driver: item registry → GeoJSONcluster:truesource + layers + cluster-click expansion zoom; emitsitem-click/update. Usable standalone for any declarative clustered map + list.MapboxClusterItemis a rendered item exposingid/lngLat/properties/popupContent, reflectingdata-in-bounds/data-activewhen driven.StoreLocatoris a thin orchestrator owning the search UX only: selection (select/deselect/active/flyTo/popup), geocoderresult→ fit, viewportmoveend→ in-bounds + distance sort +filter,fit-on-update.StoreLocatorItemis dissolved.Validation
innerHTMLreplace → adopted + data refreshed, no throw); whole-map-section removal (zero TypeErrors); queue-not-wedged after a failure; child-before-map resolves on connect.item-clickbus, viewport filter (source not rebuilt on pan), Fetch-swap keeping list + map in lockstep with stale-selection drop, geocoder→fit, and bare-cluster-does-not-select.Companion upstream PRs (defense-in-depth, not required)
The package hardening is self-sufficient, but two js-toolkit PRs harden the shared machinery underneath and let the workarounds relax over time:
add()promise.Supersedes #566
The lazy-children approach in #566 is subsumed: children are independently registered now, so lazy loading is just the existing
importWhen*+ per-component subpaths — no special child-resolution path needed. #566 will be closed in favour of this.🤖 Generated with Claude Code