Skip to content

Revert: Allow element and attr injections #71

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

Closed
jeme opened this issue Apr 3, 2013 · 4 comments
Closed

Revert: Allow element and attr injections #71

jeme opened this issue Apr 3, 2013 · 4 comments

Comments

@jeme
Copy link
Contributor

jeme commented Apr 3, 2013

#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:

  • Any kind of DOM manipulation — Controllers should contain only business logic. DOM manipulation—the presentation logic of an application—is well known for being hard to test. Putting any presentation logic into controllers significantly affects testability of the business logic. Angular offers databinding for automatic DOM manipulation. If you have to perform your own manual DOM manipulation, encapsulate the presentation logic in directives.
  • Input formatting — Use angular form controls instead.
  • Output filtering — Use angular filters instead.
  • To run stateless or stateful code shared across controllers — Use angular services instead.
  • To instantiate or manage the life-cycle of other components (for example, to create service instances).
@ksperling
Copy link
Contributor

I agree with @jeme.

@ProLoser
Copy link
Member

ProLoser commented Apr 9, 2013

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

ProLoser commented Apr 9, 2013

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

jeme commented Apr 9, 2013

@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: angular/angular.js#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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants