Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Once #697

Closed
wants to merge 11 commits into from
Closed

Once #697

wants to merge 11 commits into from

Conversation

@tbranyen
Copy link
Collaborator

@tbranyen tbranyen commented Oct 29, 2011

once method

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jan 13, 2012

I'm afraid I still don't think this is a common enough use case to bake into Backbone.Events ... but you can use _.once to accomplish the same thing, when you need it.

@jashkenas jashkenas closed this Jan 13, 2012
@powmedia
Copy link

@powmedia powmedia commented Mar 31, 2012

+1 for a proper .once() method which removes the event listener; could help prevent memory leaks

@TheCloudlessSky
Copy link

@TheCloudlessSky TheCloudlessSky commented Apr 26, 2012

+1 for this. @jashkenas - it's not the same thing since _.once would still keep the handler bound after the first time it's called. Here's my use case:

  1. Create a model.
  2. Bind once to the sync event to the model.
  3. Show a dialog and when closed, save the model.
  4. The once'd sync event is triggered (with { wait: true}) and the model is added to the collection.

For example:

var newAppointment = new mos.model.Appointment();
newAppointment.once('sync', function() {
  // Only add to the collection after it has been saved.
  collection.add(newAppointment);
});

var dialog = new mos.ui.AppointmentDialog({
  model: newAppointment,
  mode: mos.ui.DialogMode.CREATE
});

dialog.render(); // User then clicks 'Save' which would trigger "model.save()"
@braddunbar braddunbar mentioned this pull request Aug 25, 2012
@nibblebot
Copy link

@nibblebot nibblebot commented Sep 5, 2012

+1 @jashkenas please re-open this for consideration. _.once does not provide the same functionality

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Sep 5, 2012

I would, but GitHub seems to have disappeared the "reopen" button that used to be on these pages. If the button comes back, I'd be glad to reopen it.

@tbranyen tbranyen reopened this Sep 6, 2012
@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Sep 6, 2012

I re-opened this issue and rebased to merge cleanly. A few other things as well: I updated the source code to not modify the original callback that was passed in. This previous method didn't sit well with me, because it meant code like this wouldn't work:

var obj = _.extend({}, Backbone.Events);
var test = function() { console.log("triggered"); };
obj.once("test", test);

// Would not work.
obj.off("test", test);

This new method simply binds an additional "cleanup" event that removes immediately after the first event fires. @jashkenas @braddunbar code review of this would be awesome.

I also updated the qunit tests to use equal instead of equals all passing atm.

@gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented Sep 6, 2012

I like this one too, use it quite regularly. +1

@braddunbar
braddunbar reviewed Sep 6, 2012
View changes
backbone.js Outdated
// Bind the original event.
this.on(events, callback, context);
// Unbind the previous event and this after it is invoked.
this.on(events, unbind, context);

This comment has been minimized.

@braddunbar

braddunbar Sep 6, 2012
Collaborator

I believe the above should be this.on(events, unbind, this).

var context = {};
obj.on('event', function(){}, context);
// Dies because `unbind` is called with `context` instead of `obj`.
obj.trigger('event');

Also, I think the convention would be to return this, though I'm tempted to use the simpler form and return unbind for offing the handler.

@braddunbar
braddunbar reviewed Sep 6, 2012
View changes
backbone.js Outdated
// Bind an event like `on`, but unbind the event following the first trigger.
once: function(events, callback, context) {
// Ensure proper context.
context = context || this;

This comment has been minimized.

@braddunbar

braddunbar Sep 6, 2012
Collaborator

No need to default context here since it's done for us in trigger.

@braddunbar
Copy link
Collaborator

@braddunbar braddunbar commented Sep 6, 2012

Mornin' @tbranyen! The approach is interesting but I have to wonder, given the way once is essentially a callback, how often it would be useful to off the handler anyway. It's a good deal of overhead for something that can already be done with little effort and no extra cost.

model.on('event', function handler() {
  model.off('event', handler);
  // ...
}, model);

// ...

if (condition) model.off('event', handler);
@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Sep 6, 2012

@braddunbar Is it a common pattern to use a NFE inside of event binding to emulate once? Personally I haven't seen this, but a cursory glance at EventEmitter, EventEmitter2, Emitter, and jQuery all have a once implementation.

Also: Ember.js and Spine.js

@braddunbar
Copy link
Collaborator

@braddunbar braddunbar commented Sep 6, 2012

Good question. I only used the NFE because it was easier to type out in a comment.

@MFoster
Copy link

@MFoster MFoster commented Sep 6, 2012

@tbranyen +1. It's very convenient for setting off one time operations that need to respond once to a recurring event, such as a set up operation.

@nibblebot
Copy link

@nibblebot nibblebot commented Sep 6, 2012

@tbranyen Interesting. I hadn't considered the use of case of .off'ing a .once. I can see where you might want this flexibility but now we have 2x overhead for .once (2 .on handlers). I suppose I'd be willing to make that trade though :)

@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Sep 6, 2012

@nibblebot They are synchronous calls, the overhead isn't as bad at what you'd think. It's basically an additional function call which we were already doing.

@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Sep 13, 2012

@braddunbar I changed the implementation around, does this meet your concerns? There is almost no overhead with this new approach.

@braddunbar
Copy link
Collaborator

@braddunbar braddunbar commented Sep 13, 2012

@tbranyen Actually, I prefer the previous version. I think it's rather elegant and I'm ok with the extra call to off, especially since it's still possible to do it yourself if you like.

I know the extra cost to trigger is small, but I think that any changes we make to trigger should be to make it faster. It also doesn't handle obj.once('all', ...), which would add another check.

Thanks for the discussion and my apologies for the back and forth.

@tgriesser
Copy link
Collaborator

@tgriesser tgriesser commented Sep 13, 2012

@braddunbar did we determine if there'd be any potential issues with the NFE?

@braddunbar
Copy link
Collaborator

@braddunbar braddunbar commented Sep 13, 2012

I've looked through the NFE bugs and I don't see any potential issues, but I wouldn't mind if we broke it out just to be safe.

@jdalton
Copy link
Contributor

@jdalton jdalton commented Sep 18, 2012

NFE leak in IE < 9 because it creates 2 distinct function; 1 function declaration and 1 function expression.
See http://kangax.github.com/nfe/#jscript-memory-management

@philfreo
Copy link
Contributor

@philfreo philfreo commented Sep 29, 2012

👍 for once

@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Oct 15, 2012

Just found another place in my code in which once would be heavily desired.

// Special cases for when a parent View that has not been rendered is
// involved.
if (manager.parent && !manager.parent.__manager__.hasRendered) {
  return manager.parent.on("afterRender", function() {
    // Unbind this event... really wish we had once.
    manager.parent.off(null, null, this);

    // Trigger the afterRender and set hasRendered.
    completeRender();
  }, this);
}
@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Oct 18, 2012

Another situation where it would be much cleaner to have once:

        ctor.prototype.fetch = function(options) {
            // Set the nSync property to true indicating we are doing something.
            this.nSync = true;

            function reallyWantOnce() {
                this.off("reset", reallyWantOnce);
                this.nSync = false;
            }

            this.on("reset", reallyWantOnce, this);
@khepin
Copy link

@khepin khepin commented Oct 27, 2012

Definitely would be interesting for me like I already said in #594

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Oct 29, 2012

@tbranyen I sent you a PR (tbranyen#1) that accounts for once in the callback, avoiding a potential infinite loop. Test included too.

@jergason
Copy link

@jergason jergason commented Nov 2, 2012

Just another word of support: I would love the once functionality in Backbone events. It is in the node EventEmitter, and is very useful there.

@BlakeWilliams
Copy link

@BlakeWilliams BlakeWilliams commented Nov 6, 2012

+1

@grydstedt
Copy link

@grydstedt grydstedt commented Nov 14, 2012

+1

@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Nov 16, 2012

Anything I can do to get this merged? @jashkenas @braddunbar ?

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Nov 16, 2012

@tbranyen please take a look at my pull request on your branch. It prevents an easily avoidable more-than-once "once" case.

@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Nov 16, 2012

Awesome, updated @caseywebdev 👯

@caseywebdev
Copy link
Collaborator

@caseywebdev caseywebdev commented Nov 16, 2012

No prob! I love once =D 💃

@braddunbar
Copy link
Collaborator

@braddunbar braddunbar commented Nov 16, 2012

@tbranyen I posted some comments above, though I'll leave the merging to @jashkenas.

@tbranyen Actually, I prefer the previous version. I think it's rather elegant and I'm ok with the extra call to off, especially since it's still possible to do it yourself if you like.

I know the extra cost to trigger is small, but I think that any changes we make to trigger should be to make it faster. It also doesn't handle obj.once('all', ...), which would add another check.

Thanks for the discussion and my apologies for the back and forth.

Another reason for the previous version, is that the current version fails in the following instance:

var a = new Backbone.Model().once('event', f);
var b = new Backbone.Model().on('event', f);

a.trigger('event'); // Calls `f`
a.trigger('event'); // Noop

b.trigger('event'); // Calls `f`
b.trigger('event'); // Should call f, but doesn't

I submitted a pull request with a failing test case.

@RStankov
Copy link
Contributor

@RStankov RStankov commented Nov 17, 2012

+1

@tbranyen
Copy link
Collaborator Author

@tbranyen tbranyen commented Dec 4, 2012

@jashkenas @braddunbar Let me know if you have any further concerns before we merge it.

backbone.js Outdated
for (i = 0, length = list.length; i < length; i += 3) {
// Remove the special `once` event immediately before triggering.
if (list[i + 2]) {
delete calls[event];

This comment has been minimized.

@braddunbar

braddunbar Dec 4, 2012
Collaborator

Removing all callbacks for the event isn't appropriate here. The other events aren't necessarily once callbacks. Pull request with failing test case incoming.

var incrB = function(){ obj.counterB += 1 };
obj.once('event', incrA);
obj.once('event', incrB);
debugger;

This comment has been minimized.

@tgriesser

tgriesser Dec 7, 2012
Collaborator

leftover debugger

@@ -153,15 +162,21 @@

// Execute event callbacks.
if (list) {
console.log(calls[event]);

This comment has been minimized.

@braddunbar

braddunbar Dec 7, 2012
Collaborator

snip

@tgriesser
Copy link
Collaborator

@tgriesser tgriesser commented Dec 7, 2012

+1 - I've run into several cases where this would have been helpful to have.

@jashkenas jashkenas closed this in 7dbfecc Dec 7, 2012
@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Dec 7, 2012

Alright folks -- I've just pushed a patch that implements "once". Please take a very close look at it -- I think it's an elegant little implementation -- and add any additional test cases that you think I might be missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet