Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
feat($compile): bind isolate scope properties to controller
Browse files Browse the repository at this point in the history 
It is now possible to ask the $compiler's isolate scope property machinery to bind isolate
scope properties to a controller rather than scope itself. This feature requires the use of
controllerAs, so that the controller-bound properties may still be referenced from binding
expressions in views.

The current syntax is to prefix the scope name with a '@', like so:

    scope: {
        "myData": "=someData",
        "myString": "@someInterpolation",
        "myExpr": "&someExpr"
    },
    controllerAs: "someCtrl",
    bindtoController: true

The putting of properties within the context of the controller will only occur if
controllerAs is used for an isolate scope with the `bindToController` property of the
directive definition object set to `true`.

Closes #7635
Closes #7645
  • Loading branch information
@caitp @IgorMinar
caitp authored and IgorMinar committed Aug 29, 2014
1 parent cb73a37 commit 5f3f25a
Show file tree
Hide file tree
Showing 3 changed files with 194 additions and 50 deletions.
105 changes: 64 additions & 41 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@
* by calling the `localFn` as `localFn({amount: 22})`.
*
*
* #### `bindToController`
* When an isolate scope is used for a component (see above), and `controllerAs` is used, `bindToController` will
* allow a component to have its properties bound to the controller, rather than to scope. When the controller
* is instantiated, the initial values of the isolate scope bindings are already available.
*
* #### `controller`
* Controller constructor function. The controller is instantiated before the
Expand Down Expand Up @@ -981,7 +985,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

if (transcludeControllers) {
for (var controllerName in transcludeControllers) {
$linkNode.data('$' + controllerName + 'Controller', transcludeControllers[controllerName]);
$linkNode.data('$' + controllerName + 'Controller', transcludeControllers[controllerName].instance);
}
}

Expand Down Expand Up @@ -1316,6 +1320,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
var terminalPriority = -Number.MAX_VALUE,
newScopeDirective,
controllerDirectives = previousCompileContext.controllerDirectives,
controllers,
newIsolateScopeDirective = previousCompileContext.newIsolateScopeDirective,
templateDirective = previousCompileContext.templateDirective,
nonTlbTranscludeDirective = previousCompileContext.nonTlbTranscludeDirective,
Expand Down Expand Up @@ -1553,7 +1558,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
value = null;

if (elementControllers && retrievalMethod === 'data') {
value = elementControllers[require];
if (value = elementControllers[require]) {
value = value.instance;
}
}
value = value || $element[retrievalMethod]('$' + require + 'Controller');

Expand Down Expand Up @@ -1586,14 +1593,56 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

if (newIsolateScopeDirective) {
var LOCAL_REGEXP = /^\s*([@=&])(\??)\s*(\w*)\s*$/;

isolateScope = scope.$new(true);
}

transcludeFn = boundTranscludeFn && controllersBoundTransclude;
if (controllerDirectives) {
// TODO: merge `controllers` and `elementControllers` into single object.
controllers = {};
elementControllers = {};
forEach(controllerDirectives, function(directive) {
var locals = {
$scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope,
$element: $element,
$attrs: attrs,
$transclude: transcludeFn
}, controllerInstance;

controller = directive.controller;
if (controller == '@') {
controller = attrs[directive.name];
}

controllerInstance = $controller(controller, locals, true, directive.controllerAs);

// For directives with element transclusion the element is a comment,
// but jQuery .data doesn't support attaching data to comment nodes as it's hard to
// clean up (http://bugs.jquery.com/ticket/8335).
// Instead, we save the controllers for the element in a local hash and attach to .data
// later, once we have the actual element.
elementControllers[directive.name] = controllerInstance;
if (!hasElementTranscludeDirective) {
$element.data('$' + directive.name + 'Controller', controllerInstance.instance);
}

controllers[directive.name] = controllerInstance;
});
}

if (newIsolateScopeDirective) {
var LOCAL_REGEXP = /^\s*([@=&])(\??)\s*(\w*)\s*$/;

compile.$$addScopeInfo($element, isolateScope, true, !(templateDirective && (templateDirective === newIsolateScopeDirective ||
templateDirective === newIsolateScopeDirective.$$originalDirective)));
compile.$$addScopeClass($element, true);

var isolateScopeController = controllers && controllers[newIsolateScopeDirective.name];
var isolateBindingContext = isolateScope;
if (isolateScopeController && isolateScopeController.identifier &&
newIsolateScopeDirective.bindToController === true) {
isolateBindingContext = isolateScopeController.instance;
}
forEach(newIsolateScopeDirective.scope, function(definition, scopeName) {
var match = definition.match(LOCAL_REGEXP) || [],
attrName = match[3] || scopeName,
Expand All @@ -1614,7 +1663,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if( attrs[attrName] ) {
// If the attribute has been provided then we trigger an interpolation to ensure
// the value is there for use in the link fn
isolateScope[scopeName] = $interpolate(attrs[attrName])(scope);
isolateBindingContext[scopeName] = $interpolate(attrs[attrName])(scope);
}
break;

Expand All @@ -1630,21 +1679,21 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
parentSet = parentGet.assign || function() {
// reset the change, or we will throw this exception on every $digest
lastValue = isolateScope[scopeName] = parentGet(scope);
lastValue = isolateBindingContext[scopeName] = parentGet(scope);
throw $compileMinErr('nonassign',
"Expression '{0}' used with directive '{1}' is non-assignable!",
attrs[attrName], newIsolateScopeDirective.name);
};
lastValue = isolateScope[scopeName] = parentGet(scope);
lastValue = isolateBindingContext[scopeName] = parentGet(scope);
var unwatch = scope.$watch($parse(attrs[attrName], function parentValueWatch(parentValue) {
if (!compare(parentValue, isolateScope[scopeName])) {
if (!compare(parentValue, isolateBindingContext[scopeName])) {
// we are out of sync and need to copy
if (!compare(parentValue, lastValue)) {
// parent changed and it has precedence
isolateScope[scopeName] = parentValue;
isolateBindingContext[scopeName] = parentValue;
} else {
// if the parent can be assigned then do so
parentSet(scope, parentValue = isolateScope[scopeName]);
parentSet(scope, parentValue = isolateBindingContext[scopeName]);
}
}
return lastValue = parentValue;
Expand All @@ -1654,7 +1703,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

case '&':
parentGet = $parse(attrs[attrName]);
isolateScope[scopeName] = function(locals) {
isolateBindingContext[scopeName] = function(locals) {
return parentGet(scope, locals);
};
break;
Expand All @@ -1667,37 +1716,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
});
}
transcludeFn = boundTranscludeFn && controllersBoundTransclude;
if (controllerDirectives) {
elementControllers = {};
forEach(controllerDirectives, function(directive) {
var locals = {
$scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope,
$element: $element,
$attrs: attrs,
$transclude: transcludeFn
}, controllerInstance;

controller = directive.controller;
if (controller == '@') {
controller = attrs[directive.name];
}

controllerInstance = $controller(controller, locals);
// For directives with element transclusion the element is a comment,
// but jQuery .data doesn't support attaching data to comment nodes as it's hard to
// clean up (http://bugs.jquery.com/ticket/8335).
// Instead, we save the controllers for the element in a local hash and attach to .data
// later, once we have the actual element.
elementControllers[directive.name] = controllerInstance;
if (!hasElementTranscludeDirective) {
$element.data('$' + directive.name + 'Controller', controllerInstance);
}

if (directive.controllerAs) {
locals.$scope[directive.controllerAs] = controllerInstance;
}
if (controllers) {
forEach(controllers, function(controller) {
controller();
});
controllers = null;
}

// PRELINKING
Expand Down
61 changes: 52 additions & 9 deletions src/ng/controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,24 @@ function $ControllerProvider() {
* It's just a simple call to {@link auto.$injector $injector}, but extracted into
* a service, so that one can override this service with [BC version](https://gist.github.com/1649788).
*/
return function(expression, locals) {
return function(expression, locals, later, ident) {
// PRIVATE API:
// param `later` --- indicates that the controller's constructor is invoked at a later time.
// If true, $controller will allocate the object with the correct
// prototype chain, but will not invoke the controller until a returned
// callback is invoked.
// param `ident` --- An optional label which overrides the label parsed from the controller
// expression, if any.
var instance, match, constructor, identifier;
later = later === true;
if (ident && isString(ident)) {
identifier = ident;
}

if(isString(expression)) {
match = expression.match(CNTRL_REG),
constructor = match[1],
identifier = match[3];
identifier = identifier || match[3];
expression = controllers.hasOwnProperty(constructor)
? controllers[constructor]
: getter(locals.$scope, constructor, true) ||
Expand All @@ -83,19 +94,51 @@ function $ControllerProvider() {
assertArgFn(expression, constructor, true);
}

instance = $injector.instantiate(expression, locals, constructor);
if (later) {
// Instantiate controller later:
// This machinery is used to create an instance of the object before calling the
// controller's constructor itself.
//
// This allows properties to be added to the controller before the constructor is
// invoked. Primarily, this is used for isolate scope bindings in $compile.
//
// This feature is not intended for use by applications, and is thus not documented
// publicly.
var Constructor = function() {};
Constructor.prototype = (isArray(expression) ?
expression[expression.length - 1] : expression).prototype;
instance = new Constructor();

if (identifier) {
if (!(locals && typeof locals.$scope === 'object')) {
throw minErr('$controller')('noscp',
"Cannot export controller '{0}' as '{1}'! No $scope object provided via `locals`.",
constructor || expression.name, identifier);
if (identifier) {
addIdentifier(locals, identifier, instance, constructor || expression.name);
}

locals.$scope[identifier] = instance;
return extend(function() {
$injector.invoke(expression, instance, locals, constructor);
return instance;
}, {
instance: instance,
identifier: identifier
});
}

instance = $injector.instantiate(expression, locals, constructor);

if (identifier) {
addIdentifier(locals, identifier, instance, constructor || expression.name);
}

return instance;
};

function addIdentifier(locals, identifier, instance, name) {
if (!(locals && isObject(locals.$scope))) {
throw minErr('$controller')('noscp',
"Cannot export controller '{0}' as '{1}'! No $scope object provided via `locals`.",
name, identifier);
}

locals.$scope[identifier] = instance;
}
}];
}
78 changes: 78 additions & 0 deletions test/ng/compileSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3516,6 +3516,84 @@ describe('$compile', function() {
expect(componentScope.$$isolateBindings.exprAlias).toBe('&expr');

}));


it('should expose isolate scope variables on controller with controllerAs when bindToController is true', function() {
var controllerCalled = false;
module(function($compileProvider) {
$compileProvider.directive('fooDir', valueFn({
template: '<p>isolate</p>',
scope: {
'data': '=dirData',
'str': '@dirStr',
'fn': '&dirFn'
},
controller: function($scope) {
expect(this.data).toEqualData({
'foo': 'bar',
'baz': 'biz'
});
expect(this.str).toBe('Hello, world!');
expect(this.fn()).toBe('called!');
controllerCalled = true;
},
controllerAs: 'test',
bindToController: true
}));
});
inject(function($compile, $rootScope) {
$rootScope.fn = valueFn('called!');
$rootScope.whom = 'world';
$rootScope.remoteData = {
'foo': 'bar',
'baz': 'biz'
};
element = $compile('<div foo-dir dir-data="remoteData" ' +
'dir-str="Hello, {{whom}}!" ' +
'dir-fn="fn()"></div>')($rootScope);
expect(controllerCalled).toBe(true);
});
});


it('should expose isolate scope variables on controller with controllerAs when bindToController is true', function() {
var controllerCalled = false;
module(function($compileProvider) {
$compileProvider.directive('fooDir', valueFn({
templateUrl: 'test.html',
scope: {
'data': '=dirData',
'str': '@dirStr',
'fn': '&dirFn'
},
controller: function($scope) {
expect(this.data).toEqualData({
'foo': 'bar',
'baz': 'biz'
});
expect(this.str).toBe('Hello, world!');
expect(this.fn()).toBe('called!');
controllerCalled = true;
},
controllerAs: 'test',
bindToController: true
}));
});
inject(function($compile, $rootScope, $templateCache) {
$templateCache.put('test.html', '<p>isolate</p>');
$rootScope.fn = valueFn('called!');
$rootScope.whom = 'world';
$rootScope.remoteData = {
'foo': 'bar',
'baz': 'biz'
};
element = $compile('<div foo-dir dir-data="remoteData" ' +
'dir-str="Hello, {{whom}}!" ' +
'dir-fn="fn()"></div>')($rootScope);
$rootScope.$digest();
expect(controllerCalled).toBe(true);
});
});
});


Expand Down

16 comments on commit 5f3f25a

@rodyhaddad
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just me, or is the commit message not quite right when it says:

The current syntax is to prefix the scope name with a '@', like so:
scope: {
"myData": "=someData",
"myString": "@someInterpolation",
"myExpr": "&someExpr"
},
controllerAs: "someCtrl",
bindtoController: true

We'll have to remember that for the changelog

@caitp
Copy link
Contributor Author

@caitp caitp commented on 5f3f25a Aug 29, 2014

Choose a reason for hiding this comment

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

oh, derp. yes, forgot to change that part of the message when the api changed

@IgorMinar
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. there are two issues:

  • mention of @
  • and bindtoController vs bindToController

@twhitbeck
Copy link
Contributor

Choose a reason for hiding this comment

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

am I correct in thinking that the way to make use of this is as follows

  • Define isolate scope as usual
  • add bindToController: true to directive definition object
  • provide a controllerAs property in directive definition object

Something like:

return {
  scope: {
    data: '='
  },
  bindToController: true,
  controller: function() {
    // This will be two-way bound to containing scope
    data = {
      count: 0
    };
  },
  controllerAs: 'ctrl',
  link: function(scope, el, attrs) {
  },
  template: '<button ng-click="ctrl.data.count = ctrl.data.count + 1">Increment</button>'
};

In this case data is not directly accessible on the scope via scope.data but indirectly through the controller which is on the scope like scope.ctrl.data, correct?

That's what I gathered from reading various issues and such, but the changelog merely points to this commit which doesn't summarize the correct/most recent syntax. Seems like a useful feature, thanks for any clarification.

@caitp
Copy link
Contributor Author

@caitp caitp commented on 5f3f25a Sep 3, 2014

Choose a reason for hiding this comment

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

@twhitbeck that is correct.

Yes, the commit message was not correctly updated, I think the documentation site should be okay though. If it's not, please file a bug

@srigi
Copy link

@srigi srigi commented on 5f3f25a Sep 18, 2014

Choose a reason for hiding this comment

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

@twhitbeck there is one small errror in your example:

controller: function() {
  data = {
    count: 0
  };
},

should be

controller: function() {
  this.data = {
    count: 0
  };
},

@caitp
Copy link
Contributor Author

@caitp caitp commented on 5f3f25a Sep 18, 2014

Choose a reason for hiding this comment

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

oh good eye, I didn't notice that @srigi :) yes, you won't be able to bind to properties in the controller if they're actually in the global object

@kentcdodds
Copy link
Member

Choose a reason for hiding this comment

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

Would there be any way to not require the controller function? If I just want to use the scope properties in my template, but I have no need of a controller function, with the current implementation, I have to have a controller property (which I assign to angular.noop). Just an extra line of boilerplate that I don't feel is necessary. If there aren't any technical limitations to making the controller optional, I'd be happy to take a stab at a PR.

@kentcdodds
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it could be done by adding a || angular.noop to this line but I'm unaware of what would break in this case...

@caitp
Copy link
Contributor Author

@caitp caitp commented on 5f3f25a Oct 24, 2014

Choose a reason for hiding this comment

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

if you don't have a controller, why would you want to use bindToController: true in the first place? o_o

@kentcdodds
Copy link
Member

Choose a reason for hiding this comment

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

As I said, I want to be able to be consistent in my templates and use controllerAs: 'vm' for all of my directives and routes. This way, every template is using the same controller name and it's easy to switch between templates. Also, if I ever decide to add a controller in the future, I don't have to worry about updating the template to use controllerAs.

Edit: apologies, I wasn't very clear about my reasons in my first comment.

@caitp
Copy link
Contributor Author

@caitp caitp commented on 5f3f25a Oct 24, 2014

Choose a reason for hiding this comment

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

I don't think that really makes sense :(

@caitp
Copy link
Contributor Author

@caitp caitp commented on 5f3f25a Oct 24, 2014

Choose a reason for hiding this comment

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

bindToController is basically the switch you flip to turn on binding to the controller instead of the scope --- so you still have a switch to flip, but it shouldn't be a terrible amount of work

@kentcdodds
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'll try to be more clear:

angular.module('foobar', []).directive('foo', function() {
  return {
    template: '<div>this is bar: {{vm.bar}}</div>'
    scope: {
      bar: '='
    },
    controllerAs: 'vm',
    bindToController: true
  };
});

with a template-only directive (as opposed to one that adds functionality to the element or model), there is no need for a controller. For the above to work, I have to add controller: angular.noop.

@caitp
Copy link
Contributor Author

@caitp caitp commented on 5f3f25a Oct 24, 2014

Choose a reason for hiding this comment

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

Oops, accidentally deleted a comment --- anyways, you could open a feature request for that since it's tiny, but I dunno how I feel about it

@kentcdodds
Copy link
Member

Choose a reason for hiding this comment

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

Done: #9774

Please sign in to comment.