7 ms·

The unfortunate thing about the lengths people go to in order to try to "fix" `this` in event handlers is that it wouldn't be a problem if people just implement

The unfortunate thing about the lengths people go to in order to try to "fix" `this` in event handlers is that it wouldn't be a problem if people just implemented the event listener interface correctly. PSA Don't do this: proto.foo = function() { /* ... */ var self = this; element.addEventListener("click", (function(event) { self.iHaveAnUnhealthyObsessionWithLambdas(event.target); })); } proto.iHaveAnUnhealthyObsessionWithLamdas = function(clicked) { /* do something with clicked element `clicked` */ } Do this instead: proto.foo = function() { /* ... */ element.addEventListener("click", this); } proto.handleEvent = function(event) { /* do something with clicked element `event.target` */ }

thousande 9y ago
Today I learned In general, a good practice is to avoid anonymous functions for the listener else it is not possible to remove the event later.
- wiredearp 9y ago
  I guess the function could still be anonymous, but you will at least need to keep it assigned to some variable or property name. If you make use of the `handleEvent` pattern that @carussell suggested, you can however avoid having to keep track of all these callback functions since the listener can always be removed by passing `this` a second time: ['click', 'focus', 'blur'].forEach(type => elm.removeEventListener(type, this));
icebraining 9y ago
But then if you're listening to multiple events, won't you have to try to disambiguate after the fact in handleEvent()?
- wiredearp 9y ago
  That's the fun part, because here you can code your event handling to read something more like a state machine. Here's a quick example in bogus code, adding and removing event listeners as we go along (and without keeping reference to all the callback functions): class ElementDragger { constructor(elm) { elm.addEventListener('mousedown', this); } handleEvent(e) { const elm = e.target; switch(e.type) { case 'mousedown': elm.removeEventListener('mousedown', this). elm.addEventListener('mousemove', this); elm.addEventListener('mouseup', this); break; case 'mousemove': elm.style.left = e.clientX + 'px'; break; case 'mouseup': elm.removeEventListener('mousedown', this); elm.removeEventListener('mouseup', this); elm.addEventListener('mousedown', this); break; } } }
  - richthegeek 9y ago
    That seems complicated, fragile, and bad for performance in a DOM. Event bindings are slow (or at least, used to be). I don't know the purpose unbinding mousedown on mousedown - it only fires once per mousedown doesn't it? Why not just stick with `elm.addEventListener('mousedown', this.handleMouseDown.bind(this))`? Where's the benefit other than making things more verbose?
    - rayshan 9y ago
      Bind is less performant, because bind creates a new function with new context. At the framework level, bind is avoided at all costs. @wiredearp's approach is correct, because it only adds a single event listener, everything else is conditional.
    - carussell 9y ago
      If you think the above code has bad performance because "event bindings are slow" then your preferred form using `bind` is going to have all of that and more; it sidesteps nothing (except extremely fast switch case matching) and only adds overhead, both in runtime and memory costs. > Where's the benefit other than making things more verbose? To be clear what you're asking, are you saying that: elm.addEventListener('mousedown', this); ... is more verbose than: elm.addEventListener('mousedown', this.handleMouseDown.bind(this))
      - yarrel 9y ago
        No, that a switch statement is more verbose than a single function. Switch statements in OOP are a sign that something is wrong.
        wiredearp 9y ago
        It's however easy to read and that feels good if not right, since the common solutions to this imagined problem require substantial mental overhead as seen on https://hackernoon.com/rethinking-javascript-eliminate-the-switch-statement-for-better-code-5c81c044716d https://hackernoon.com/rethinking-javascript-eliminate-the-s.... To promote "single functions" we could: handleEvent(e) { this[e.type](e, e.target); } mousemove(e, elm) { elm.style.left = e.clientX + 'px'; } ... but then the discussion would almost certainly revolve around this questionable decision instead of the original topic.
janci 9y ago
Better yet element.addEventListener("click", self.handleClick.bind(this))
- rayshan 9y ago
  This is less performant, because bind creates a new function with new context. At the framework level, bind is avoided at all costs.
  - janci 9y ago
    The overhead is not important, if you bind once. It becomes problem, if you do it too often, like in react render function.
amelius 9y ago
The problem is that the approach is different depending on context. You are making use of the assumption that the addEventListener function knows to call the "handleEvent" method of the object in the second argument, and give it a proper "this" object. That's a big assumption, which should be documented in the code. The code using "self" doesn't have this problem; it's more self-contained so to speak (no pun intended).
- kitd 9y ago
  Agreed. Much prefer 'var self = this' style. Keep the model transparent for future maintenance. One time I don't is in Jasmine, where they explicitly state that 'this' is shared across all 'beforeEach', 'it' and 'afterEach' calls. Which backs up your first point.
  - nkozyra 9y ago
    I thought self had gone out of favor due to its relatively new global purpose.
    - kitd 9y ago
      ok, call it something else. I meant to say, save away the current ref to 'this'.
  - scruple 9y ago
    This is where I've landed with the JavaScript that I write. It keeps things relatively sane, at least better than the other approaches I've tried.
- carussell 9y ago
  > That's a big assumption No, it's not. At all. It's part of the DOM Level 2 standard that specifies what `addEventListener` does and has been for going on 20 years. https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-...

thousande 9y ago

Today I learned In general, a good practice is to avoid anonymous functions for the listener else it is not possible to remove the event later.

wiredearp 9y ago
I guess the function could still be anonymous, but you will at least need to keep it assigned to some variable or property name. If you make use of the `handleEvent` pattern that @carussell suggested, you can however avoid having to keep track of all these callback functions since the listener can always be removed by passing `this` a second time: ['click', 'focus', 'blur'].forEach(type => elm.removeEventListener(type, this));

icebraining 9y ago

But then if you're listening to multiple events, won't you have to try to disambiguate after the fact in handleEvent()?

wiredearp 9y ago
That's the fun part, because here you can code your event handling to read something more like a state machine. Here's a quick example in bogus code, adding and removing event listeners as we go along (and without keeping reference to all the callback functions): class ElementDragger { constructor(elm) { elm.addEventListener('mousedown', this); } handleEvent(e) { const elm = e.target; switch(e.type) { case 'mousedown': elm.removeEventListener('mousedown', this). elm.addEventListener('mousemove', this); elm.addEventListener('mouseup', this); break; case 'mousemove': elm.style.left = e.clientX + 'px'; break; case 'mouseup': elm.removeEventListener('mousedown', this); elm.removeEventListener('mouseup', this); elm.addEventListener('mousedown', this); break; } } }
- richthegeek 9y ago
  That seems complicated, fragile, and bad for performance in a DOM. Event bindings are slow (or at least, used to be). I don't know the purpose unbinding mousedown on mousedown - it only fires once per mousedown doesn't it? Why not just stick with `elm.addEventListener('mousedown', this.handleMouseDown.bind(this))`? Where's the benefit other than making things more verbose?
  - rayshan 9y ago
    Bind is less performant, because bind creates a new function with new context. At the framework level, bind is avoided at all costs. @wiredearp's approach is correct, because it only adds a single event listener, everything else is conditional.
  - carussell 9y ago
    If you think the above code has bad performance because "event bindings are slow" then your preferred form using `bind` is going to have all of that and more; it sidesteps nothing (except extremely fast switch case matching) and only adds overhead, both in runtime and memory costs. > Where's the benefit other than making things more verbose? To be clear what you're asking, are you saying that: elm.addEventListener('mousedown', this); ... is more verbose than: elm.addEventListener('mousedown', this.handleMouseDown.bind(this))
    - yarrel 9y ago
      No, that a switch statement is more verbose than a single function. Switch statements in OOP are a sign that something is wrong.
      - wiredearp 9y ago
        It's however easy to read and that feels good if not right, since the common solutions to this imagined problem require substantial mental overhead as seen on https://hackernoon.com/rethinking-javascript-eliminate-the-switch-statement-for-better-code-5c81c044716d https://hackernoon.com/rethinking-javascript-eliminate-the-s.... To promote "single functions" we could: handleEvent(e) { this[e.type](e, e.target); } mousemove(e, elm) { elm.style.left = e.clientX + 'px'; } ... but then the discussion would almost certainly revolve around this questionable decision instead of the original topic.

janci 9y ago

Better yet element.addEventListener("click", self.handleClick.bind(this))

rayshan 9y ago
This is less performant, because bind creates a new function with new context. At the framework level, bind is avoided at all costs.
- janci 9y ago
  The overhead is not important, if you bind once. It becomes problem, if you do it too often, like in react render function.

amelius 9y ago

The problem is that the approach is different depending on context. You are making use of the assumption that the addEventListener function knows to call the "handleEvent" method of the object in the second argument, and give it a proper "this" object. That's a big assumption, which should be documented in the code. The code using "self" doesn't have this problem; it's more self-contained so to speak (no pun intended).

kitd 9y ago
Agreed. Much prefer 'var self = this' style. Keep the model transparent for future maintenance. One time I don't is in Jasmine, where they explicitly state that 'this' is shared across all 'beforeEach', 'it' and 'afterEach' calls. Which backs up your first point.
- nkozyra 9y ago
  I thought self had gone out of favor due to its relatively new global purpose.
  - kitd 9y ago
    ok, call it something else. I meant to say, save away the current ref to 'this'.
- scruple 9y ago
  This is where I've landed with the JavaScript that I write. It keeps things relatively sane, at least better than the other approaches I've tried.
carussell 9y ago
> That's a big assumption No, it's not. At all. It's part of the DOM Level 2 standard that specifies what `addEventListener` does and has been for going on 20 years. https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-...