Various clean up and optimizations

[wyuenho]

No description provided.

[wyuenho]

I'm not quite sure what to do with undelegateEvents. Can't change the check to this.el or fail a bunch of tests. But not having an _undelegate is inconsistent.

[akre54]

Doesnt this test fail? I'm ok with the new method signature, but I rather like the no second argument form.

[wyuenho]

Nope test passed.

[akre54]

check it again... the wrong logic branch gets called (it's why I had the _.isFunction check there)

[akre54]

@wyuenho Ok, so with this pull, the methods a native subview would need to override would be $, _createContext, _removeElement, _delegate, and undelegateEvents, right?

[wyuenho]

Yep.

[wyuenho]

Still deciding on undelegateEvents but that's bikeshedding. It's icky but meh.

[wyuenho]

Also, I don't think _delegate should be advertised as delegate for external usage. The logic inside is still very implementation specific and situational with the eventName and selector branching and all that, so that's why I remove the isFunction check.

[wyuenho]

But let me fix the test first.

[wyuenho]

Turns out the 3rd param to $.fn.on is ignored so we are ok.

[wyuenho]

@akre54 This PR should be good. We can continue the discussion on the ticket once this is merged.

[akre54]

Turns out the 3rd param to $.fn.on is ignored so we are ok.

Nah, method is undefined in the two argument form. You need the isFunction check if we want the two argument form.

I'm not sold on _createContext, or at least the way it's applied here.

[akre54]

I'm warming up to _removeElement, I'll think about solutions to _createContext...

[akre54]

This adds an extra call to $.fn.off on the element, and creates a potentially very slow path on what is supposed to be hot code. Where previously $.fn.off wouldn't have to do any mucking in namespaces since it's about to be removed, now it has to do both a slow and fast listener remove.

I'm in favor of letting the native view be responsible for removing events before deleting the element.

[wyuenho]

You are right this is slower, but remember this isn't the hot path. The hot path is context creation and event delegation.

Sent from my iPhone

On 20 Feb, 2014, at 9:20 pm, Adam Krebs [email protected] wrote:

In backbone.js:

 remove: function() {

•  this.$el.remove();
  

•  this.undelegateEvents();
  

  This adds an extra call to $.fn.off on the element, and creates a potential slow path. Where previously it wouldn't have to do any mucking in namespaces since it's about to be removed, now it has to do both a slow and fast remove.

I'm in favor of letting the native view be responsible for removing events before deleting the element.

—
Reply to this email directly or view it on GitHub.

[akre54]

No, but potentially if you're deleting many rows, items, whatever, while creating new ones constantly it will be a hot path. That's what this PR was supposed to address.

[wyuenho]

Yep. Just trying the find the right balance of abstraction and performance. When that extra function becomes a problem, you can still override remove. But trust me, no matter how you shake it, that one call to parent.removeChild's got to be way faster then jquery to offset the cost. I can run some jsperf later tonight when I get home.

Sent from my iPhone

On 20 Feb, 2014, at 9:28 pm, Adam Krebs [email protected] wrote:

In backbone.js:

 remove: function() {

•  this.$el.remove();
  

•  this.undelegateEvents();
  

  No, but potentially if you're deleting many rows, items, whatever, while creating new ones constantly it will be a hot path. That's what this PR was supposed to address.

—
Reply to this email directly or view it on GitHub.

[akre54]

The main reason I want delegate exposed is for plugins like Stickit and Rivets that add their own bindings. They shouldn't need to rely on $el to do event attachment.

[wyuenho]

Nah, method is undefined in the two argument form. You need the isFunction check if we want the two argument form.

You sure? You aren't checking selector is '' anymore, you are checking for truthiness. Which is false, which falls to the second branch, in which case selector is actually method, and method is undefined.

The test really checks out dude.

[wyuenho]

The main reason I want delegate exposed is for plugins like Stickit and Rivets that add their own bindings. They shouldn't need to rely on $el to do event attachment.

Let me take a look.

[akre54]

The test is view._delegate('click', view.increment);, which hits the second logic branch. I guess the variables are just misnamed then.

Are you saying we should add back in the if (selector !== '') check and let jQuery work out the type finding itself? (it will be passed $el.on(eventName, fn, undefined)).

[wyuenho]

Just let jQuery deal with it. I just put a comment in.

[wyuenho]

So apparently Rivet uses it's own DOM event listen shims, nothing but BaseView can save them from repeating this logic :)

Stickit seems to be doing event delegation all over again. They can still use _delegate(). It's JS, do whatever you want if you know what you are doing.

Did I miss anything?

[akre54]

They can still use _delegate()

The minified version will mangle an underscore-prefixed method to some shorthand, making it unreliable to hook into.

I'd like to create a common interface that any plugin can hook into. And delegate / undelegate seem pretty important.

[wyuenho]

The minified version will mangle an underscore-prefixed method to some shorthand, making it unreliable to hook into.

Huh? Which minifier are you talking? Uglify doesn't do it. In fact I don't think any sane minifier will do this unless you explicitly give an option (if there's such an option). Remember that the method names are just keys of an object to the compiler. You can't just change the keys arbitrarily without being able to chase down every reference of it, to do that you need to run the program to find out, which you can't do inside a compiler - halting problem.

[akre54]

Whups, you're right. You have to pass an option.