feat($compile): allow using bindToController as object, support both new/isolate scopes #10467

caitp · 2014-12-15T17:35:33Z

bindToController is now able to be specified as a convenient object notation:

bindToController: {
  text: '@text',
  obj: '=obj',
  expr: '&expr'
},
scope: {
  notBoundToCtrl: '@someOtherAttr'
}

It can also be used in conjunction with new scopes, rather than exclusively isolate scopes:

bindToController: {
  text: '@text',
  obj: '=obj',
  expr: '&expr'
},
scope: true

PING @petebacondarwin / @johnlindquist

I am not super keen on this, because it makes things much crazier than they used to be. That said:

Currently, bindToController does not automatically imply an isolate scope --- maybe it should (I hate this though)
It could probably do with more errors being thrown when people use it wrong
The errors it does throw could be better
The errors it throws are potentially breaking changes :(

Closes #10420

googlebot · 2014-12-15T17:35:37Z

CLAs look good, thanks!

johnlindquist · 2014-12-15T17:55:13Z

Why do you hate implying an isolate scope?

Also, what are the goals of "bindToController"?

For example, I'm trying to reconcile bindToController with the tabs example on the http://angularjs.org
http://jsfiddle.net/g6jb1d4b/

and using bindToController in conjunction with "require:'^tabs'" and a linking function bother me a bit. :/

(fyi, I'm working on a pull request to update the angularjs.org homepage examples)

caitp · 2014-12-15T17:59:12Z

The goal of it is really just to make isolate scopes work better with controllerAs --- that's all it was ever intended to do. But, there's no reason it couldn't work with new scopes too, technically.

caitp · 2014-12-15T18:00:21Z

Anyways, it's going to be a while before this (or something like it) lands, so it would be good to get some feedback on it, try it out and see how you like it, find bugs

johnlindquist · 2014-12-15T18:10:11Z

@caitp ok, thanks.

So if the goal is "scopes work better with controllerAs", have you discussed:

If a "controllerAs" is defined, imply that scope:{} bindings belong on that controller? That way you could remove bindToController completely...

caitp · 2014-12-15T18:12:50Z

We can't really do that, it would be a breaking change

caitp · 2014-12-15T18:12:57Z

Even for 1.4, it's too much of a break --- well, "too much of a break" --- it's more that controllerAs does not imply that you want bindings attached to the controller, it doesn't really make sense to do it that way even if it is a bit simpler.

Obviously, the directive api is stupidly complicated, and making it more complicated isn't helping anyone, so finding ways to simplify this would benefit everyone. But it's also a huge inconvenience due to breaking apps

petebacondarwin · 2014-12-15T19:29:12Z

I think the primary goal is to move even more toward the way things will work in Angular 2 where we don't use scope directly for passing data to the template but instead attach stuff to the controller that is exposed on to the template - in 1.x this is via a scope but in 2 the controller instance is the scope

caitp · 2014-12-15T19:32:53Z

neh, the rationale had nothing to do with v2, v2's prototype wasn't even as settled as it is now at that point.

But that's not a very important thing -- the important thing is that people generally like controllerAs and want to use it, and want to be able to put their isolate scope bindings in the controller.

johnlindquist · 2014-12-15T19:43:27Z

@caitp and @petebacondarwin
I would love to see your take on this famous homepage example updated with bindToController and controllerAs. I left some comments where it feels 'weird':

https://jsbin.com/xugomi/3/edit?html,js,output

petebacondarwin · 2015-01-19T12:29:28Z

docs/content/error/$compile/noctrl.ngdoc

@@ -0,0 +1,68 @@
+@ngdoc error
+@name $compile:noctrl
+@fullName Controller is required.


Should this full name be more like: "controller as" identifier required to bind to controller?

If I am not mitaken, this error will also be thrown if there is bindToController: true but no controler:.... So the literals have to account for all cases (i.e. bindToController: true but no controller or no controller identifier).

Yep, I see that now. I was thrown by the main body of the error document, which doesn't really mention that aspect of the error at all.
I wonder if we ought to have two separate errors?

petebacondarwin · 2015-01-19T13:58:33Z

Apart from splitting the error message, this looks good to me. @gkalpak do you have any further comments.

gkalpak · 2015-01-19T15:16:55Z

👍 for separate error messages.
_{In the original PR, @caitp mentioned something about potential leaks and performance regressions.

I don't think it would be much of an issue (but I wouldn't know better); do you think it would make sense to run some benchmarks (if not already done so) ?}
Other than that it LGTM.

caitp · 2015-01-19T15:37:25Z

if you want to release this today, i'll split up the error message and do whatever other cleanup in about 30 minutes.

caitp · 2015-01-19T18:29:36Z

updated to have 2 different errors

gkalpak · 2015-01-20T09:43:24Z

docs/content/error/$compile/noident.ngdoc

+
+When using the `bindToController` feature of AngularJS, a directive is required
+to have a Controller identifier. This can be supplied using the "controllerAs"
+property of the directive object, or alternatively by adding " as ALIAS" to the


directive definition object

petebacondarwin · 2015-01-27T11:36:53Z

@caitp - can you take a look at the comments here and update this PR? Thanks

caitp · 2015-01-27T18:24:20Z

I don't think we should be calling it "alias", because it's misleading --- so I'm just going to ignore all of the comments about renaming "identifier" to "alias"

petebacondarwin · 2015-01-28T08:23:34Z

@caitp - it is fine for you not to think we should not call it alias. As I explained in this comment, what we do need is consistency.

I assume that your preferred name is identifier. In which case please go through and make sure that this is used throughout. In particular remove any use of alias and label.

frfancha · 2015-01-29T15:07:33Z

@caitp
You say "the important thing is that people generally like controllerAs and want to use it".
I wonder why. In our experiences, directives with controller are much slower than directives with just link function and scope access, especially used under ng-repeat

…new/isolate scopes bindToController is now able to be specified as a convenient object notation: ``` bindToController: { text: '@text', obj: '=obj', expr: '&expr' }, scope: {} ``` It can also be used in conjunction with new scopes, rather than exclusively isolate scopes: ``` bindToController: { text: '@text', obj: '=obj', expr: '&expr' }, scope: true ``` Closes angular#10420

The return value of the controller constructor is now respected in all cases. If controllerAs is used, the controller will be re-bound to scope. If bindToController is used, the previous binding $watches (if any) will be unwatched, and bindings re-installed on the new controller.

Clarify that these aliases are identifier names used to reference the controller.

caitp · 2015-01-29T18:24:24Z

this is ready to merge

petebacondarwin · 2015-01-29T21:43:41Z

Great! LGTM! Thanks @caitp. I will merge tomorrow unless you want to do it today.

caitp · 2015-01-29T21:46:56Z

I've got it, thanks for reviewing

googlebot added the cla: yes label Dec 15, 2014

caitp added type: feature component: $compile branch: 1.4.x labels Dec 15, 2014

caitp added this to the 1.4.x milestone Dec 15, 2014

caitp assigned petebacondarwin Dec 15, 2014

caitp force-pushed the issue-10420 branch from 9a97d4b to 7e77e03 Compare December 15, 2014 17:42

caitp mentioned this pull request Dec 17, 2014

fix($compile): respect return value from controller constructor #10502

Closed

petebacondarwin reviewed Jan 19, 2015
View reviewed changes

caitp force-pushed the issue-10420 branch from 7e77e03 to eff13aa Compare January 19, 2015 18:29

caitp force-pushed the issue-10420 branch from eff13aa to 6929976 Compare January 19, 2015 18:34

gkalpak reviewed Jan 20, 2015
View reviewed changes

petebacondarwin assigned caitp and unassigned petebacondarwin Jan 27, 2015

caitp force-pushed the issue-10420 branch from ff55902 to df6abf5 Compare January 27, 2015 18:41

petebacondarwin mentioned this pull request Jan 29, 2015

cannot create two directives on the same element with "scope"attributes and 'bindToController:true' #10888

Closed

caitp added 4 commits January 29, 2015 12:54

docs($compile): create new error for "missing controller identifier"

4ed70af

docs($compile,$route): reword "controller alias" to clarify

fcaca73

Clarify that these aliases are identifier names used to reference the controller.

caitp force-pushed the issue-10420 branch from df6abf5 to fcaca73 Compare January 29, 2015 18:17

caitp closed this in 35498d7 Jan 29, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($compile): allow using bindToController as object, support both new/isolate scopes #10467

feat($compile): allow using bindToController as object, support both new/isolate scopes #10467

caitp commented Dec 15, 2014

googlebot commented Dec 15, 2014

johnlindquist commented Dec 15, 2014

caitp commented Dec 15, 2014

caitp commented Dec 15, 2014

johnlindquist commented Dec 15, 2014

caitp commented Dec 15, 2014

caitp commented Dec 15, 2014

petebacondarwin commented Dec 15, 2014

caitp commented Dec 15, 2014

johnlindquist commented Dec 15, 2014

petebacondarwin Jan 19, 2015

gkalpak Jan 19, 2015 •

edited

Loading

petebacondarwin Jan 19, 2015

petebacondarwin commented Jan 19, 2015

gkalpak commented Jan 19, 2015

caitp commented Jan 19, 2015

caitp commented Jan 19, 2015

gkalpak Jan 20, 2015

petebacondarwin commented Jan 27, 2015

caitp commented Jan 27, 2015

petebacondarwin commented Jan 28, 2015

frfancha commented Jan 29, 2015

caitp commented Jan 29, 2015

petebacondarwin commented Jan 29, 2015

caitp commented Jan 29, 2015

feat($compile): allow using bindToController as object, support both new/isolate scopes #10467

feat($compile): allow using bindToController as object, support both new/isolate scopes #10467

Conversation

caitp commented Dec 15, 2014

googlebot commented Dec 15, 2014

johnlindquist commented Dec 15, 2014

caitp commented Dec 15, 2014

caitp commented Dec 15, 2014

johnlindquist commented Dec 15, 2014

caitp commented Dec 15, 2014

caitp commented Dec 15, 2014

petebacondarwin commented Dec 15, 2014

caitp commented Dec 15, 2014

johnlindquist commented Dec 15, 2014

petebacondarwin Jan 19, 2015

Choose a reason for hiding this comment

gkalpak Jan 19, 2015 • edited Loading

Choose a reason for hiding this comment

petebacondarwin Jan 19, 2015

Choose a reason for hiding this comment

petebacondarwin commented Jan 19, 2015

gkalpak commented Jan 19, 2015

caitp commented Jan 19, 2015

caitp commented Jan 19, 2015

gkalpak Jan 20, 2015

Choose a reason for hiding this comment

petebacondarwin commented Jan 27, 2015

caitp commented Jan 27, 2015

petebacondarwin commented Jan 28, 2015

frfancha commented Jan 29, 2015

caitp commented Jan 29, 2015

petebacondarwin commented Jan 29, 2015

caitp commented Jan 29, 2015

gkalpak Jan 19, 2015 •

edited

Loading