Skip to content
This repository has been archived by the owner before Nov 9, 2022. It is now read-only.
Permalink
Browse files
feat($controller): disable using global controller constructors
With the exception of simple demos, it is not helpful to use globals
for controller constructors. This adds a new method to `$controllerProvider`
to re-enable the old behavior, but disables this feature by default.

BREAKING CHANGE:
`$controller` will no longer look for controllers on `window`.
The old behavior of looking on `window` for controllers was originally intended
for use in examples, demos, and toy apps. We found that allowing global controller
functions encouraged poor practices, so we resolved to disable this behavior by
default.

To migrate, register your controllers with modules rather than exposing them
as globals:

Before:

```javascript
function MyController() {
  // ...
}
```

After:

```javascript
angular.module('myApp', []).controller('MyController', [function() {
  // ...
}]);
```

Although it's not recommended, you can re-enable the old behavior like this:

```javascript
angular.module('myModule').config(['$controllerProvider', function($controllerProvider) {
  // this option might be handy for migrating old apps, but please don't use it
  // in new ones!
  $controllerProvider.allowGlobals();
}]);
```
  • Loading branch information
btford committed Jul 8, 2014
1 parent d5305d5 commit 3f2232b5a181512fac23775b1df4a6ebda67d018
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 16 deletions.
@@ -12,6 +12,7 @@
*/
function $ControllerProvider() {
var controllers = {},
globals = false,
CNTRL_REG = /^(\S+)(\s+as\s+(\w+))?$/;


@@ -32,6 +33,15 @@ function $ControllerProvider() {
}
};

/**
* @ngdoc method
* @name $controllerProvider#allowGlobals
* @description If called, allows `$controller` to find controller constructors on `window`
*/
this.allowGlobals = function() {
globals = true;
};


this.$get = ['$injector', '$window', function($injector, $window) {

@@ -46,7 +56,8 @@ function $ControllerProvider() {
*
* * check if a controller with given name is registered via `$controllerProvider`
* * check if evaluating the string on the current scope returns a constructor
* * check `window[constructor]` on the global `window` object
* * if $controllerProvider#allowGlobals, check `window[constructor]` on the global
* `window` object (not recommended)
*
* @param {Object} locals Injection locals for Controller.
* @return {Object} Instance of given controller.
@@ -66,7 +77,8 @@ function $ControllerProvider() {
identifier = match[3];
expression = controllers.hasOwnProperty(constructor)
? controllers[constructor]
: getter(locals.$scope, constructor, true) || getter($window, constructor, true);
: getter(locals.$scope, constructor, true) ||
(globals ? getter($window, constructor, true) : undefined);

assertArgFn(expression, constructor, true);
}
@@ -64,6 +64,21 @@ describe('$controller', function() {
$controllerProvider.register('hasOwnProperty', function($scope) {});
}).toThrowMinErr('ng', 'badname', "hasOwnProperty is not a valid controller name");
});


it('should instantiate a controller defined on window if allowGlobals is set',
inject(function($window) {
var scope = {};
var Foo = function() {};

$controllerProvider.allowGlobals();

$window.a = {Foo: Foo};

var foo = $controller('a.Foo', {$scope: scope});
expect(foo).toBeDefined();
expect(foo instanceof Foo).toBe(true);
}));
});


@@ -97,15 +112,15 @@ describe('$controller', function() {
});


it('should instantiate controller defined on window', inject(function($window) {
it('should not instantiate a controller defined on window', inject(function($window) {
var scope = {};
var Foo = function() {};

$window.a = {Foo: Foo};

var foo = $controller('a.Foo', {$scope: scope});
expect(foo).toBeDefined();
expect(foo instanceof Foo).toBe(true);
expect(function () {
$controller('a.Foo', {$scope: scope});
}).toThrow();
}));


@@ -7,9 +7,8 @@ describe('ngController', function() {
$controllerProvider.register('PublicModule', function() {
this.mark = 'works';
});
}));
beforeEach(inject(function($window) {
$window.Greeter = function($scope) {

var Greeter = function($scope) {
// private stuff (not exported to scope)
this.prefix = 'Hello ';

@@ -22,20 +21,22 @@ describe('ngController', function() {

$scope.protoGreet = bind(this, this.protoGreet);
};
$window.Greeter.prototype = {
Greeter.prototype = {
suffix: '!',
protoGreet: function(name) {
return this.prefix + name + this.suffix;
}
};
$controllerProvider.register('Greeter', Greeter);

$window.Child = function($scope) {
$controllerProvider.register('Child', function($scope) {
$scope.name = 'Adam';
};
});

$window.Public = function() {
$controllerProvider.register('Public', function($scope) {
this.mark = 'works';
};
});

}));

afterEach(function() {
@@ -77,11 +78,11 @@ describe('ngController', function() {


it('should instantiate controller defined on scope', inject(function($compile, $rootScope) {
$rootScope.Greeter = function($scope) {
$rootScope.VojtaGreeter = function($scope) {
$scope.name = 'Vojta';
};

element = $compile('<div ng-controller="Greeter">{{name}}</div>')($rootScope);
element = $compile('<div ng-controller="VojtaGreeter">{{name}}</div>')($rootScope);
$rootScope.$digest();
expect(element.text()).toBe('Vojta');
}));

6 comments on commit 3f2232b

@gcardoso89
Copy link

@gcardoso89 gcardoso89 commented on 3f2232b Oct 20, 2014

Choose a reason for hiding this comment

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

Hi @btford,

I have a question about this change. I've just started using AngularJS as my main development framework and I use global controllers in every angular app that I develop because I found to be a good practice (maybe I misunderstood your documentation). So as you could imagine this change affects my code a lot.

I know that you offer a solution to re-enable global controllers, and I'm thankful for that, but you refer to this as a "poor practice" and tell us to "please don't use it in new apps". Could you please explain me why? Why that developing using global controllers is a "poor practice"?

I'm just answering you this to improve my angular app's, not to be against your decision. :)

Cheers,
Gonçalo

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented on 3f2232b Oct 20, 2014

Choose a reason for hiding this comment

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

@gcardoso89 - global controllers refer to your controllers being defined as function on the window object. This means that they are openly available to conflict with any other bit of JavaScript that happens to define a function with the same name. Admittedly, if you post-fix your controllers with ...Controller then this could well not happen but there is always the chance, especially if you were to use a number of 3rd party libraries.
It is much safer to put these controller functions inside the safety of a module. You then have more control over when and where this module gets loaded. Unfortunately controller names are global across an individual Angular app and so you still have the potential for conflict but at least you can't clash with completely different code in the JavaScript global namespace.

@caitp
Copy link
Contributor

@caitp caitp commented on 3f2232b Oct 20, 2014

Choose a reason for hiding this comment

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

don't worry, es6 modules will fix this for everyone forever --- waits

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented on 3f2232b Oct 20, 2014

Choose a reason for hiding this comment

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

+1 @caitp

@gcardoso89
Copy link

@gcardoso89 gcardoso89 commented on 3f2232b Oct 21, 2014

Choose a reason for hiding this comment

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

@petebacondarwin thank you for the explanation. I'll consider this in my future apps then!

@liwanchong04719
Copy link

@liwanchong04719 liwanchong04719 commented on 3f2232b Sep 18, 2015

Choose a reason for hiding this comment

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

requirejs aload controller can't work why

Please sign in to comment.