angular-ui / ui-router

The de-facto solution to flexible routing with nested views in AngularJS
http://ui-router.github.io/
MIT License
13.53k stars 3k forks source link

Revert: Allow element and attr injections #71

Closed jeme closed 11 years ago

jeme commented 11 years ago

https://github.com/angular-ui/ui-router/pull/70

Was merged into the master stream without discussion, I think this is really bad as it goes directly against what is stated on http://docs.angularjs.org/guide/dev_guide.mvc.understanding_controller

Using Controllers Correctly

In general, a controller shouldn't try to do too much. It should contain only the business logic needed for a single view.

The most common way to keep controllers slim is by encapsulating work that doesn't belong to controllers into services and then using these services in controllers via dependency injection. This is discussed in the Dependency Injection Services sections of this guide.

Do not use controllers for:

ksperling commented 11 years ago

I agree with @jeme.

ProLoser commented 11 years ago

Seconded. Reviewing the previous thread (and the thread that stemmed this entire discussion on SO http://stackoverflow.com/questions/15893954/angularjs-view-page-according-to-the-role-of-user-who-is-going-to-login ) the general consensus is that this is improper practice. In addition, with the advent of ng-animate this should no longer be necessary, and instead ui-view should be enhanced to support ng-animate.

Allowing this simply encourages/allows improper design.

Now there's ONE thing I need to point out because I don't have time to dive into code: Is injecting $element or $attributes supported by ng-controller? Are these normal injectables that we are simply blacklisting? Or do we have to create additional code that 'whitelists' (adds support for) these injectables? If it is the former, I do not necessarily a reason to go out of the way to hinder default behavior. However if it is the latter, then the code that explicitly adds these injectables SHOULD in fact be removed.

ProLoser commented 11 years ago

PS:

There is no 'linking' phase of ui-view, and as such modifications made to $element from within a controller will likely immediately collide when the view is rendered. For this very critical reason I believe this should be rolled back. When developing directive controllers, the methods to perform business logic are usually located in the controller, however the DOM manipulations are still required to be performed in a linking function of a directive.

jeme commented 11 years ago

@ProLoser $element or $attrs are only supported by Directive Controllers, ng-controller currently uses the controller as such by setting a controller property to '@'...

ngView on the other hand does not support those 2 parameters on the injector.

I Believe that the later is intentional from the core team, and the first one is really just because it was far easier to tread the controller defined through ng-controller as a directive controller.

I have an issue trying to clarify that: https://github.com/angular/angular.js/issues/2289 which i hope they will respond to at some point.

In any case though, ng-controller is far more tightly bound to the defined controller, meaning that you can't redefine it at "runtime"... Which is exactly what you wan't to do in ng-view/ui-view...