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

View hooks for native and non-jQuery libraries #3003

Merged
merged 92 commits into from Mar 16, 2014

Conversation

Projects
None yet
@akre54
Collaborator

akre54 commented Feb 18, 2014

This is a continuation of the discussion in #2959, and focuses on making View easier to extend by users wishing to use non-jQuery libraries. It's similar in plugability to Backbone.ajax or other overridable parts of Backbone.

Native Views and non-jQuery DOM libraries would be able to create a native View implementation by overriding $, make, remove, setElement, delegate, and undelegateEvents (see the NativeView reference implementation). This preserves the majority of the public contract of View (with the notable exception of View#$ and view.$el), and the public interface of the constructor (including passing options), delegateEvents/undelegateEvents, render, remove, and setElement would be virtually unchanged. The PR also re-adds View#make.

The next step would be to remove Backbone.$() calls from History and perhaps extend View#setElement and View#make to use a common exposed createElement utility.


Update for folks tuning in late: The final list of overridable methods is available on the wiki with instructions for how to use it in your own project.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Feb 18, 2014

Owner

Do the other interested parties agree that these hooks are the minimal surface area required to implement a NativeView in a fashion that's optimally performant?

Owner

jashkenas commented Feb 18, 2014

Do the other interested parties agree that these hooks are the minimal surface area required to implement a NativeView in a fashion that's optimally performant?

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Feb 18, 2014

Contributor

👍
You could cleanup the addEventListener by removing useCapture and providing false for its arg.
Also you might be able to cleanup the iframe creation with make.

Contributor

jdalton commented Feb 18, 2014

👍
You could cleanup the addEventListener by removing useCapture and providing false for its arg.
Also you might be able to cleanup the iframe creation with make.

@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Feb 19, 2014

Contributor

@akre54 I like how you just copied 99% of my PR and stamped your own name on the copyright there :)

The event listener shims are slower because of the extra function calls and branching. useCapture also useless. Best keep the highly optimized event listener shims implemented in #2959. Not sure about using make for iframe creation because that brings back the whole Backbone.utils thing.

Contributor

wyuenho commented Feb 19, 2014

@akre54 I like how you just copied 99% of my PR and stamped your own name on the copyright there :)

The event listener shims are slower because of the extra function calls and branching. useCapture also useless. Best keep the highly optimized event listener shims implemented in #2959. Not sure about using make for iframe creation because that brings back the whole Backbone.utils thing.

@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Feb 19, 2014

Contributor

Also, there needs to be a View#_removeElement or something, or you'll forget to call undelegateEvents or stopListening like @akre54 reference implementation.

Contributor

wyuenho commented Feb 19, 2014

Also, there needs to be a View#_removeElement or something, or you'll forget to call undelegateEvents or stopListening like @akre54 reference implementation.

@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Feb 19, 2014

Contributor

And if you make setElement and _ensureElement make use of make, you only have to worry about make.

Contributor

wyuenho commented Feb 19, 2014

And if you make setElement and _ensureElement make use of make, you only have to worry about make.

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

Yeah its just boilerplate from one of my other plugins. I credited you in
the readme and in the changeset since I couldn't chery-pick your commit.

The more important thing is if this PR (and the native view plugin) works
for you. It seems it should. Whose name appears on it I could really care
less.

The shims are only used in History, which shouldnt be a hot code path. In
my eyes the delegate method is the right solution here.
On Feb 19, 2014 12:15 AM, "Jimmy Yuen Ho Wong" notifications@github.com
wrote:

@akre54 https://github.com/akre54 I like how you just copied 99% my PR
and stamped your own name on the copyright there :)

The event listener shims are slow because of the extra function calls and
branching. useCapture also useless. Best keep the highly optimized event
listener shims implemented in #2959#2959.
Not sure about using make for iframe creation because that brings back
the whole Backbone.utils thing.

Reply to this email directly or view it on GitHubhttps://github.com//pull/3003#issuecomment-35466977
.

Collaborator

akre54 commented Feb 19, 2014

Yeah its just boilerplate from one of my other plugins. I credited you in
the readme and in the changeset since I couldn't chery-pick your commit.

The more important thing is if this PR (and the native view plugin) works
for you. It seems it should. Whose name appears on it I could really care
less.

The shims are only used in History, which shouldnt be a hot code path. In
my eyes the delegate method is the right solution here.
On Feb 19, 2014 12:15 AM, "Jimmy Yuen Ho Wong" notifications@github.com
wrote:

@akre54 https://github.com/akre54 I like how you just copied 99% my PR
and stamped your own name on the copyright there :)

The event listener shims are slow because of the extra function calls and
branching. useCapture also useless. Best keep the highly optimized event
listener shims implemented in #2959#2959.
Not sure about using make for iframe creation because that brings back
the whole Backbone.utils thing.

Reply to this email directly or view it on GitHubhttps://github.com//pull/3003#issuecomment-35466977
.

@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Feb 19, 2014

Contributor

That's true if you've decided to not bank on the shims to pull out a native BaseView.

Contributor

wyuenho commented Feb 19, 2014

That's true if you've decided to not bank on the shims to pull out a native BaseView.

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

you might be able to cleanup the iframe creation with make.

I thought about this, but being able to set html in a clean, cross-browser way in make is important enough to use jQuery there, and for History we're trying to avoid jQuery altogether. I think it is even a little cleaner than adding another helper method, too.

Collaborator

akre54 commented Feb 19, 2014

you might be able to cleanup the iframe creation with make.

I thought about this, but being able to set html in a clean, cross-browser way in make is important enough to use jQuery there, and for History we're trying to avoid jQuery altogether. I think it is even a little cleaner than adding another helper method, too.

Show outdated Hide outdated backbone.js Outdated
Show outdated Hide outdated backbone.js Outdated
Show outdated Hide outdated backbone.js Outdated
Show outdated Hide outdated backbone.js Outdated
@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Feb 19, 2014

Contributor

Mmmm, $el strikes again. Doesn't look like there's a way to use make inside setElement and not break things huh? If not then make is kinda pointless.

Contributor

wyuenho commented Feb 19, 2014

Mmmm, $el strikes again. Doesn't look like there's a way to use make inside setElement and not break things huh? If not then make is kinda pointless.

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

Eh? setElement is one of the methods that inheriting views should override. make's contract is to return an Element, not a jquery-wrapped element.

As I said in #2959, plugins should rely on el unless they absolutely must look to $el. This would be no different with a BaseView approach.

Collaborator

akre54 commented Feb 19, 2014

Eh? setElement is one of the methods that inheriting views should override. make's contract is to return an Element, not a jquery-wrapped element.

As I said in #2959, plugins should rely on el unless they absolutely must look to $el. This would be no different with a BaseView approach.

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

@jdalton whoah, didn't know you could pass html directly into createElement. Awesome.

Collaborator

akre54 commented Feb 19, 2014

@jdalton whoah, didn't know you could pass html directly into createElement. Awesome.

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

We'd still need the IE sniff for the off hashChange, right?

Collaborator

akre54 commented Feb 19, 2014

We'd still need the IE sniff for the off hashChange, right?

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

In chrome at least I'm getting an error: invalid characters

Collaborator

akre54 commented Feb 19, 2014

In chrome at least I'm getting an error: invalid characters

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Feb 19, 2014

Contributor

@akre54

We'd still need the IE sniff for the off hashChange, right?

Ya, I removed the UA sniff not the this._oldIE prop.

In chrome at least I'm getting an error: invalid characters

That's why it's wrapped in a try-catch. That syntax is only supported in IE < 9.

Contributor

jdalton commented Feb 19, 2014

@akre54

We'd still need the IE sniff for the off hashChange, right?

