davej / angular-classy

Cleaner class-based controllers with Angular 1
http://davej.github.io/angular-classy/
813 stars 27 forks source link

Incorrect context is bound to promise success handlers #8

Closed davemo closed 10 years ago

davemo commented 10 years ago

the problem

screen shot 2014-04-28 at 12 43 38 pm

Given the following controller definition using classy and coffeescript:

app = angular.module 'app'

app.classy.controller

  name: 'LoginController'
  inject: ['$scope', '$location', 'AuthenticationService']

  init: ->
    @credentials =
      username: ""
      password: ""

  login: ->
    @AuthenticationService.login(@credentials).success(@_redirect)

  _redirect: ->
    #@$location is undefined here, the context for this function is the global, `window`
    @$location.path '/home' 

I named my redirect function with an underscore to avoid having it placed onto the scope but there is no way to bind the execution context of _redirect such that this will resolve to the Constructor as it does inside the login function.

Using => (or, Function.bind if you are in vanilla JS) doesn't work either, as there is no external way to get a handle on the Constructor that classy creates.

proposed solution

  1. Make all function members of the object literal passed to classy.controller be bound to the Constructor. This works similar to Backbone.View definitions where functions defined with the object are auto-bound to the instance of the view.

Thoughts?

davej commented 10 years ago

Thanks that makes a lot more sense. I've merged it into the develop branch and more than likely I'll include it in a bugfix release tomorrow. Technically, it breaks backwards compatibility but the version number is still 0.x so I think it's ok.

davemo commented 10 years ago

I ran tests and things didn't seem to break, is there some other backwards compatible change I didn't notice that this causes?

davej commented 10 years ago

No, I just mean that this now refers to something different in some circumstances. Technically, that's a backwards incompatible change. Not sure why I mentioned it, it doesn't really matter because not many people are using Classy yet and this is a bugfix. Thanks for the patch, I'll do a release soon.

davemo commented 10 years ago

Right, the usurping of this in functions is a standard problem I've encountered across projects in Backbone and other frameworks that utilize jQuery and map jQuery event handling functions into a domain object (like Backbone.View or classy.controller). Consistency is the name of the game here; I think you're doing a good job being cautious about not wanting to introduce backwards compatibility breaking changes, so keep on that :)