From 5721321d25e160fb0a8d49a6c5c0bc04247f944c Mon Sep 17 00:00:00 2001 From: Masashi Katsumata Date: Sat, 7 Nov 2026 21:56:48 -0800 Subject: [PATCH 1/2] Stop resync from rewriting sources and layers that did not change styleReady flipped between true and false about every 0.3 s on a map that was doing nothing (26-52 flips in 10 s on the clearOverlays test page, with the map re-rendering ~100 times in that window). The loop: a styledata arriving while a GeoJSON source is still loading sets styleReady to false, the idle that follows sets it back and resyncs every overlay, and each resync called setData on every source and moveLayer on the marker layers. moveLayer marks the style changed even when the layer stays where it was, so another styledata came while the just-rewritten sources were loading, and so on. Stopping any single controller's resync did not end it, because they all wrote. bringMarkerLayersToFront now moves nothing when the marker layers are already on top (or the order is still the one it last produced), and the layers write through setSourceFeatures, which skips setData when the features are the same objects as the last write to that same source. Renderers build a new feature for every change, so identity means "unchanged"; a style swap creates new source objects, so a lost source is always written again and design changes still restore the overlays. ResyncWritesNothing.test.mjs pins both, plus "a resync with nothing to do touches nothing" and "after a style swap it rebuilds". --- src/circle/MapplsCircleLayer.ts | 4 +- src/helpers.ts | 72 +++++++++++++++- src/marker/MarkerLayer.ts | 4 +- src/polygon/MapplsPolygonLayer.ts | 6 +- src/polyline/MapplsPolylineLayer.ts | 4 +- test/ResyncWritesNothing.test.mjs | 129 ++++++++++++++++++++++++++++ 6 files changed, 209 insertions(+), 10 deletions(-) create mode 100644 test/ResyncWritesNothing.test.mjs diff --git a/src/circle/MapplsCircleLayer.ts b/src/circle/MapplsCircleLayer.ts index 476800d..15e3433 100644 --- a/src/circle/MapplsCircleLayer.ts +++ b/src/circle/MapplsCircleLayer.ts @@ -3,7 +3,7 @@ import type { LayerSpecification, } from 'maplibre-gl'; import { type CircleEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type PolygonFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type PolygonFeature } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export type MapplsActualCircle = PolygonFeature & { id?: string | number }; @@ -51,7 +51,7 @@ export class MapplsCircleLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/src/helpers.ts b/src/helpers.ts index 15e60fe..48cd857 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -42,14 +42,84 @@ export function removeSourceIfExists(map: maplibregl.Map, sourceId: string): voi if (map.getSource(sourceId)) map.removeSource(sourceId); } +const MARKER_LAYER_IDS = ['mc-raster-lyr-mc-marker-tiles', 'mc-marker-layer', 'mc-marker-drag-layer']; + +// The layer order this module last produced, per map. Lets bringMarkerLayersToFront tell +// "nothing moved since I arranged it" apart from "someone added a layer on top". +const arrangedOrders = new WeakMap(); + +function currentLayerOrder(map: maplibregl.Map): string[] | null { + try { + const m = map as unknown as { + getLayersOrder?: () => string[]; + style?: { order?: string[] }; + getStyle?: () => { layers?: Array<{ id: string }> } | undefined; + }; + if (typeof m.getLayersOrder === 'function') return m.getLayersOrder(); + if (Array.isArray(m.style?.order)) return [...m.style!.order!]; + const layers = m.getStyle?.()?.layers; + return layers ? layers.map((layer) => layer.id) : null; + } catch { + return null; + } +} + +/** + * Keep the marker layers above every other layer. + * + * **Moves nothing when they already are.** `moveLayer` marks the style as changed even when + * the layer stays where it was, and every change fires `styledata`. The provider treats a + * `styledata` that arrives while sources are still loading as "style not ready", and the + * `idle` that follows resyncs every overlay — which called this again. The style flipped + * between ready and not ready about every 0.3 s, forever, with nothing on the map changing. + */ export function bringMarkerLayersToFront(map: maplibregl.Map): void { - for (const layerId of ['mc-raster-lyr-mc-marker-tiles', 'mc-marker-layer', 'mc-marker-drag-layer']) { + const order = currentLayerOrder(map); + if (order) { + const present = MARKER_LAYER_IDS.filter((layerId) => order.includes(layerId)); + if (present.length === 0) return; + const tail = order.slice(order.length - present.length); + if (tail.every((layerId, i) => layerId === present[i])) return; + if (arrangedOrders.get(map) === order.join('\n')) return; + } + for (const layerId of MARKER_LAYER_IDS) { try { if (map.getLayer(layerId)) map.moveLayer(layerId); } catch { continue; } } + const arranged = currentLayerOrder(map); + if (arranged) arrangedOrders.set(map, arranged.join('\n')); +} + +// What each GeoJSON source was last given, keyed by the source object itself. A style swap +// replaces the source objects, so a lost source never matches and is written again. +const writtenFeatures = new WeakMap(); + +/** + * `source.setData` that skips the write when the features are the same objects, in the same + * order, as the last write to this very source. + * + * Renderers build a new feature object whenever an overlay changes, so identity is enough to + * say "nothing changed". Skipping matters for the same reason as in + * [bringMarkerLayersToFront]: a redundant setData puts the source back into "loading", the + * next `styledata` then reads as "style not ready", and the resync on `idle` wrote it again. + */ +export function setSourceFeatures( + source: { setData(data: GeoJSONSourceData): unknown }, + features: FeatureCollection['features'], +): void { + const previous = writtenFeatures.get(source); + if ( + previous && + previous.length === features.length && + previous.every((feature, i) => feature === features[i]) + ) { + return; + } + source.setData({ type: 'FeatureCollection', features } as GeoJSONSourceData); + writtenFeatures.set(source, [...features]); } export function metersToPixels(meters: number, latitude: number): number { diff --git a/src/marker/MarkerLayer.ts b/src/marker/MarkerLayer.ts index 464bc02..530df96 100644 --- a/src/marker/MarkerLayer.ts +++ b/src/marker/MarkerLayer.ts @@ -4,7 +4,7 @@ import type { LayerSpecification, } from 'maplibre-gl'; import type { MarkerEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type PointFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type PointFeature } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export type MapplsActualMarker = PointFeature; @@ -93,7 +93,7 @@ export class MarkerLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/src/polygon/MapplsPolygonLayer.ts b/src/polygon/MapplsPolygonLayer.ts index 60337d5..31ef258 100644 --- a/src/polygon/MapplsPolygonLayer.ts +++ b/src/polygon/MapplsPolygonLayer.ts @@ -5,7 +5,7 @@ import type { LineFeature, PolygonFeature, } from '../helpers'; -import { bringMarkerLayersToFront } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export interface MapplsActualPolygon { @@ -69,8 +69,8 @@ export class MapplsPolygonLayer { const outlineSource = this.holder.map.getSource(this.outlineSourceId) as GeoJSONSource | undefined; if (!fillSource || !outlineSource) return false; - fillSource.setData(fillData); - outlineSource.setData(outlineData); + setSourceFeatures(fillSource, fillData.features); + setSourceFeatures(outlineSource, outlineData.features); return true; } catch { return false; diff --git a/src/polyline/MapplsPolylineLayer.ts b/src/polyline/MapplsPolylineLayer.ts index 3b2fe93..8be9363 100644 --- a/src/polyline/MapplsPolylineLayer.ts +++ b/src/polyline/MapplsPolylineLayer.ts @@ -1,6 +1,6 @@ import type { GeoJSONSource, LayerSpecification } from 'maplibre-gl'; import type { PolylineEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type LineFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type LineFeature } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export type MapplsActualPolyline = LineFeature[]; @@ -45,7 +45,7 @@ export class MapplsPolylineLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/test/ResyncWritesNothing.test.mjs b/test/ResyncWritesNothing.test.mjs new file mode 100644 index 0000000..ccb2904 --- /dev/null +++ b/test/ResyncWritesNothing.test.mjs @@ -0,0 +1,129 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { after, test } from 'node:test'; +import { pathToFileURL } from 'node:url'; +import { resolve } from 'node:path'; +import { build } from 'tsup'; +import { createGeoPoint, createPolylineState, PolylineManager } from '@mapconductor/js-sdk-core'; + +// The provider marks the style "not ready" on a styledata that arrives while a source is +// still loading, and resyncs every overlay on the idle that follows. A resync that rewrote +// unchanged sources (setData) or re-moved layers that were already on top (moveLayer) fired +// another styledata, so the style flipped between ready and not ready every ~0.3 s forever. +// These tests pin "a resync with nothing to do writes nothing", and that a lost style is +// still rebuilt. +mkdirSync(new URL('../build/', import.meta.url), { recursive: true }); +const outDir = mkdtempSync(new URL('../build/resync-tests-', import.meta.url)); +after(() => rmSync(outDir, { recursive: true, force: true })); +await build({ + entry: { + helpers: 'src/helpers.ts', + controller: 'src/polyline/MapplsPolylineController.ts', + renderer: 'src/polyline/MapplsPolylineOverlayRenderer.ts', + layer: 'src/polyline/MapplsPolylineLayer.ts', + }, + outDir, format: ['esm'], platform: 'node', splitting: false, + dts: false, sourcemap: false, config: false, silent: true, +}); +const { bringMarkerLayersToFront, setSourceFeatures } = await import(pathToFileURL(resolve(outDir, 'helpers.mjs'))); +const { MapplsPolylineController } = await import(pathToFileURL(resolve(outDir, 'controller.mjs'))); +const { MapplsPolylineOverlayRenderer } = await import(pathToFileURL(resolve(outDir, 'renderer.mjs'))); +const { MapplsPolylineLayer } = await import(pathToFileURL(resolve(outDir, 'layer.mjs'))); + +function fakeMap(initialLayers = ['background', 'road-label']) { + const writes = { setData: 0, moveLayer: 0, addSource: 0, addLayer: 0 }; + let sources = new Map(); + let order = [...initialLayers]; + const layers = new Map(initialLayers.map((id) => [id, { id }])); + const map = { + writes, + get order() { return [...order]; }, + getLayersOrder: () => [...order], + style: { get order() { return [...order]; } }, + getStyle: () => ({ layers: order.map((id) => layers.get(id)) }), + getSource: (id) => sources.get(id), + getLayer: (id) => layers.get(id), + addSource(id) { + writes.addSource++; + sources.set(id, { setData() { writes.setData++; } }); + }, + addLayer(layer, before) { + writes.addLayer++; + layers.set(layer.id, layer); + const at = before ? order.indexOf(before) : -1; + if (at >= 0) order.splice(at, 0, layer.id); else order.push(layer.id); + }, + moveLayer(id, before) { + writes.moveLayer++; + order = order.filter((x) => x !== id); + const at = before ? order.indexOf(before) : -1; + if (at >= 0) order.splice(at, 0, id); else order.push(id); + }, + // A style swap drops every source and layer the SDK added. + reloadStyle() { + sources = new Map(); + order = order.filter((id) => !id.startsWith('mc-') && !id.startsWith('polyline')); + for (const id of [...layers.keys()]) if (!order.includes(id)) layers.delete(id); + }, + }; + return map; +} + +test('bringMarkerLayersToFront moves nothing when the marker layers are already on top', () => { + const map = fakeMap(['background', 'mc-marker-layer', 'mc-marker-drag-layer']); + bringMarkerLayersToFront(map); + assert.equal(map.writes.moveLayer, 0); +}); + +test('bringMarkerLayersToFront moves the marker layers once, then leaves them alone', () => { + const map = fakeMap(['background', 'mc-marker-layer', 'mc-marker-drag-layer']); + map.addLayer({ id: 'polyline-layer' }); + bringMarkerLayersToFront(map); + assert.deepEqual(map.order.slice(-2), ['mc-marker-layer', 'mc-marker-drag-layer']); + const moved = map.writes.moveLayer; + assert.ok(moved > 0); + bringMarkerLayersToFront(map); + bringMarkerLayersToFront(map); + assert.equal(map.writes.moveLayer, moved); +}); + +test('setSourceFeatures skips identical features but writes to a replaced source', () => { + let calls = 0; + const source = { setData() { calls++; } }; + const a = { type: 'Feature' }; + const b = { type: 'Feature' }; + setSourceFeatures(source, [a, b]); + setSourceFeatures(source, [a, b]); + assert.equal(calls, 1); + setSourceFeatures(source, [b, a]); + assert.equal(calls, 2, 'a different order is a change'); + setSourceFeatures(source, [b]); + assert.equal(calls, 3, 'a removed feature is a change'); + const replaced = { setData() { calls++; } }; + setSourceFeatures(replaced, [b]); + assert.equal(calls, 4, 'a new source object (style swap) is always written'); +}); + +test('resync with nothing changed writes nothing; after a style swap it rebuilds', async () => { + const map = fakeMap(['background', 'road-label', 'mc-marker-layer', 'mc-marker-drag-layer']); + const holder = { map }; + const layer = new MapplsPolylineLayer({ holder, canEditStyle: () => true }); + const renderer = new MapplsPolylineOverlayRenderer({ layer, polylineManager: new PolylineManager(), holder }); + const controller = new MapplsPolylineController(renderer); + await controller.add([createPolylineState({ + id: 'PL', + points: [createGeoPoint({ latitude: 35, longitude: 139 }), createGeoPoint({ latitude: 36, longitude: 140 })], + })]); + const afterAdd = { ...map.writes }; + assert.ok(afterAdd.setData > 0); + + await controller.resync(); + await controller.resync(); + assert.deepEqual(map.writes, afterAdd, 'a resync with nothing to do must not touch the style'); + + map.reloadStyle(); + await controller.resync(); + assert.ok(map.getSource('polyline-source'), 'the lost source is added again'); + assert.ok(map.writes.setData > afterAdd.setData, 'and its data written again'); + assert.ok(map.order.includes('polyline-layer'), 'and its layer'); +}); From c7019969a042262567f2111b046efe39ba1fb35e Mon Sep 17 00:00:00 2001 From: Masashi Katsumata Date: Sat, 7 Nov 2026 22:01:58 -0800 Subject: [PATCH 2/2] Load marker icons through the callback form of loadImage Every Mappls page with a marker logged "Uncaught (in promise) TypeError: e is not a function" from inside the Mappls SDK (21 times on the marker sample, 2 on the polygon sample). The Mappls map is on the mapbox-gl v1 line, where loadImage(url, callback) returns nothing and answers through the callback. The renderer awaited it in maplibre's promise form, so it got undefined, fell back to a plain Image (fetching each icon twice), and the SDK called the missing callback when its own fetch finished. The SDK is behaving as documented for its version; the call was wrong. loadImageThroughMap passes a callback and still honours a promise if a later SDK returns one. MarkerIconLoad.test.mjs uses a callback-only loadImage like the SDK's. The API snapshot gains the private member. --- api/react-for-mappls.api.d.ts | 11 +++++ src/marker/MapplsMarkerOverlayRenderer.ts | 33 +++++++++++++- test/MarkerIconLoad.test.mjs | 55 +++++++++++++++++++++++ 3 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 test/MarkerIconLoad.test.mjs diff --git a/api/react-for-mappls.api.d.ts b/api/react-for-mappls.api.d.ts index 3fcd5f4..1867351 100644 --- a/api/react-for-mappls.api.d.ts +++ b/api/react-for-mappls.api.d.ts @@ -185,6 +185,17 @@ declare class MapplsMarkerOverlayRenderer extends AbstractMarkerOverlayRenderer< private ensureImages; private ensureImage; private loadBitmapIcon; + /** + * `map.loadImage` in the callback form the Mappls SDK has. + * + * The Mappls map is built on the mapbox-gl v1 line, where `loadImage(url, callback)` + * returns nothing and reports through the callback. Awaiting it as maplibre's promise + * form got `undefined` (so this fell back to a plain Image and fetched the icon twice), + * and the SDK then called the missing callback when its own fetch finished, which threw + * "e is not a function" as an unhandled rejection inside the SDK on every page with a + * marker. A promise, if a future SDK returns one, is honoured as well. + */ + private loadImageThroughMap; private ensureFallbackDefaultIcon; private removeUnusedImages; private syncIconOffsets; diff --git a/src/marker/MapplsMarkerOverlayRenderer.ts b/src/marker/MapplsMarkerOverlayRenderer.ts index aff247a..483dd9d 100644 --- a/src/marker/MapplsMarkerOverlayRenderer.ts +++ b/src/marker/MapplsMarkerOverlayRenderer.ts @@ -228,8 +228,7 @@ export class MapplsMarkerOverlayRenderer extends AbstractMarkerOverlayRenderer< url: string, ): Promise { try { - const image = await this.holder.map.loadImage(url); - return image.data; + return await this.loadImageThroughMap(url); } catch { const image = new Image(); image.decoding = 'async'; @@ -242,6 +241,36 @@ export class MapplsMarkerOverlayRenderer extends AbstractMarkerOverlayRenderer< } } + /** + * `map.loadImage` in the callback form the Mappls SDK has. + * + * The Mappls map is built on the mapbox-gl v1 line, where `loadImage(url, callback)` + * returns nothing and reports through the callback. Awaiting it as maplibre's promise + * form got `undefined` (so this fell back to a plain Image and fetched the icon twice), + * and the SDK then called the missing callback when its own fetch finished, which threw + * "e is not a function" as an unhandled rejection inside the SDK on every page with a + * marker. A promise, if a future SDK returns one, is honoured as well. + */ + private loadImageThroughMap(url: string): Promise { + type Loaded = HTMLImageElement | ImageBitmap | ImageData; + const map = this.holder.map as unknown as { + loadImage( + url: string, + callback: (error?: Error | null, image?: Loaded | null) => void, + ): void | Promise<{ data: Loaded }>; + }; + return new Promise((resolve, reject) => { + const returned = map.loadImage(url, (error, image) => { + if (error) reject(error); + else if (image) resolve(image); + else reject(new Error(`Failed to load marker icon: ${url}`)); + }); + if (returned && typeof (returned as Promise<{ data: Loaded }>).then === 'function') { + (returned as Promise<{ data: Loaded }>).then((response) => resolve(response.data), reject); + } + }); + } + private ensureFallbackDefaultIcon(): boolean { if (this.holder.map.hasImage(DEFAULT_ICON_ID)) return true; const size = 48; diff --git a/test/MarkerIconLoad.test.mjs b/test/MarkerIconLoad.test.mjs new file mode 100644 index 0000000..0556a09 --- /dev/null +++ b/test/MarkerIconLoad.test.mjs @@ -0,0 +1,55 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { after, test } from 'node:test'; +import { pathToFileURL } from 'node:url'; +import { resolve } from 'node:path'; +import { build } from 'tsup'; +import { createGeoPoint, createMarkerState } from '@mapconductor/js-sdk-core'; + +// The Mappls map is on the mapbox-gl v1 line: loadImage(url, callback) returns nothing. +// The renderer awaited it in maplibre's promise form, never passed a callback, and the SDK +// threw "e is not a function" when its fetch finished (a pageerror on every page with a +// marker). This fake has exactly that callback-only signature. +mkdirSync(new URL('../build/', import.meta.url), { recursive: true }); +const outDir = mkdtempSync(new URL('../build/marker-icon-tests-', import.meta.url)); +after(() => rmSync(outDir, { recursive: true, force: true })); +await build({ + entry: { renderer: 'src/marker/MapplsMarkerOverlayRenderer.ts' }, + outDir, format: ['esm'], platform: 'node', splitting: false, + dts: false, sourcemap: false, config: false, silent: true, +}); +const { MapplsMarkerOverlayRenderer } = await import(pathToFileURL(resolve(outDir, 'renderer.mjs'))); + +function fixture() { + const images = new Map(); + const calls = []; + const map = { + hasImage: (id) => images.has(id), + addImage: (id, image) => { images.set(id, image); }, + loadImage(url, callback) { + calls.push({ url, callbackType: typeof callback }); + setTimeout(() => { + if (typeof callback !== 'function') return; // the real SDK throws here + callback(null, { fakeImage: url }); + }, 0); + return undefined; + }, + }; + const noLayer = { draw() { return true; }, drawSelected() { return true; }, setIconOffsets() {} }; + const renderer = new MapplsMarkerOverlayRenderer({ + holder: { map }, + markerManager: { allEntities: () => [] }, + markerLayer: noLayer, + dragLayer: noLayer, + }); + return { renderer, images, calls }; +} + +test('marker icons are loaded through the callback form of map.loadImage', async () => { + const f = fixture(); + const state = createMarkerState({ id: 'M', position: createGeoPoint({ latitude: 35, longitude: 139 }) }); + await f.renderer.onAdd([{ state, bitmapIcon: state.icon?.toBitmapIcon?.() ?? { url: 'data:image/png;base64,' } }]); + assert.ok(f.calls.length > 0, 'loadImage was used'); + assert.ok(f.calls.every((c) => c.callbackType === 'function'), 'every loadImage call passes a callback'); + assert.deepEqual(f.images.get('mc-default'), { fakeImage: f.calls[0].url }, 'the loaded image is registered'); +});