Ya, I removed the UA sniff not the this._oldIE prop.

In chrome at least I'm getting an error: invalid characters

That's why it's wrapped in a try-catch. That syntax is only supported in IE < 9.

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

Yup, got it. Neat little old feature.

Collaborator

akre54 commented Feb 19, 2014

Yup, got it. Neat little old feature.

Show outdated Hide outdated backbone.js Outdated
Show outdated Hide outdated backbone.js Outdated
@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Feb 19, 2014

Contributor

@akre54 @jdalton Can you guys just clean up History and leave View alone for now? think we can all agree on removing jQuery from History.

I'm gonna remove all the shims and changes in History in #2959 and experiment a little with BaseView to iron out a minimal set of hooks to expose that doesn't break things.

Contributor

wyuenho commented Feb 19, 2014

@akre54 @jdalton Can you guys just clean up History and leave View alone for now? think we can all agree on removing jQuery from History.

I'm gonna remove all the shims and changes in History in #2959 and experiment a little with BaseView to iron out a minimal set of hooks to expose that doesn't break things.

@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Feb 19, 2014

Contributor

Also, @akre54 If you don't use make internally? Why introduce/advertise it as a hook?

Contributor

wyuenho commented Feb 19, 2014

Also, @akre54 If you don't use make internally? Why introduce/advertise it as a hook?

Show outdated Hide outdated backbone.js Outdated
@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

@akre54 @jdalton Can you guys just clean up History and leave View alone for now? I can think we can all agree on removing jQuery from History.

I can open another pull, but I think these are part and parcel to the same concept. Might as well merge 'em together or leave them both out.

Also, @akre54 If you don't use make internally? Why introduce/advertise it as a hook?

We do, in _ensureElement. The reason for breaking out make is to allow _ensureElement to remain untouched in a Native View, and only have to override make. I was thinking of doing something similar with setElement, but I think that's integrated enough that just overriding that line makes more sense here.

#2959 takes the opposite tack with View as this one. Are there any speed problems with this strategy in your eyes?

Collaborator

akre54 commented Feb 19, 2014

@akre54 @jdalton Can you guys just clean up History and leave View alone for now? I can think we can all agree on removing jQuery from History.

I can open another pull, but I think these are part and parcel to the same concept. Might as well merge 'em together or leave them both out.

Also, @akre54 If you don't use make internally? Why introduce/advertise it as a hook?

We do, in _ensureElement. The reason for breaking out make is to allow _ensureElement to remain untouched in a Native View, and only have to override make. I was thinking of doing something similar with setElement, but I think that's integrated enough that just overriding that line makes more sense here.

#2959 takes the opposite tack with View as this one. Are there any speed problems with this strategy in your eyes?

@akre54 akre54 referenced this pull request Feb 19, 2014

Merged

History native calls #3006

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Feb 19, 2014

Collaborator

@wyuenho moving History hooks to a new PR, and turning this one into just View.

Collaborator

akre54 commented Feb 19, 2014

@wyuenho moving History hooks to a new PR, and turning this one into just View.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Mar 14, 2014

Collaborator

Some minor nits addressed in akre54#19.

Collaborator

braddunbar commented Mar 14, 2014

Some minor nits addressed in akre54#19.

@braddunbar braddunbar referenced this pull request Mar 14, 2014

Merged

A little clarification. #19

braddunbar and others added some commits Mar 14, 2014

A little clarification.
* Rename #_remove as #_removeElement.
* Add #_setAttributes instead of using #setElement.
* Slim down tests a bit.
* #delegate returns this.
Merge pull request #20 from braddunbar/view-hooks
Add #_setAttributes and clean up #setElement.
@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Mar 16, 2014

Collaborator

Alright, this is looking good. The final consensus is view subclasses that do not use an API conforming to Backbone.$ will have to override six methods which cover all DOM operations in Backbone.View.

  • _removeElement
  • delegate
  • undelegate
  • undelegateEvents
  • _setElement
  • _setAttributes

