Fixes #1038 - Document changes to `view.$`. #1040

Merged
merged 1 commit into from Feb 25, 2012

Conversation

Projects
None yet
3 participants
@braddunbar
Collaborator

braddunbar commented Feb 22, 2012

No description provided.

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Feb 25, 2012

Owner

I don't know if this is necessary -- there's no conceivable reason to ever do that, or to have ever been doing it in the past. Why do we need to call it out?

Owner

jashkenas commented Feb 25, 2012

I don't know if this is necessary -- there's no conceivable reason to ever do that, or to have ever been doing it in the past. Why do we need to call it out?

@akavlie

This comment has been minimized.

Show comment Hide comment
@akavlie

akavlie Feb 25, 2012

I ran into this; it was the main gotcha with upgrading Backbone 0.9. Our main view was using a template as view.el, and that worked fine in Backbone 0.5.3 (even if not intended). Eventually figured out what was going on by looking at the code, but a mention in the upgrade notes would have helped.

akavlie commented Feb 25, 2012

I ran into this; it was the main gotcha with upgrading Backbone 0.9. Our main view was using a template as view.el, and that worked fine in Backbone 0.5.3 (even if not intended). Eventually figured out what was going on by looking at the code, but a mention in the upgrade notes would have helped.

@braddunbar

This comment has been minimized.

Show comment Hide comment
@braddunbar

braddunbar Feb 25, 2012

Collaborator

It's equivalent to running: $(selector, this.el)

I don't think there is any question that the documentation above should change since it's patently false. As for the upgrade notes, there are certainly legitimate situations in which the previous behavior is very convenient. For instance:

var el = this.$(foo ? selector : htmlstring);

Obviously this could have been written differently but for what reason? The documentation spells out very clearly that it will work just fine.

Collaborator

braddunbar commented Feb 25, 2012

It's equivalent to running: $(selector, this.el)

I don't think there is any question that the documentation above should change since it's patently false. As for the upgrade notes, there are certainly legitimate situations in which the previous behavior is very convenient. For instance:

var el = this.$(foo ? selector : htmlstring);

Obviously this could have been written differently but for what reason? The documentation spells out very clearly that it will work just fine.

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Feb 25, 2012

Owner

If it's convenient in real-world code -- why don't we just go back to the previous behavior?

Owner

jashkenas commented Feb 25, 2012

If it's convenient in real-world code -- why don't we just go back to the previous behavior?

@braddunbar

This comment has been minimized.

Show comment Hide comment
@braddunbar

braddunbar Feb 25, 2012

Collaborator

The reason for the change was performance and a comparison shows that it can be 15-30% faster, and never slower. Is this enough to justify the change in API? I think it's a fairly easy thing to fix for a clear performance win. However, I don't have a strong opinion either way except that the change should be documented if the implementation is left as is.

Collaborator

braddunbar commented Feb 25, 2012

The reason for the change was performance and a comparison shows that it can be 15-30% faster, and never slower. Is this enough to justify the change in API? I think it's a fairly easy thing to fix for a clear performance win. However, I don't have a strong opinion either way except that the change should be documented if the implementation is left as is.

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Feb 25, 2012

Owner

Ok -- I'll relent ;)

Owner

jashkenas commented Feb 25, 2012

Ok -- I'll relent ;)

jashkenas added a commit that referenced this pull request Feb 25, 2012

Merge pull request #1040 from braddunbar/$-docs
Fixes #1038 - Document changes to `view.$`.

@jashkenas jashkenas merged commit 41e9d1a into jashkenas:master Feb 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment