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.

Sign up for GitHub

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

Various clean up and optimizations #5

Merged
merged 13 commits into from
Feb 21, 2014
Merged

Various clean up and optimizations #5

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
19d82b8
Move delegate up and move check into undelegateEvents
wyuenho Feb 19, 2014
846de91
Speed up method calls in delegateEvents and delegate
wyuenho Feb 19, 2014
54252e6
Further isolate $.remove by exposing removeElement in case subclasses…
wyuenho Feb 19, 2014
33e23df
Rename make to _createContext and have _ensureElement and setElement …
wyuenho Feb 19, 2014
8ab8646
Prefix all hooks with _ and introduce _undelegate
wyuenho Feb 19, 2014
31bb428
Revert introduction of _undelegate
wyuenho Feb 19, 2014
15cb64e
Shortened _createContext
wyuenho Feb 20, 2014
09d3762
_createContext is side-effect only
wyuenho Feb 20, 2014
f872032
Disallow chaining in hook calls
wyuenho Feb 20, 2014
7c46856
Fix comment
wyuenho Feb 20, 2014
aca58b7
No reason jQuery can't be a little faster too
wyuenho Feb 20, 2014
e14a903
Fix typo
wyuenho Feb 20, 2014
be3a354
Comment for 2 arg form inside _delegate
wyuenho Feb 20, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 49 additions & 31 deletions backbone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,20 +1031,29 @@
return this;
},

// Remove this view by taking the element out of the DOM, and removing any
// applicable Backbone.Events listeners.
// Remove this view by taking the element out of the document, remove all
// the DOM event listeners attached to it, and remove any applicable
// Backbone.Events listeners.
remove: function() {
this.$el.remove();
this.undelegateEvents();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this._removeElement();
this.stopListening();
return this;
},

// Remove this view's element from the document and remove all the event
// listeners attached to it. Useful for subclasses to override in order to
// utilize an alternative DOM manipulation API.
_removeElement: function() {
this.$el.remove();
return this;
},

// Change the view's element (`this.el` property), including event
// re-delegation.
setElement: function(element, delegate) {
if (this.$el) this.undelegateEvents();
this.$el = element instanceof Backbone.$ ? element : Backbone.$(element);
this.el = this.$el[0];
this.undelegateEvents();
this._createContext(element);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if createContext set $el and returned el to be set here:

this.el = this._createContext(this.el);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah. There should be one and only one place that assigns to this.el and this.$el directly. Anywhere else is just creating extra coupling.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.el and this.$el get assigned in the constructor too, in the viewOptions pick. I think it makes _createContext's contract a little cleaner to do it here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's only el in viewOptions.

I don't know about making _createContext all functional and stuff. It's even weirder if we tell people the contract is side-effect free while putting a side-effect in there ourselves.

if (delegate !== false) this.delegateEvents();
return this;
},
Expand All @@ -1067,49 +1076,57 @@
delegateEvents: function(events) {
if (!(events || (events = _.result(this, 'events')))) return this;
this.undelegateEvents();
var _delegate = this._delegate;
for (var key in events) {
var method = events[key];
if (!_.isFunction(method)) method = this[events[key]];
if (!method) continue;

var match = key.match(delegateEventSplitter);
var eventName = match[1], selector = match[2];
this.delegate(eventName, selector, method);
method = method.bind && method.bind(this) || _.bind(method, this);
_delegate.call(this, eventName, selector, method);
}
return this;
},

// Add a single event listener to the element responding only to the
// optional `selector` or catches all `eventName` events. Subclasses can
// override this to utilize an alternative DOM event management API.
_delegate: function(eventName, selector, method) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer unprefixed delegate so that it can be called directly.

eventName += '.delegateEvents' + this.cid;
if (!selector) {
this.$el.on(eventName, method);
} else {
this.$el.on(eventName, selector, method);
}
return this;
},

