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

fix global keyboard and window listeners are not removed after the map is destroyed #1434

Conversation

plainheart
Copy link
Contributor

Hi,

I noticed the global listeners bound in the keyboard mixin were not unbound after the map was destroyed, which makes the listener always exist and makes the code throw errors even if the component is destroyed in those single-page applications.

So I think we should unbind the listeners somewhere after destroying the map. I currently put this logic into the _initKeyListener function of the Keyboard mixin. Also, to keep the isolation of the mixin, I changed the singleton Keyboard mixin object to a function that returns a new object at each call to make it work for multiple map instances, or the entire listener will be removed as long as one map is destroyed, which causes the event listener for other maps to not work.

To reproduce,

Copy link
Collaborator

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

Thank you, good catch!

I will approve this but are you able to add some tests?

And do you think we have with some other Mixins problems when there are multiple maps?

@plainheart
Copy link
Contributor Author

plainheart commented Feb 4, 2024

@Falke-Design Just added a simple test case but I'm not sure if it's enough.

And do you think we have with some other Mixins problems when there are multiple maps?

It looks other mixins are not binding events on the global objects like window and document. There might be no obvious problem with them. One suggestion is to provide an API to allow users to unmount (or destroy/remove/dispose) the PM plugin and clean up the relative resources like listeners and mounted objects. - This may need a considerable amount of job.

Copy link
Collaborator

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

This test is enough for now. Please check out the comments

src/js/Mixins/Keyboard.js Outdated Show resolved Hide resolved
cypress/integration/mixins.spec.js Show resolved Hide resolved
cypress/integration/mixins.spec.js Outdated Show resolved Hide resolved
src/js/Mixins/Keyboard.js Outdated Show resolved Hide resolved
Co-authored-by: Florian Bischof <[email protected]>
Copy link
Collaborator

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@Falke-Design Falke-Design merged commit 77b6545 into geoman-io:develop Feb 4, 2024
2 checks passed
@plainheart plainheart deleted the fix/mixin-keyboard/unbind-global-listeners-after-map-destroyed branch February 4, 2024 16:17
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.

2 participants