Unregister event handler #127
Conversation
|
Changes Unknown when pulling 4464ce9 on sophypal:fix-leak into ** on ciena-frost:master**. |
| }, | ||
|
|
||
| willDestroy () { | ||
| $(document).off(`keyup.${this.elementId} keydown.${this.elementId}`, this.setShift.bind(this)) | ||
| $(document).off(`keyup.${this.elementId} keydown.${this.elementId}`, this._keyHandler) |
There was a problem hiding this comment.
Any way to add a unit test for this? Like maybe stubbing out setShift.bind to return something different in willDestroy to make sure it's the instance from before that's passed into off?
There was a problem hiding this comment.
Integration test added.
addon/components/frost-list.js
Outdated
| if (!this.isDestroyed) { | ||
| this.set('_isShiftDown', event.shiftKey) | ||
| } | ||
| run.next(() => { |
There was a problem hiding this comment.
Previously holding shift while the test would run would cause errors. This fixes that.
|
Changes Unknown when pulling afd94b6 on sophypal:fix-leak into ** on ciena-frost:master**. |
addon/components/frost-list.js
Outdated
| this.set('_isShiftDown', event.shiftKey) | ||
| } | ||
| run.next(() => { | ||
| if (!this.isDestroyed) { |
There was a problem hiding this comment.
You probably want:
if (this.isDestroyed || this.isDestroying) return
this.set('_isShiftDown', event.shiftKey)
There was a problem hiding this comment.
I don't think this.set will cause errors if the component is being destroyed but I can update it.
| @@ -150,11 +152,12 @@ export default Component.extend({ | |||
| }) | |||
| } | |||
|
|
|||
| $(document).on(`keyup.${this.elementId} keydown.${this.elementId}`, this.setShift.bind(this)) | |||
| this._keyHandler = this.setShift.bind(this) | |||
There was a problem hiding this comment.
I'd personally prefer you name it this._keyUpHandler to be more specific.
There was a problem hiding this comment.
Except that it's for both keyup and keydown events.
| @@ -150,11 +152,12 @@ export default Component.extend({ | |||
| }) | |||
| } | |||
|
|
|||
| $(document).on(`keyup.${this.elementId} keydown.${this.elementId}`, this.setShift.bind(this)) | |||
| this._keyHandler = this.setShift.bind(this) | |||
| $(document).on(`keyup.${this.elementId} keydown.${this.elementId}`, this._keyHandler) | |||
There was a problem hiding this comment.
I assume there are no scenarios in which the elementId can change during a components life cycle? Otherwise this would be problematic as the .off() could be for different events than .on().
There was a problem hiding this comment.
elementId shouldn't change for the same component instance.
|
Changes Unknown when pulling 0b339ff on sophypal:fix-leak into ** on ciena-frost:master**. |
|
👍 |
CHANGELOG
keyup/downevents.