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: remove event listeners when component is destroyed #1947

Merged

Conversation

arturovt
Copy link
Contributor

@arturovt arturovt commented Nov 5, 2024

We should manually remove event listeners to prevent memory leaks.

We should manually remove event listeners to prevent memory leaks.
@KillerCodeMonkey
Copy link
Owner

But since the listeners are bind to native Dom elements they should be destroyed, when the Dom element gets destroyed. So I guess there is no memory leak?

@arturovt
Copy link
Contributor Author

arturovt commented Nov 6, 2024

No, they're not automatically removed when DOM nodes are removed. They're marked as dangling references preventing the DOM from being garbage collected too.

@KillerCodeMonkey
Copy link
Owner

KillerCodeMonkey commented Nov 6, 2024

But there should not any references to the dom node itself and then the listeners gets removed automatically, as far as i know.

and since the parent of those nodes are removed the listeners should removed as well.

PS: i just want to understand where the memory should come from. I am glad you are caring a lot about this library :)

@KillerCodeMonkey
Copy link
Owner

KillerCodeMonkey commented Nov 6, 2024

okay i know a little how garbage collection works, but in a short way:

since we are not knowing what a programmer is doing with quill and its instance we can not be sure, that the eventlistener gets remove when the domnode is removed, because the node could persist in memory as well.

So this is a memory leak that can occur but is per se not a memory leak directly occurring when using ngx-quill, because there are those 3 event listeners, right?

So it is more a safety handling that this will not lead to a memory leak issue, right?

@arturovt
Copy link
Contributor Author

arturovt commented Nov 6, 2024

No, this is a leak directly caused by ngx-quill.

@KillerCodeMonkey
Copy link
Owner

KillerCodeMonkey commented Nov 6, 2024

strange... i do not get it in my head, because then quilljs or someone else need to hold some references on the editor instance?

because if the component gets destroyed there are no outer references until it is stored somewhere where it is used?

@KillerCodeMonkey KillerCodeMonkey merged commit 8dc5f4e into KillerCodeMonkey:master Nov 6, 2024
1 check passed
@arturovt
Copy link
Contributor Author

arturovt commented Nov 6, 2024

Screenshot from 2024-11-06 16-00-35


Screenshot from 2024-11-06 16-00-40

@arturovt arturovt deleted the fix/event-listeners branch November 6, 2024 14:05
@KillerCodeMonkey
Copy link
Owner

@arturovt thanks :). seems like using this.quillEditor inside the callback is enough to create the dep-cycle. so even if the editor instance is removed it stays in memory, because the handler is referencing the it. And so the instance itselfs stays there.

now i got it.

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