OL: fix style type switching and stale event listeners on style change

providers/beta/openlayers/src/appEvents.js

@@ -1,19 +1,12 @@
-import { createTileSource, createVectorTileLayer, createOGCVectorTileLayer } from './utils/tileLayers.js'
+import { createTileSource, createVectorTileLayer, createOGCVectorTileLayer, createMapStyleLayer } from './utils/tileLayers.js'
 
-export { createTileSource, createVectorTileLayer, createOGCVectorTileLayer }
+export { createTileSource, createVectorTileLayer, createOGCVectorTileLayer, createMapStyleLayer }
 
-export function attachAppEvents ({ mapProvider, layer, layerType, transformRequest, events, eventBus, map }) {
+export function attachAppEvents ({ mapProvider, transformRequest, events, eventBus, map, onBaseSourceChange }) {
   const handleSetMapStyle = async (mapStyle) => {
-    if (layerType === 'raster') {
-      const source = createTileSource(mapStyle.url, transformRequest)
-      layer.setSource(source)
-    } else if (mapStyle.type === 'ogc-vt') {
-      const { layer: newLayer } = await createOGCVectorTileLayer(mapStyle.url, transformRequest, mapStyle)
-      map.getLayers().setAt(0, newLayer)
-    } else {
-      const { layer: newLayer } = await createVectorTileLayer(mapStyle.url, transformRequest, mapStyle)
-      map.getLayers().setAt(0, newLayer)
-    }
+    const { layer, source } = await createMapStyleLayer(mapStyle, transformRequest)
+    map.getLayers().setAt(0, layer)
+    onBaseSourceChange(source)
     eventBus.emit(events.MAP_STYLE_CHANGE, { mapStyleId: mapStyle.id })
   }
 

providers/beta/openlayers/src/appEvents.test.js

@@ -1,14 +1,13 @@
 import { attachAppEvents } from './appEvents.js'
 
-import { createTileSource, createVectorTileLayer } from './utils/tileLayers.js'
+import { createMapStyleLayer } from './utils/tileLayers.js'
 
+const mockLayerInstance = {}
 const mockSourceInstance = {}
-const mockVectorTileLayerInstance = {}
 
 jest.mock('./utils/tileLayers.js', () => ({
   __esModule: true,
-  createTileSource: jest.fn(() => mockSourceInstance),
-  createVectorTileLayer: jest.fn(async () => ({ layer: mockVectorTileLayerInstance, source: {} }))
+  createMapStyleLayer: jest.fn(async () => ({ layer: mockLayerInstance, source: mockSourceInstance }))
 }))
 
 const events = {
@@ -30,24 +29,33 @@ describe('attachAppEvents', () => {
   describe('raster', () => {
     function makeSetup () {
       jest.clearAllMocks()
-      const layer = { setSource: jest.fn() }
+      const setAt = jest.fn()
+      const map = { getLayers: jest.fn(() => ({ setAt })), setPixelRatio: jest.fn() }
+      const onBaseSourceChange = jest.fn()
       const eventBus = { on: jest.fn(), off: jest.fn(), emit: jest.fn() }
       const mapProvider = makeProvider()
-      const handles = attachAppEvents({ mapProvider, layer, layerType: 'raster', transformRequest: null, events, eventBus, map: makeMap() })
+      const handles = attachAppEvents({ mapProvider, transformRequest: null, events, eventBus, map, onBaseSourceChange })
       const handlerFor = (event) => eventBus.on.mock.calls.find(c => c[0] === event)[1]
-      return { layer, eventBus, handles, handlerFor, mapProvider }
+      return { map, setAt, onBaseSourceChange, eventBus, handles, handlerFor, mapProvider }
     }
 
     it('subscribes to MAP_SET_STYLE on the event bus', () => {
       const { eventBus } = makeSetup()
       expect(eventBus.on).toHaveBeenCalledWith(events.MAP_SET_STYLE, expect.any(Function))
     })
 
-    it('on MAP_SET_STYLE: creates a tile source and sets it on the layer', async () => {
-      const { layer, handlerFor } = makeSetup()
-      await handlerFor(events.MAP_SET_STYLE)({ url: 'https://new.tiles.com/{z}/{x}/{y}', id: 'newStyle' })
-      expect(createTileSource).toHaveBeenCalledWith('https://new.tiles.com/{z}/{x}/{y}', null)
-      expect(layer.setSource).toHaveBeenCalledWith(mockSourceInstance)
+    it('on MAP_SET_STYLE: creates a map style layer and places it at index 0', async () => {
+      const { setAt, handlerFor } = makeSetup()
+      const mapStyle = { url: 'https://new.tiles.com/{z}/{x}/{y}', id: 'newStyle', type: 'raster' }
+      await handlerFor(events.MAP_SET_STYLE)(mapStyle)
+      expect(createMapStyleLayer).toHaveBeenCalledWith(mapStyle, null)
+      expect(setAt).toHaveBeenCalledWith(0, mockLayerInstance)
+    })
+
+    it('on MAP_SET_STYLE: updates the base source for map events', async () => {
+      const { onBaseSourceChange, handlerFor } = makeSetup()
+      await handlerFor(events.MAP_SET_STYLE)({ url: 'https://new.tiles.com/{z}/{x}/{y}', id: 'newStyle', type: 'raster' })
+      expect(onBaseSourceChange).toHaveBeenCalledWith(mockSourceInstance)
     })
 
     it('on MAP_SET_STYLE: emits MAP_STYLE_CHANGE with the new style id', async () => {
@@ -72,7 +80,7 @@ describe('attachAppEvents', () => {
       const map = { getLayers: jest.fn(() => ({ setAt })), setPixelRatio: jest.fn() }
       const eventBus = { on: jest.fn(), off: jest.fn(), emit: jest.fn() }
       const mapProvider = makeProvider()
-      const handles = attachAppEvents({ mapProvider, layer: {}, layerType: 'vector', transformRequest: null, events, eventBus, map })
+      const handles = attachAppEvents({ mapProvider, transformRequest: null, events, eventBus, map, onBaseSourceChange: jest.fn() })
       const handlerFor = (event) => eventBus.on.mock.calls.find(c => c[0] === event)[1]
       return { map, setAt, eventBus, handles, handlerFor, mapProvider }
     }
@@ -81,8 +89,8 @@ describe('attachAppEvents', () => {
       const { setAt, handlerFor } = makeSetup()
       const mapStyle = { url: 'https://example.com/styles', id: 'newVts' }
       await handlerFor(events.MAP_SET_STYLE)(mapStyle)
-      expect(createVectorTileLayer).toHaveBeenCalledWith(mapStyle.url, null, mapStyle)
-      expect(setAt).toHaveBeenCalledWith(0, mockVectorTileLayerInstance)
+      expect(createMapStyleLayer).toHaveBeenCalledWith(mapStyle, null)
+      expect(setAt).toHaveBeenCalledWith(0, mockLayerInstance)
     })
 
     it('on MAP_SET_STYLE: emits MAP_STYLE_CHANGE with the new style id', async () => {
@@ -98,7 +106,7 @@ describe('attachAppEvents', () => {
       const map = makeMap()
       const eventBus = { on: jest.fn(), off: jest.fn(), emit: jest.fn() }
       const mapProvider = makeProvider()
-      attachAppEvents({ mapProvider, layer: {}, layerType: 'vector', transformRequest: null, events, eventBus, map })
+      attachAppEvents({ mapProvider, transformRequest: null, events, eventBus, map, onBaseSourceChange: jest.fn() })
       const handlerFor = (event) => eventBus.on.mock.calls.find(c => c[0] === event)[1]
       return { map, mapProvider, handlerFor }
     }

providers/beta/openlayers/src/mapEvents.js

@@ -6,17 +6,6 @@ const DEBOUNCE_IDLE_TIME = 500
 const MOVE_THROTTLE_TIME = 10
 const DRAG_TOLERANCE = 6
 
-function attachLoadEvents ({ source, map, emit, eventBus, events, on }) {
-  let loadedEmitted = false
-  on(source, 'tileloadend', () => {
-    if (!loadedEmitted) {
-      loadedEmitted = true
-      eventBus.emit(events.MAP_LOADED)
-    }
-  })
-  map.once('rendercomplete', () => emit(events.MAP_FIRST_IDLE))
-}
-
 function attachMoveEvents ({ map, view, emit, eventBus, events, on, debouncers }) {
   let moving = false
 
@@ -79,6 +68,7 @@ export function attachMapEvents ({
 }) {
   let destroyed = false
   const listeners = []
+  let sourceListeners = []
   const debouncers = []
 
   const view = map.getView()
@@ -110,21 +100,47 @@ export function attachMapEvents ({
     listeners.push([target, type, handler])
   }
 
-  attachLoadEvents({ source, map, emit, eventBus, events, on })
-  attachMoveEvents({ map, view: map.getView(), emit, eventBus, events, on, debouncers })
-  attachClickEvents({ map, eventBus, events, on })
+  const onSource = (target, type, handler) => {
+    target.on(type, handler)
+    sourceListeners.push([target, type, handler])
+  }
 
-  on(map, 'postrender', () => eventBus.emit(events.MAP_RENDER))
+  let loadedEmitted = false
+  const emitLoaded = () => {
+    if (!loadedEmitted) {
+      loadedEmitted = true
+      eventBus.emit(events.MAP_LOADED)
+    }
+  }
 
   const emitDataChange = debounce(() => emit(events.MAP_DATA_CHANGE), DEBOUNCE_IDLE_TIME)
   debouncers.push(emitDataChange)
-  on(source, 'tileloadend', emitDataChange)
+
+  const setSource = (newSource) => {
+    if (destroyed) {
+      return
+    }
+
+    sourceListeners.forEach(([target, type, handler]) => target.un(type, handler))
+    sourceListeners = []
+    onSource(newSource, 'tileloadend', emitLoaded)
+    onSource(newSource, 'tileloadend', emitDataChange)
+  }
+
+  attachMoveEvents({ map, view: map.getView(), emit, eventBus, events, on, debouncers })
+  setSource(source)
+  map.once('rendercomplete', () => emit(events.MAP_FIRST_IDLE))
+  attachClickEvents({ map, eventBus, events, on })
+
+  on(map, 'postrender', () => eventBus.emit(events.MAP_RENDER))
 
   return {
+    setSource,
     remove () {
       destroyed = true
       debouncers.forEach(d => d.cancel())
       listeners.forEach(([target, type, handler]) => target.un(type, handler))
+      sourceListeners.forEach(([target, type, handler]) => target.un(type, handler))
     }
   }
 }

providers/beta/openlayers/src/mapEvents.test.js

@@ -35,7 +35,9 @@ function makeTarget () {
   return {
     on: jest.fn((type, handler) => { (handlers[type] = handlers[type] || []).push(handler) }),
     once: jest.fn((type, handler) => { (handlers[type] = handlers[type] || []).push(handler) }),
-    un: jest.fn(),
+    un: jest.fn((type, handler) => {
+      handlers[type] = (handlers[type] || []).filter(h => h !== handler)
+    }),
     trigger: (type, event = {}) => (handlers[type] || []).forEach(h => h(event))
   }
 }
@@ -104,14 +106,14 @@ describe('attachMapEvents — move events', () => {
   it('emits MAP_MOVE_END (with state) when debounced callback fires', () => {
     const { view, eventBus } = setup()
     view.trigger('change')
-    mockDebounceFns[0].fn() // emitMoveEnd's inner callback
+    mockDebounceFns[1].fn() // emitMoveEnd's inner callback
     expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_MOVE_END, expect.any(Object))
   })
 
   it('resets moving flag after MAP_MOVE_END so MAP_MOVE_START can fire again', () => {
     const { view, eventBus } = setup()
     view.trigger('change')
-    mockDebounceFns[0].fn() // resets moving = false
+    mockDebounceFns[1].fn() // resets moving = false
     eventBus.emit.mockClear()
     view.trigger('change')
     expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_MOVE_START)
@@ -168,33 +170,46 @@ describe('attachMapEvents — render and data events', () => {
   it('emits MAP_DATA_CHANGE (with state) when debounced tileloadend callback fires', () => {
     const { source, eventBus } = setup()
     source.trigger('tileloadend')
-    mockDebounceFns[1].fn() // emitDataChange's inner callback
+    mockDebounceFns[0].fn() // emitDataChange's inner callback
     expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_DATA_CHANGE, expect.any(Object))
   })
+
+  it('moves source listeners when source changes', () => {
+    const { source, handles, eventBus } = setup()
+    const newSource = makeTarget()
+
+    handles.setSource(newSource)
+
+    source.trigger('tileloadend')
+    expect(eventBus.emit).not.toHaveBeenCalledWith(events.MAP_LOADED)
+
+    newSource.trigger('tileloadend')
+    expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_LOADED)
+  })
 })
 
 describe('attachMapEvents — dynamic zoom bounds', () => {
   it('isAtMaxZoom is updated in emitted event when view max zoom changes', () => {
     const { view, eventBus } = setup()
     view.trigger('change')
-    mockDebounceFns[0].fn()
+    mockDebounceFns[1].fn()
     expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_MOVE_END, expect.objectContaining({ isAtMaxZoom: false }))
 
     eventBus.emit.mockClear()
     view.getMaxZoom.mockReturnValue(7)
-    mockDebounceFns[0].fn()
+    mockDebounceFns[1].fn()
     expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_MOVE_END, expect.objectContaining({ isAtMaxZoom: true }))
   })
 
   it('isAtMinZoom is updated in emitted event when view min zoom changes', () => {
     const { view, eventBus } = setup()
     view.trigger('change')
-    mockDebounceFns[0].fn()
+    mockDebounceFns[1].fn()
     expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_MOVE_END, expect.objectContaining({ isAtMinZoom: false }))
 
     eventBus.emit.mockClear()
     view.getMinZoom.mockReturnValue(7)
-    mockDebounceFns[0].fn()
+    mockDebounceFns[1].fn()
     expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_MOVE_END, expect.objectContaining({ isAtMinZoom: true }))
   })
 })
@@ -203,8 +218,8 @@ describe('attachMapEvents — remove', () => {
   it('calls cancel on all debouncers', () => {
     const { handles } = setup()
     handles.remove()
-    expect(mockDebounceFns[0].wrapper.cancel).toHaveBeenCalled()
     expect(mockDebounceFns[1].wrapper.cancel).toHaveBeenCalled()
+    expect(mockDebounceFns[0].wrapper.cancel).toHaveBeenCalled()
   })
 
   it('calls un on all registered event targets', () => {
@@ -215,10 +230,28 @@ describe('attachMapEvents — remove', () => {
     expect(source.un).toHaveBeenCalled()
   })
 
+  it('unbinds source listeners when source changes', () => {
+    const { source, handles } = setup()
+    handles.setSource(makeTarget())
+    expect(source.un).toHaveBeenCalledWith('tileloadend', expect.any(Function))
+  })
+
+  it('does not bind a new source after remove', () => {
+    const { eventBus, handles } = setup()
+    const newSource = makeTarget()
+
+    handles.remove()
+    handles.setSource(newSource)
+    newSource.trigger('tileloadend')
+
+    expect(newSource.on).not.toHaveBeenCalled()
+    expect(eventBus.emit).not.toHaveBeenCalledWith(events.MAP_LOADED)
+  })
+
   it('prevents state-bearing events from emitting after destroy', () => {
     const { eventBus, handles } = setup()
     handles.remove()
-    mockDebounceFns[0].fn() // emitMoveEnd fires after destroy → getMapState returns null
+    mockDebounceFns[1].fn() // emitMoveEnd fires after destroy → getMapState returns null
     expect(eventBus.emit).not.toHaveBeenCalledWith(events.MAP_MOVE_END, expect.anything())
   })
 })

providers/beta/openlayers/src/openlayersProvider.js

@@ -1,13 +1,12 @@
 import OlMap from 'ol/Map.js'
 import View from 'ol/View.js'
-import TileLayer from 'ol/layer/Tile.js'
 import { defaults as defaultInteractions } from 'ol/interaction/defaults.js'
 import proj4 from 'proj4'
 import { register } from 'ol/proj/proj4.js'
 import { supportedShortcuts, DEFAULTS } from './defaults.js'
 import { getViewResolutionConfig, ZOOM_ALIGNMENT } from './utils/zoom.js'
 import { attachMapEvents } from './mapEvents.js'
-import { attachAppEvents, createTileSource, createVectorTileLayer, createOGCVectorTileLayer } from './appEvents.js'
+import { attachAppEvents, createMapStyleLayer } from './appEvents.js'
 import { getAreaDimensions, getCardinalMove, getExtentFromGeoJSON, getPaddedExtent, isGeometryObscured } from './utils/spatial.js'
 
 const CRS = 'EPSG:27700'
@@ -42,19 +41,7 @@ export default class OpenLayersProvider {
     this.mapSize = mapSize
     const { events, eventBus } = this
 
-    let tileLayer, source
-    if (mapStyle.type === 'raster') {
-      source = createTileSource(mapStyle.url, transformRequest)
-      tileLayer = new TileLayer({ source })
-    } else if (mapStyle.type === 'ogc-vt') {
-      const vectorTile = await createOGCVectorTileLayer(mapStyle.url, transformRequest, mapStyle)
-      tileLayer = vectorTile.layer
-      source = vectorTile.source
-    } else {
-      const vectorTile = await createVectorTileLayer(mapStyle.url, transformRequest, mapStyle)
-      tileLayer = vectorTile.layer
-      source = vectorTile.source
-    }
+    const { layer: tileLayer, source } = await createMapStyleLayer(mapStyle, transformRequest)
 
     const viewResolutions = getViewResolutionConfig(this.zoomAlignment ?? ZOOM_ALIGNMENT.UK)
 
@@ -97,18 +84,15 @@ export default class OpenLayersProvider {
 
     this.appEventHandles = attachAppEvents({
       mapProvider: this,
-      layer: tileLayer,
-      layerType: mapStyle.type ?? 'vector',
       transformRequest,
       events,
       eventBus,
-      map
+      map,
+      onBaseSourceChange: this.mapEventHandles.setSource
     }) || []
 
     this.map = map
     this.view = view
-    this.tileLayer = tileLayer
-    this.source = source
 
     // MAP_READY is synchronous — OL map is immediately interactive after construction
     eventBus.emit(events.MAP_READY, {
@@ -132,8 +116,6 @@ export default class OpenLayersProvider {
     }
 
     this.view = null
-    this.tileLayer = null
-    this.source = null
   }
 
   // ==========================