From 3845e53329f7deda1de0bd9e11d0a0feb9738d6b Mon Sep 17 00:00:00 2001 From: Buddy Date: Sat, 22 Aug 2026 14:34:58 -0700 Subject: [PATCH] fix: remove parent-reaches-into-child patterns, fix a real load-order bug Two parent-looks-for-child patterns found and removed, per the child-emits-events-parent-never-reaches-in principle: - leaflet-control-layers: #childLayers() queried children directly via querySelectorAll + reading their leafletObject/attributes, in two call sites. Verified empirically that a parent's connectedCallback always completes before a freshly-connected child's does (even for an already-built subtree attached in one shot), which means both call sites always ran against unpopulated children -- dead code. Deleted; #onChildRegister (already event-driven) was doing all the real work. - leaflet-polygon/leaflet-polyline: #coords() queried children via querySelectorAll + read their .latlng property directly, using a MutationObserver + a payload-less event only as a "something changed, rescan everyone" signal. Rewritten to be purely event-driven: now fires leaflet-line-sync (connect + every lat/lng change, carrying its own position) and leaflet-line-remove (disconnect) on itself; a new shared VertexTracker (src/core/vertex-tracker.ts) turns that event stream into an ordered coordinate list, using compareDocumentPosition only to place a newly-registered vertex at its real document position rather than assuming registration order matches DOM order. No querySelectorAll, no MutationObserver, no reading a child's property. Testing the polygon rewrite in an actual browser (not jsdom) surfaced two real bugs, both fixed: 1. disconnectedCallback fires *after* a node is already detached from its parent, so leaflet-line's removal event had nowhere to bubble to. Fixed by caching parentNode while still connected and dispatching from that cached reference instead of from the (by-then-detached) node. 2. A load-order hazard affecting every "parent listens for a specific child tag's announcement" relationship in this codebase: customElements.define() upgrades every matching element already parsed into the page immediately and synchronously, so whichever tag gets defined first wins a race -- a child tag defined before its listening parent fires its one-shot connect-time announcement into a parent that doesn't exist yet, and it's lost for good. This broke polygon/polyline (introduced by this rewrite, since leaflet-line was exported before leaflet-polygon) and, independently, was already silently broken for leaflet-control-layers (a base/overlay layer leaked directly onto the map, bypassing the control entirely) and latently for leaflet-layer-group/leaflet-feature-group. src/index.ts's export order now encodes and documents the real dependency hierarchy (map -> containers that listen for child announcements -> concrete layer types -> their own children). Added test/load-order.test.ts, which is structurally different from every other test file: it never statically imports a component module, building the DOM with plain undefined elements first and only dynamically import()ing src/index.ts afterward -- reproducing a real page's actual load order, which every other test's "define everything first" pattern cannot catch. Confirmed it fails against the old ordering and passes against the fix. --- CLAUDE.md | 5 +- src/components/leaflet-control-layers.ts | 47 +++----------- src/components/leaflet-line.ts | 20 +++++- src/components/leaflet-polygon.ts | 35 ++++++----- src/components/leaflet-polyline.ts | 35 ++++++----- src/core/register.ts | 30 +++++++-- src/core/vertex-tracker.ts | 37 +++++++++++ src/index.ts | 36 ++++++++--- test/components/shapes.test.ts | 27 ++++++++ test/load-order.test.ts | 80 ++++++++++++++++++++++++ 10 files changed, 263 insertions(+), 89 deletions(-) create mode 100644 src/core/vertex-tracker.ts create mode 100644 test/load-order.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 18a008d..2a92946 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,6 +21,7 @@ Tests run under Vitest + jsdom (`test/**/*.test.ts`), with a single setup file ( - Some Leaflet DOM state (the ``/`