ampersandjs / ampersand-events Goto Github PK

Given that most projects of moderate size are likely to have a transitive dependency on events.EventEmitter, would it make sense to reimplement ampersand-events as an augmentation thereof? I imagine this would reduce both code size and cognitive load (dealing with subtly different event emitters is surprisingly problematic).

Implementing, for example, a draggable element would be so nice like this:

const DraggableView = AmpersandView.extend ({
  ...
  events: {
    'mousedown': 'down', // start dragging and set listeners
  },
  down: function (ev) {
    ...
    // we need to listen on window to handle panning outside the browser
    this.listenTo (window, 'mousemove', this.move)
    this.listenTo (window, 'mouseup', this.up)
  },
  move: function (ev) {
    ...
  },
  up: function (ev) {
    ...
    // this is way elegant!
    this.stopListening (window, 'mousemove')
    this.stopListening (window, 'mouseup')
  },
})

Being able to do this would have huge advantages over other solutions:

• Defining mousemove and mouseup in the events hash causes atrocities like the element being left behind if the mouse if moved too fast, or being stuck dragging if the mouse is moved outside the window.
• Using .listenTo preserves the context for this in the callback function.
• The event listeners would be removed when the view was destroyed.
• The .stopListening is way more elegant than the DOM's .removeEventListener

I think implementing this should be just as simple as modifying ampersand-events to allow .listenTo and .stopListening on DOM elements. I think the code to stop listening on destruction is already in place and wouldn't need modification.

What do you think? Surely you cannot deny the awesomeness of what this would allow you to do; is this as simple to implement as it seems?

I'm considering using all Ampersand modules and creating a fake backbone module with the following so that I can use backbone.radio.

module.exports = {
    Events: require('ampersand-events')
}

Will this work?

I recently ran into a small but confusing bug in my app while using ampersand-events (or rather, one of its dependents). I was using the once method to listen to two event names with one callback, expecting it to work like this:

var count = 0;
model.once("error success", function() {
    //I assumed, incorrectly, that this callback would fire only once, ever.
    count++;
});
model.trigger("error");
model.trigger("success");
assert.equal(count, 1); // nope. count actually equals `2`

see live example on TonicDev

But I misunderstood the functionality. There were rare cases where both success and error events could happen while those listeners were in place causing unexpected results based on the callback being called multiple times.

While this behaviour is by design –as a convenience method for calling once for multiple event names–, and while this could be achieved by wrapping the callback function using lodash.once, I believe there is room in this library for a method that only ever triggers for a group of events. It can be called onlyOnce or firstOnce or similar. The implementation would be minimal: simply wrap the callback in lodash.once and calling the regular once with the newly wrapped callback.

Thoughts?

I think it's pretty much ready to go. Can I get some review from @AmpersandJS/core-team so we can ship it?

What do you think about adding .emit () as an alias for .trigger (). This would allow it to have a signature very similar to NodeJS's EventEmitter class which would be nice for server-side code.

Along with this might come aliasing .removeListener () to .off ().

What do you &yet-ers think about this? Is this kind of uniformity worth doing for you?

^{P.S. I'm LOVING AmpersandJS, using it everywhere from my API on Node to the app in the browser. Keep up the good work!}

Here is a proof of the behaviour (or see this notebook on TonicDev)

const AmpersandState = require("ampersand-state");

const Model = AmpersandState.extend({});

const model = new Model();
const someObject = {};

//passing a context argument to `on` correctly applies the context to the callback
model.on("foo", function testThisEqualsModel2() {
    console.log("foo event via `on`; this === someObject");
    console.log(this === someObject);
}, someObject);

//... but doing the same for `once` doesn't work.
model.once("bar", function testThisEqualsModel2() {
    console.log("bar event via `once`; this === someObject")
    console.log(this === someObject);
}, someObject);

model.trigger("foo");
// "foo event via `on`; this === someObject"
// true

model.trigger("bar");
// "bar event via `once`; this === someObject"
// false

The potential problem code in ampersand-events.js (L24):

   once: function (name, callback, context) {
        if (!utils.eventsApi(this, 'once', name, [callback, context]) || !callback) return this;
        var self = this;
        var once = runOnce(function () {
            self.off(name, once);
            callback.apply(self, arguments);
        });
        once._callback = callback;
        return this.on(name, once, context);
    },

Note the callback is being called via apply(self, arguments), instead of something like apply(context, arguments).

This currently affects AmpersandJS/ampersand-view-switcher#35