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.

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

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

Closed
wants to merge 4 commits into from

Conversation

@caitp
Copy link
Contributor

@caitp caitp commented Dec 15, 2014

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
Copy link

@googlebot googlebot commented Dec 15, 2014

CLAs look good, thanks!

@johnlindquist
Copy link
Contributor

@johnlindquist johnlindquist commented Dec 15, 2014

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
Copy link
Contributor Author

@caitp caitp commented Dec 15, 2014

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
Copy link
Contributor Author

@caitp caitp commented Dec 15, 2014

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
Copy link
Contributor

@johnlindquist johnlindquist commented Dec 15, 2014

@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
Copy link
Contributor Author

@caitp caitp commented Dec 15, 2014

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

@caitp
Copy link
Contributor Author

@caitp caitp commented Dec 15, 2014

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
Copy link
Member

@petebacondarwin petebacondarwin commented Dec 15, 2014

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
Copy link
Contributor Author

@caitp caitp commented Dec 15, 2014

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
Copy link
Contributor

@johnlindquist johnlindquist commented Dec 15, 2014

@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

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

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

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

This comment has been minimized.

@gkalpak

gkalpak Jan 19, 2015
Member

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).

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

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?

@description

When using the `bindToController` feature of AngularJS, a directive is required
to have a Controller, in addition to a controller identifier.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

When using the bindToController feature of AngularJS, a directive is required
to provide the controller as identifier, in addition to providing a controller.

This comment has been minimized.

@gkalpak

gkalpak Jan 19, 2015
Member

Same as above.

BTW, I like "controller identifier" (or "controller alias") better than "controller as identifier", as the latter might be misunderstood as implying that a controllerAs property is mandatory (which is not the case).
Not a big issue though, since the examples below make it clear.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

I like controller alias

var controllerAs = directive.controllerAs;
if (!directive.controller || !identifierForController(controller, controllerAs)) {
throw $compileMinErr('noctrl',
"Cannot bind to controller without directive '{0}'s controller.",

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

"Cannot bind to controller without directive '{0}'s controller and controller as identifier."

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

I think we probably need two different errors here. One for a missing controller and one for a missing controller alias.

});


it('should put controller in scope when labelled but not using controllerAs', function() {

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

I don't follow the description of this test. The code below does indeed use controllerAs.

This comment has been minimized.

@caitp

caitp Jan 19, 2015
Author Contributor

It's not using the controllerAs property of the DDO.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

Ah right. I didn't get that "labelled" meant something and that controllerAs was referring to the DDO property rather than implying that the controller had an alias.

I am still not sure what is being tested here. Is it that having a bindToController property should not prevent the controller from being attached to the scope?

This comment has been minimized.

@caitp

caitp Jan 19, 2015
Author Contributor

bindToController is actually irrelevant here, but previously the controller label in the string was ignored. It's just simpler to support the controller-as label too, so it's testing that it works.

This comment has been minimized.

@caitp

caitp Jan 19, 2015
Author Contributor

although I'm not super keen on it because it creates 2 ways to do a thing, but it can be worried about later

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

OK, we need to change labelled to has a controller alias or something like that for consistency. It is only now that I realise that label has been used interchangeably with alias and identity throughout the source.

var match = CNTRL_REG.exec(controller);
if (match) return match[3];
}
}

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 19, 2015
Member

Since this function is only used in compile.js perhaps it should be there and we just allow the CNTRL_REG to be a global instead?

This comment has been minimized.

@caitp

caitp Jan 19, 2015
Author Contributor

I think it's better to keep it here, and it's probably better to use it in controller.js too, just to make sure the same logic is used in both places

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

In that case this function needs refactoring to return the controller name as well, since that is what is needed in $controller.

All the references to label, ident and identifier in this file ought to be changed to alias for consistency. If you don't like alias then you can choose an alternate and change all the references to ident, identifier, alias and label to a single consistent term throughout the codebase.

directiveName, scopeName, definition);
directiveName, scopeName, definition,
(isController ? "controller bindings definition" :
"isolate scope definition"));

This comment has been minimized.

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Jan 19, 2015

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

