Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly remove controls from the map on map unload #6488

Merged
merged 1 commit into from Jan 22, 2019

Conversation

IvanSanchez
Copy link
Member

This is a blind attempt (i.e. untested) to fix #6487. A map being destroyed will fire the unload event, and any controls from that map can use that event to remove themselves from the map.

@callumacrae
Copy link

Sweet, onRemove is being called now 👍

@callumacrae
Copy link

callumacrae commented Jan 22, 2019

It looks like layers have the same problem (untested, just looked at the code) - should the same code be added there too?

Copy link
Sponsor Collaborator

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@IvanSanchez
Copy link
Member Author

Sweet, onRemove is being called now 👍

I'm more worried about the memory leaks - does this help?

It looks like layers have the same problem (untested, just looked at the code) - should the same code be added there too?

No need, because

Leaflet/src/map/Map.js

Lines 797 to 799 in 8108ff9

for (i in this._layers) {
this._layers[i].remove();
}

@callumacrae
Copy link

callumacrae commented Jan 22, 2019

I'm more worried about the memory leaks - does this help?

It helps in the example I posted, it doesn't help (measurably) in the actual application I'm working on - which probably just means the problem is somewhere else, probably outside of leaflet. I'll carry on investigating and keep you posted if I find anything else related to leaflet!

@IvanSanchez
Copy link
Member Author

It helps in the example I posted,

That's good enough for me! 😄

@IvanSanchez IvanSanchez merged commit 61ef177 into master Jan 22, 2019
@cherniavskii cherniavskii deleted the unload-controls branch January 22, 2019 17:09
@callumacrae
Copy link

Thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

control.onRemove isn't called when map.remove() is called
3 participants