Always clean up event listener on binding disconnect#845
Open
dfritsch wants to merge 1 commit intohotwired:mainfrom
Open
Always clean up event listener on binding disconnect#845dfritsch wants to merge 1 commit intohotwired:mainfrom
dfritsch wants to merge 1 commit intohotwired:mainfrom
Conversation
Updates the binding observer and dispatch logic to always clean up the event listeners. Without this change, removing an element _inside_ a controller can cause the bindings on that element to be removed but the event listeners are left dangling. Even if the controller is eventually also removed, these events will never be cleaned up, so these elements will remain in memory indefinitely. Based on the tests, the one downside of this is that if you adjust an event that was set to once and move it to a different order on the element, that action now has a different index so will disconnect the old binding and add a new binding. Previously this would find the old event listener that wasn't properly cleaned up and re-use it, keeping the "once" logic. Actions which are added on top of the existing action would keep the same event, so this still partly works. `clearEventListeners` would be passed as `true` in all cases with this update, so the parameter is removed.
dfritsch
commented
Jun 2, 2025
|
|
||
| private disconnectAllActions() { | ||
| this.bindings.forEach((binding) => this.delegate.bindingDisconnected(binding, true)) | ||
| this.bindings.forEach((binding) => this.delegate.bindingDisconnected(binding)) |
Author
There was a problem hiding this comment.
The actual "fix" is adding true to line 70 above. However, this makes all cases true, so instead the option is removed entirely.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the binding observer and dispatch logic to always clean up the event listeners.
Without this change, removing an element inside a controller can cause the bindings on that element to be removed but the event listeners are left dangling. Even if the controller is eventually also removed, these events will never be cleaned up, so these elements will remain in memory indefinitely.
Based on the tests, the one downside of this is that if you adjust an event that was set to once and move it to a different order on the element, that action now has a different index so will disconnect the old binding and add a new binding. Previously this would find the old event listener that wasn't properly cleaned up and re-use it, keeping the "once" logic. Actions which are added on top of the existing action would keep the same event, so this still partly works.
clearEventListenerswould be passed astruein all cases with this update, so the parameter is removed.Fixes #838