AmpersandJS / ampersand-events

Standalone event system that can be mixed into any object
MIT License
18 stars 7 forks source link

Feature idea: onlyOnce method #11

Open dhritzkiv opened 8 years ago

dhritzkiv commented 8 years ago

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?

cdaringe commented 8 years ago

interesting. i expected the behavior you expected as well. i'm not sure that this isn't a bug*

wraithgar commented 8 years ago

onlyOnce makes sense as a name to me, and I wonder if once with multiple names should log a warning. I for one would have fallen into the same assumption. Agreeing w/ @cdaringe that this could even be considered a bug (the fixing of which would probably necessitate a major version however)

dhritzkiv commented 8 years ago

The tests indicate that isn't not a insidious bug in as much as the tests specifically expect the callback to be called for each event name. (https://github.com/AmpersandJS/ampersand-events/blob/master/test/index.js#L528)

The reason for this expectation, I believe, is Backbone's events works this way (I found out through Stackoverflow, where other devs were confused by this behaviour too). Check out this assertion example (using Backbone event instead of ampersand-events) to see for yourselves

dhritzkiv commented 8 years ago

What really confuses me is the expected behaviour after reading through the code logic of ampersand-event's once:

    once: function (name, callback, context) {
        var self = this;
        var once = runOnce(function () {
            self.off(name, once);
            callback.apply(this, arguments);
        });
        once._callback = callback;
        return this.on(name, once, context);
    }

it seems that callback is being wrapped inside a function that also calls off to remove this particular listener for any future events for this name, and then that function is then being wrapped in lodash.once. Not sure if that's right.