Repository navigation
Angular should throw an error for name collisions #13657
Description
We ran into a nasty bug where a factory we created was replacing an existing angular service in another library.
We named the factory 'dateFilter' (it was a component for filtering items by date). Admittedly, this was a bit of a simplistic and naive name. Got a bunch of TypeErrors in the console and spent hours tracking down the fact that angular UI's bootstrap components rely on a service also named 'dateFilter'.
Seems like it wouldn't be hard for angular to crash with a nice error message if an angular entity is registered with a duplicate name. For example, with the following code:
angular.module('main', ['sub'])
.controller('MyController', someFunc);
// Different file
angular.module('sub', [])
.controller('MyController', differentFunc);
Current behavior is that one controller would replace the other silently, resulting in difficult to find errors. Instead, why not have Angular print an error to the console complaining that 'MyController' was declared twice?
Activity
Narretz commentedon Dec 31, 2015
This is the nature of angular's di system. But throwing is probably something we can talk about. I just can guarantee you that somebody is relying on overwriting a service in an app somewhere.
SonofNun15 commentedon Jan 1, 2016
It actually wouldn't even be necessary to throw. If you are concerned about somebody relying on this behavior somewhere, perhaps just print a warning to the console when a second entity is registered with the same name.
m-amr commentedon Jan 2, 2016
@SonofNun15 @Narretz
I think we should print a warning to notify the Developer that this controller is already defined so he can
change it's name from the beginning, but we should also consider the compatibility the already
written application that rely on that.
i think we can make it optional
so lets make overwriting by default true so it will not break the already written application
but also give the developer the decision to overwrite or throw exception by adding {overwriting: false} as an argument to the controller.
Thanks
SonofNun15 commentedon Jan 4, 2016
This would be an acceptable solution, although it might be preferred to be able to globally set overwriting to false somewhere via angular app configuration, just so you don't have to set overwrite to false everywhere if you are not intending to use this behavior.
Setting overwrite to false on the app could still just change the default so you could override for individual cases if you needed to.
dlinx commentedon Jan 5, 2016
Instead of overwriting, we should add feature to access that controller using its module name. for ex. sub.MyController
For example, To access controller from
And from view it should be accessible like
petebacondarwin commentedon Jan 10, 2016
The new Angular 2 framework has a hierarchical injector and also doesn't rely on global naming so it does not fall foul of these problems. Unfortunatey, Angular 1 expects people to override services to fit their needs and so we should not be throwing or logging. I believe there may have been an
ngHintmodule that could pick up such renames but the simplest approach is to prefix your application services with an application specific namespace.Narretz commentedon Apr 27, 2017
I agree that warning / throwing directly inside AngularJS is not a good idea, as the overridability / duplication (for directives) is central to AngularJS. Angular Hint would be a good fit, but it's not in focus - so PRs for hint would be welcomed.