@gkalpak
Copy link
Member

@gkalpak gkalpak commented Jan 19, 2015

👍 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
Copy link
Contributor Author

@caitp caitp commented Jan 19, 2015

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

@caitp caitp force-pushed the caitp:issue-10420 branch from 7e77e03 to eff13aa Jan 19, 2015
@caitp
Copy link
Contributor Author

@caitp caitp commented Jan 19, 2015

updated to have 2 different errors

@caitp caitp force-pushed the caitp:issue-10420 branch from eff13aa to 6929976 Jan 19, 2015

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

This comment has been minimized.

@gkalpak

gkalpak Jan 20, 2015
Member

directive definition object

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
controller name (in either the "controller" property, or where the controller
was registered).

This comment has been minimized.

@gkalpak

gkalpak Jan 20, 2015
Member

or where the controller was registered

(With the risk of embarrassing myself 😕) what do you mean by that ?

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

I am not sure. I don't think you can specify controllerName as alias when defining a controller. The only other time would be inside the ngController directive, where the controller name is '@' and so we get the controller from the ng-controller attribute value, which obviously can also have the controllerName as alias.

This phrase should either be clarified or removed.

@description

When using the `bindToController` feature of AngularJS, a directive is required
to have a Controller identifier. This can be supplied using the "controllerAs"

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

Let's standardize on always calling this identifier an alias throughout the code and the docs.

@@ -0,0 +1,71 @@
@ngdoc error
@name $compile:noident

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

$compile:noalias

For example, the following directives are valid:

```js
// OKAY, because controller is a string with a label component.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

// OKAY, because controller is a string, with an alias (`$ctrl`).


// OKAY, because the directive uses the controllerAs property to override
// the controller identifier.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

// the controller alias
While the following are invalid:

```js
// BAD, because the controller property is a string with no identifier.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

// BAD, because the controller property is a string with no alias.
directive("bad", function() {
return {
bindToController: true,
controller: "unlabeledCtrl",

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

controller: "unaliasedCtrl",
});


// BAD because the controller is not a string (therefore has no identifier),

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

// BAD because the controller is not a string (therefore has no alias),
@@ -153,6 +153,9 @@
"urlResolve": false,
"urlIsSameOrigin": false,

/* ng/controller.js */
"identifierForController": false,

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

"aliasForController": false,
}
if (isObject(bindings.bindToController)) {
var controller = directive.controller;
var controllerAs = directive.controllerAs;

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

var controllerAlias = directive.controllerAs;
throw $compileMinErr('noctrl',
"Cannot bind to controller without directive '{0}'s controller.",
directiveName);
} else if (!identifierForController(controller, controllerAs)) {

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

 } else if (!identifierForController(controller, controllerAlias)) {

This comment has been minimized.

@gkalpak

gkalpak Jan 20, 2015
Member

Or even go one step further and rename identifierForController to aliasForController :P

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

:-P - quite right! I knew I would miss one...

@@ -1,5 +1,15 @@
'use strict';

var CNTRL_REG = /^(\S+)(\s+as\s+(\w+))?$/;
function identifierForController(controller, ident) {

This comment has been minimized.

@petebacondarwin

petebacondarwin Jan 20, 2015
Member

aliasForController

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Jan 27, 2015

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

@caitp
Copy link
Contributor Author

@caitp caitp commented Jan 27, 2015

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"

@caitp caitp force-pushed the caitp:issue-10420 branch from ff55902 to df6abf5 Jan 27, 2015
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Jan 28, 2015

@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
Copy link

@frfancha frfancha commented Jan 29, 2015

@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

caitp added 4 commits Dec 15, 2014
…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 #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 caitp force-pushed the caitp:issue-10420 branch from df6abf5 to fcaca73 Jan 29, 2015
@caitp
Copy link
Contributor Author

@caitp caitp commented Jan 29, 2015

this is ready to merge

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Jan 29, 2015

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

@caitp
Copy link
Contributor Author

@caitp caitp commented Jan 29, 2015

I've got it, thanks for reviewing

@caitp caitp closed this in 35498d7 Jan 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants