off unbinds the correct bound function

[angus-c]

No description provided.

[tgvashworth]

(If you saw/got emails about my earlier comments, please ignore – wasn't concentrating)

This looks good, although I think off should remove all handlers, not just one at a time.

I'd also like to see include come docs about why this changed.

[angus-c]

@PhUU not getting this - if they specify a handler it just unbinds that one, if not it unbinds all

[angus-c]

merging this today unless objections

[GarrettS]

Where is the test for it? Thanks.

[angus-c]

@GarrettS https://github.com/flightjs/flight/blob/fe146ede46ee31eb8168445753ef63fb9a7de99f/test/spec/events_spec.js line 128

[tgvashworth]

@angus-c You're using callback.bound.some and returning on the first one found. That means, if one component registers the same event twice (with the same callback, on the same node), you won't remove all of them – so you have to call off once for every on. I think it should remove all matching events.

Seems you could use callback.bound = callback.bound.filter(...).

Here's a test that fails and demonstrates it:

it('unbinds all event handlers which share the same node, event, callback and instance', function () {
  var instance1 = (new Component).initialize(window.outerDiv);
  var spy = jasmine.createSpy();
  instance1.on(document, 'event1', spy);
  instance1.on(document, 'event1', spy);
  instance1.off(document, 'event1', spy);
  // uncomment this and the test passes
  // instance1.off(document, 'event1', spy);
  instance1.trigger('event1');
  expect(spy).not.toHaveBeenCalled();
});

I'm also interested: could we not have iterated on my PR (with the big commit message + explanation) to this?

[angus-c]

@PhUU because we need to be consistent with how registry and teardown works. if you bind the same event twice for the same element with the same handler (though why?) the registry would store that even t twice and teardown would call off twice. If we unbound both events in the first off, the second off would be left high and dry with nothing to do. Moreover there would be a danger of unbinding callbacks bound to other elements (but the same instance) even when such unbindings had not been requested.

I'm also interested: could we not have iterated on my PR (with the big commit message + explanation) to this?

We could have iterated on your PR but the emphasis was different, yours was entirely focused on registry.js, this one is focused on base.js

[GarrettS]

I see that you are no longer occasionally relying the guid feature of jQuery that is used internally. Good! That seemed very inconsistent and unreliable.

I would rather semi-formalize the context-bound callback. To turn this:-

callback = originalCb.bind(this);
callback.target = originalCb;
callback.context = this;

Into something more like this (untested):

// Untested.
callback = makeBoundCallback(originalCb, this);

function makeBoundCallback(originalCb, context) {
  function callback(ev) {
    originalCb.call(this.context, ev);
  }
  callback.originalCb = originalCb;
  callback.context = context;
  context = null;
  return callback;
}

[angus-c]

yes I've used this pattern in other repos and like it (can even have an additional prop which stores any curried args). I hope ES6 adds this data to bound functions for free

[tgvashworth]

I'm ok with this fix, although I don't much like adding to the callback. It seems to me the registry is where this should be focused as much as possible, as it's such a flight-specific thing, but this works too.