// Clears all callbacks previously bound to the view with `delegateEvents`.
// Clears all callbacks previously bound to the view by `delegateEvents`.
// You usually don't need to use this, but may wish to if you have multiple
// Backbone views attached to the same DOM element.
undelegateEvents: function() {
this.$el.off('.delegateEvents' + this.cid);
if (this.$el) this.$el.off('.delegateEvents' + this.cid);
return this;
},

// Add a single event listener to the element.
delegate: function(eventName, selector, method) {
if (_.isFunction(selector)) {
method = selector;
selector = undefined;
}
eventName += '.delegateEvents' + this.cid;
if (!selector) {
this.$el.on(eventName, _.bind(method, this));
// Creates the actual context for this view using the given `root` and a
// hash of `attributes` and returns the created element. `root` can be a CSS
// selector or an HTML string, a jQuery context or and Element. Subclasses
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or and Element > or an element

// can override this to utilize and alternative DOM manipulation API.
_createContext: function(root, attributes) {
var $el;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the one-liner instead?

_createContext: function(el, attrs) {
  this.$el = el instanceof Backbone.$ ? el : Backbone.$(el);
  this.$el.attr(attrs || {});
  return this.$el[0];
}

if (typeof root == 'string') {
$el = Backbone.$(root);
} else if (root instanceof Backbone.$) {
$el = root;
} else {
this.$el.on(eventName, selector, _.bind(method, this));
$el = Backbone.$(root);
}
return this;
},

// For hooking into small amounts of DOM Elements, where a full-blown template isn't
// needed, use **make** to manufacture elements, one at a time.
//
// var el = this.make('li', {'class': 'row'}, this.model.escape('title'));
//
make: function(tagName, attributes, content) {
var $el = Backbone.$('<' + tagName + '>', attributes).html(content);
return $el[0];
if (!_.isEmpty(attributes)) $el.attr(attributes);
this.$el = $el;
return (this.el = $el[0]);
},

// Ensure that the View has a DOM element to render into.
Expand All @@ -1121,7 +1138,8 @@
var attrs = _.extend({}, _.result(this, 'attributes'));
if (this.id) attrs.id = _.result(this, 'id');
if (this.className) attrs['class'] = _.result(this, 'className');
this.setElement(this.make(_.result(this, 'tagName'), attrs), false);
var el = document.createElement(_.result(this, 'tagName'));
this.setElement(this._createContext(el, attrs), false);
} else {
this.setElement(_.result(this, 'el'), false);
}
Expand Down
22 changes: 8 additions & 14 deletions test/view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,14 @@
strictEqual(view.$('a b').html(), 'test');
});

test("make", 3, function() {
var div = view.make('div', {id: 'test-div'}, "one two three");
test("_createContext", 5, function() {
var div = view._createContext('<div>', {id: 'test-div'});

equal(div.tagName.toLowerCase(), 'div');
equal(div.id, 'test-div');
equal($(div).text(), 'one two three');
});

test("make can take falsy values for content", 2, function() {
var div = view.make('div', {id: 'test-div'}, 0);
equal($(div).text(), '0');

div = view.make('div', {id: 'test-div'}, '');
equal($(div).text(), '');
equal(view.el, div);
ok(view.$el instanceof Backbone.$);
equal(view.$el[0], div);
});

test("initialize", 1, function() {
Expand Down Expand Up @@ -76,15 +70,15 @@
equal(counter2, 3);
});

test("delegate", 2, function() {
test("_delegate", 2, function() {
var counter1 = 0, counter2 = 0;

var view = new Backbone.View({el: '#testElement'});
view.increment = function(){ counter1++; };
view.$el.on('click', function(){ counter2++; });

view.delegate('click', 'h1', view.increment);
view.delegate('click', view.increment);
view._delegate('click', 'h1', view.increment);
view._delegate('click', view.increment);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope test passed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


view.$('h1').trigger('click');
equal(counter1, 2);
Expand Down