@jashkenas Note the addition of _setAttributes. I think a specific hook for setting attributes is much safer (for future API additions/changes) than passing the argument through two unrelated methods with a mix of public and internal arguments.

undelegate is also new. It mirrors the delegate API nicely and maps directly to jQuery#off.

Many thanks to @akre54, @wyuenho, and others for their patience with me, I know we covered a lot of ground here. 😃 👍

Collaborator

braddunbar commented Mar 16, 2014

Alright, this is looking good. The final consensus is view subclasses that do not use an API conforming to Backbone.$ will have to override six methods which cover all DOM operations in Backbone.View.

  • _removeElement
  • delegate
  • undelegate
  • undelegateEvents
  • _setElement
  • _setAttributes

@jashkenas Note the addition of _setAttributes. I think a specific hook for setting attributes is much safer (for future API additions/changes) than passing the argument through two unrelated methods with a mix of public and internal arguments.

undelegate is also new. It mirrors the delegate API nicely and maps directly to jQuery#off.

Many thanks to @akre54, @wyuenho, and others for their patience with me, I know we covered a lot of ground here. 😃 👍

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 16, 2014

Contributor

Huge 👍 for this work. It will be really exciting to see how this influences Backbone frameworks.

Contributor

nhunzaker commented Mar 16, 2014

Huge 👍 for this work. It will be really exciting to see how this influences Backbone frameworks.

@eastridge

This comment has been minimized.

Show comment
Hide comment
@eastridge

eastridge Mar 16, 2014

Contributor

@braddunbar @akre54 @wyuenho

Thanks for all of your work on this. Already have immediate plans for it as soon as it is ready:

https://github.com/FormidableLabs/handlebones

Contributor

eastridge commented Mar 16, 2014

@braddunbar @akre54 @wyuenho

Thanks for all of your work on this. Already have immediate plans for it as soon as it is ready:

https://github.com/FormidableLabs/handlebones

jashkenas added a commit that referenced this pull request Mar 16, 2014

Merge pull request #3003 from akre54/view-native-hooks
View hooks for native and non-jQuery libraries

@jashkenas jashkenas merged commit 3994c1c into jashkenas:master Mar 16, 2014

1 check passed

default The Travis CI build passed
Details
@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Mar 16, 2014

Owner

Anyone feel like adding a page to the wiki for library developers explaining how to take advantage of the full implications of this patch?

Owner

jashkenas commented Mar 16, 2014

Anyone feel like adding a page to the wiki for library developers explaining how to take advantage of the full implications of this patch?

@wyuenho

This comment has been minimized.

Show comment
Hide comment
@wyuenho

wyuenho Mar 17, 2014

Contributor

Heads off to @paulmillr for the initial PR too. Thanks everyone for helping out with this!

@jashkenas Since we'll have to document the new APIs anyway, shouldn't we add a section to the FAQ instead?

Contributor

wyuenho commented Mar 17, 2014

Heads off to @paulmillr for the initial PR too. Thanks everyone for helping out with this!

@jashkenas Since we'll have to document the new APIs anyway, shouldn't we add a section to the FAQ instead?

@stavarotti

This comment has been minimized.

Show comment
Hide comment
@stavarotti

stavarotti Mar 17, 2014

This has been a fantastic pr to follow. Thanks to all participants striving to make this project better.

stavarotti commented Mar 17, 2014

This has been a fantastic pr to follow. Thanks to all participants striving to make this project better.

@akre54

This comment has been minimized.

Show comment
Hide comment
@akre54

akre54 Mar 17, 2014

Collaborator

Yeah I can write something up for the wiki or docs. Nice job everybody. This was one crazy long PR!

Collaborator

akre54 commented Mar 17, 2014

Yeah I can write something up for the wiki or docs. Nice job everybody. This was one crazy long PR!

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