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 <leaflet-line> 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: <leaflet-line> 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.main
Buddy
4 weeks ago
parent
255d10499b
commit
3845e53329
@ -0,0 +1,37 @@
|
||||
// Tracks an ordered set of vertices contributed by <leaflet-line> children,
|
||||
// built entirely from the leaflet-line-sync/leaflet-line-remove events they
|
||||
// fire (see register.ts) -- never by querying the DOM or reading a child's
|
||||
// property directly. Shared by leaflet-polygon and leaflet-polyline.
|
||||
//
|
||||
// Order matters (it's the vertex sequence), so membership changes are
|
||||
// inserted at their actual document position via compareDocumentPosition --
|
||||
// that's a structural question about the tree, not a read of child state,
|
||||
// and there's no other sane source of truth for "which vertex comes first."
|
||||
export type LatLngTuple = [number, number];
|
||||
|
||||
export class VertexTracker {
|
||||
#vertices = new Map<HTMLElement, LatLngTuple>();
|
||||
#order: HTMLElement[] = [];
|
||||
|
||||
sync(element: HTMLElement, latlng: LatLngTuple): void {
|
||||
if (!this.#vertices.has(element)) this.#insertOrdered(element);
|
||||
this.#vertices.set(element, latlng);
|
||||
}
|
||||
|
||||
remove(element: HTMLElement): void {
|
||||
this.#vertices.delete(element);
|
||||
this.#order = this.#order.filter((el) => el !== element);
|
||||
}
|
||||
|
||||
coords(): LatLngTuple[] {
|
||||
return this.#order.map((el) => this.#vertices.get(el)!);
|
||||
}
|
||||
|
||||
#insertOrdered(el: HTMLElement): void {
|
||||
const idx = this.#order.findIndex(
|
||||
(existing) => (existing.compareDocumentPosition(el) & Node.DOCUMENT_POSITION_PRECEDING) !== 0,
|
||||
);
|
||||
if (idx === -1) this.#order.push(el);
|
||||
else this.#order.splice(idx, 0, el);
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,80 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
// A real page has all its <leaflet-*> markup already parsed by the browser's
|
||||
// HTML parser before the deferred module script (which calls
|
||||
// customElements.define for every tag) ever runs. customElements.define()
|
||||
// upgrades every matching element already in the document immediately and
|
||||
// synchronously -- including firing connectedCallback -- so a "child" tag
|
||||
// whose one-shot connect-time announcement is missed because its listening
|
||||
// "parent" tag wasn't defined yet is broken in exactly this scenario, even
|
||||
// though it looks fine in every other test in this suite (they all import
|
||||
// the component modules -- and so call customElements.define -- before
|
||||
// creating any element, which sidesteps the whole problem).
|
||||
//
|
||||
// This file deliberately does NOT statically import any component module,
|
||||
// so nothing here is defined yet when the tree below is built.
|
||||
describe('real page load order (markup before customElements.define)', () => {
|
||||
it('wires control-layers, layer-group and polygon/line correctly', async () => {
|
||||
const map = document.createElement('leaflet-map');
|
||||
map.setAttribute('lat', '51.5');
|
||||
map.setAttribute('lng', '-0.09');
|
||||
map.setAttribute('zoom', '13');
|
||||
|
||||
const controlLayers = document.createElement('leaflet-control-layers');
|
||||
const baseTile = document.createElement('leaflet-tile-layer');
|
||||
baseTile.setAttribute('type', 'base');
|
||||
baseTile.setAttribute('name', 'Base');
|
||||
baseTile.setAttribute('active', '');
|
||||
baseTile.setAttribute('url', 'https://tile.example/{z}/{x}/{y}.png');
|
||||
controlLayers.append(baseTile);
|
||||
|
||||
const group = document.createElement('leaflet-layer-group');
|
||||
const groupedMarker = document.createElement('leaflet-marker');
|
||||
groupedMarker.setAttribute('lat', '1');
|
||||
groupedMarker.setAttribute('lng', '2');
|
||||
group.append(groupedMarker);
|
||||
|
||||
const polygon = document.createElement('leaflet-polygon');
|
||||
const vertices = [
|
||||
['51.509', '-0.08'],
|
||||
['51.503', '-0.06'],
|
||||
['51.51', '-0.047'],
|
||||
].map(([lat, lng]) => {
|
||||
const line = document.createElement('leaflet-line');
|
||||
line.setAttribute('lat', lat);
|
||||
line.setAttribute('lng', lng);
|
||||
return line;
|
||||
});
|
||||
polygon.append(...vertices);
|
||||
|
||||
map.append(controlLayers, group, polygon);
|
||||
document.body.append(map);
|
||||
|
||||
// Nothing is upgraded yet -- these are plain, undefined elements.
|
||||
expect(map.leafletObject).toBeUndefined();
|
||||
|
||||
// Mirrors index.ts's real export order, which is what actually runs when
|
||||
// a page does `import 'leaflet-components'`.
|
||||
const lc = await import('../src/index.ts');
|
||||
|
||||
expect(map).toBeInstanceOf(lc.LeafletMap);
|
||||
expect(map.leafletObject).toBeDefined();
|
||||
expect(polygon.leafletObject?.getLatLngs()).toEqual([
|
||||
[
|
||||
{ lat: 51.509, lng: -0.08 },
|
||||
{ lat: 51.503, lng: -0.06 },
|
||||
{ lat: 51.51, lng: -0.047 },
|
||||
],
|
||||
]);
|
||||
|
||||
const controlLayersInternal = controlLayers.leafletObject as unknown as {
|
||||
_layers: { name: string }[];
|
||||
};
|
||||
// oxlint-disable-next-line no-underscore-dangle -- Leaflet's own field name
|
||||
expect(controlLayersInternal._layers.map((l) => l.name)).toEqual(['Base']);
|
||||
|
||||
expect(group.leafletObject?.hasLayer(groupedMarker.leafletObject!)).toBe(true);
|
||||
|
||||
map.remove();
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue