visgl / deck.gl

WebGL2 powered visualization framework
https://deck.gl
MIT License
12.25k stars 2.08k forks source link

[Bug] MapboxOverlay adds map move listener that is not removed when instance is finalized #7532

Closed AdamRatcliffe-TomTom closed 1 year ago

AdamRatcliffe-TomTom commented 1 year ago

Description

In deck-utils getDeckInstance a move listener is added but when finalize is called on the overlay the listener is not removed. In my use case the overlay may be removed before the end of the map lifetime, after the overlay is removed moving the map will result in an error being thrown in Deck.setProps:

deck.ts:456 Uncaught TypeError: Cannot read properties of null (reading 'setProps')
    at Deck.setProps (deck.ts:456:1)
    at onMapMove (deck-utils.ts:229:1)
    at i.<anonymous> (deck-utils.ts:66:1)
    at It.fire (maps.min.js:1:1)
    at gi._fireEvent (maps.min.js:1:1)
    at gi._fireEvents (maps.min.js:1:1)
    at gi._updateMapTransform (maps.min.js:1:1)
    at gi._applyChanges (maps.min.js:1:1)
    at Object.callback (maps.min.js:1:1)
    at _i.run (maps.min.js:1:1)
    at i._render (maps.min.js:1:1)
    at maps.min.js:1:1

Flavors

Expected Behavior

Map move listener is removed when Deck instance is finalized.

Steps to Reproduce

Add a MapboxOverlay to the Mapbox GL map. Call finalize on the overlay instance. Move the map to trigger the error.

Environment

Logs

No response

YonatanHanan commented 1 year ago

I think i get the same error when calling map.jumpto have you found a solution?

AdamRatcliffe-TomTom commented 1 year ago

@YonatanHanan unfortunately I have not.

YonatanHanan commented 1 year ago

So using the documentation https://deck.gl/docs/api-reference/mapbox/mapbox-overlay#using-with-react-map-gl did not work for me when trying to jumpto but when I added the MapboxOverlay as a control manually it did work. Could that mean I have a different problem?

AdamRatcliffe-TomTom commented 1 year ago

@YonatanHanan the problem occurs when the MapboxOverlay is added to the map and at some later time finalized before the end of the map lifetime, either through explicitly calling finalize on the control or through removing the control from the map. If that's not your situation you may have a different problem.

chrisgervang commented 1 year ago

Calling overlay.finalize() doesn't remove the map, so the map's events will still be registered. Calling map.remove() will clean up all event bindings, and deck will also be finalized.

In your use case will you be adding and removing the deck overlay multiple times onto one map instance?

AdamRatcliffe-TomTom commented 1 year ago

@chrisgervang, if you're asking about my case, the overlay is removed in response to a state change but not added back to the map again.

chrisgervang commented 1 year ago

Do you remove the overlay manually by either calling overlay.finalize() or map.removeControl(overlay), or is there another mechanism for removing it?

AdamRatcliffe-TomTom commented 1 year ago

I'm using it in React and call map.removeControl(overlay) when the component is unmounted.

Pessimistress commented 1 year ago

Fix landed in v8.8.27