mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
11.2k stars 2.22k forks source link

Layer-specific handlers not removed when layer removed #7782

Open stevage opened 5 years ago

stevage commented 5 years ago

mapbox-gl-js version: 0.51

browser:

Steps to Trigger Behavior

map.addLayer({id: 'foo', ... })
map.on('click', 'foo', window.alert);
map.removeLayer('foo');
map.addLayer({id: 'foo', ... })

Expected Behavior

The handler is removed when the layer is removed

Actual Behavior

The handler is still present, so any new layer with the same ID has the same behaviour.layer now still shows alert.

I guess the documentation doesn't exactly promise that map.on(event, layername, f) ties the event to the layer, but that feels like the expectation to me. It was very surprising to me that old handlers stick around.

It's also problematic when you're repeatedly creating and destroying the same layer, because you end up with multiple copies of the handler.

Also, because there's no way to access existing handlers, nor any way to remove them other than by specifically passing the same handler, it becomes a bit annoying to actually have to manage the reference to these various handlers.

ParryQiu commented 5 years ago
function alert(){}
map.addLayer({id: 'foo', ... })
map.on('click', 'foo', alert);
map.off('click', 'foo', alert);
map.removeLayer('foo');

If you customize the layer that inherits from CustomLayerInterface , you can turn off all events in onRemove so that all event removal can be done when removeLayer .

stevage commented 5 years ago

map.off('click', 'foo', alert);

Yeah, I know you can do this, but actually managing the handler can be tricky, particularly if it's constructed by some other function.

I can't think of much benefit to the current situation. Would a user ever want/expect a handler to persist, despite removing a layer and adding another with the same id?

andycalder commented 5 years ago

I agree. I think it would be reasonable to consider this a bug.

brdv commented 3 years ago

Is there any update on this issue? Am running in to the same situation where we have to manually remove click events, which, for some reason, isn't even working..

Would be great to hear something about this!

WouterHeirstrate commented 2 years ago

@brdv did you ever solve this issue?

brdv commented 2 years ago

@brdv did you ever solve this issue?

Hi Wouter,

I remember the issue being that the mapbox package expected us to pass the exact same function/event (something to do with the location in memory). Since we used React, the function was repeatedly created again, causing the package to see it as a different function/event. We fixed it using the useCallback React hook to create our function/event, which caused the function to stay the 'exact same' each time.

I believe it all has something to do with memory and pointers, but I'm not that experienced with those subject.

I hope this can be of any help to you, good luck!

WouterHeirstrate commented 2 years ago

I hope this can be of any help to you, good luck!

Wow, this solved the issue immediately, thank you so much for your quick reply!

edcelmanuel commented 1 year ago

I hope this can be of any help to you, good luck!

Wow, this solved the issue immediately, thank you so much for your quick reply!

Good Day Sir how did you use the useCallback to clear the function inside the event?

dangelsaurus commented 4 months ago

oh boy, glad to see this isn't fixed yet.

bawallace15 commented 2 months ago

Somewhat new to mapbox-gl so apologies if this is stupid, but Is there a reason .off needs to work this way? It seems like a more usable approach would be for events to have a name or key and then off would work like this: .off(eventType, layerName, eventName/Key). that way you wouldn't have to create and store the reference so you can track between renders. the eventName/Key could be an arbitrary string provided with the event establishment and therefore wouldn't change between renders