davej / angular-classy

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

Ease usage of watch when using controllerAs syntax #49

Closed ackwell closed 8 years ago

ackwell commented 8 years ago

There's been quite a push for use of the controllerAs syntax across the board, and while Classy mostly supports it just fine, there's a bit of a gotcha with the watch section, as it binds onto the $scope.

Current workarounds:

Proposal:

The third workaround seems to be the cleanest solution, so I'd think it worth looking into if it could be incorporated in some manner. Without looking into it really deeply, I'd think that it would be safe to automatically switch handling of watch to that implementation when addToScope is false - or potentially a separate config option.

Apologies if I've completely overlooked something obvious, I did check over what was around before writing this up but I may well have missed something.

davej commented 8 years ago

Could you give me a bit more detail on the third workaround with code on how you would currently write this in classy. It's late here and my brain isn't working :-)

ackwell commented 8 years ago

The third example is based on this StackOverflow answer. Angular's docs for $scope have info in the $watch section about passing a function instead of an expression.

It's possible to get Angular to watch anything provided you can reference it, hence the re-bound this so we retain reference to the controller object.

The current implementation in a classy controller would be something along the lines of:

module.classy.controller({
  __options: { addToScope: false },
  name: 'MyController',
  inject: ['$scope'],
  init: function() {
    $scope.$watch(angular.bind(this, function() {
      return this.changingProperty;
    }, function (newVal) {
      // (changingProperty has been modified)
    }
  },
  data: function() { return {
    changingProperty: 'someProperty'
  }; }
});

Or something along those lines. That particular example won't handle deep objects, but the usual handling for that should work just fine.

davej commented 8 years ago

Ah, I understand. I'm wary of introducing a backwards incompatible change by switching the implementation when addToScope is false.

Would something like this be acceptable instead?

watch: {
  '{this}changingProperty': function(newVal) { }
},

You could double up the keywords if needed '{object}{this}changingProperty'.

davej commented 8 years ago

Another solution might be to add another option that binds directly to the class for watch:

__options: { addToScope: false, bindWatchToClass: true },
ackwell commented 8 years ago

Given that this solution would need to be utilised on every watch expression in controllers using addToScope: false, I'd opt for the latter. Saves cluttering up all the expressions with extra keywords.

davej commented 8 years ago

Ok, shouldn't be too much hassle to implement this. I'll try and do it if I get some spare time over the coming days. :-)

ackwell commented 8 years ago

Awesome, thanks for such a quick response!

davej commented 8 years ago

Another thing to note is that expressions won't work when using bindWatchToClass (or whatever it ends up being called).

For example, this won't work:

// ...
__options: { addToScope: false, bindWatchToClass: true },
watch: {
  'firstName + secondName': function(newVal) { }
},
// ...
davej commented 8 years ago

Could be neat to introduce a method keyword as an alternative to expressions as well:

__options: { addToScope: false, bindWatchToClass: true },
watch: {
  '{method}getFullName': function(newVal) { }
},
methods: {
  getFullName: function() {
    return this.firstName + this.secondName;
  }
}

Might seem a bit verbose but a reusable function is good structure and it's not so bad in es6/coffeescript: getFullName: () => this.firstName + this.secondName

davej commented 8 years ago

Just published v1.2.0 with support for bindWatchToClass. I was able to figure out a way to also support expressions too. :-)

@ackwell: Try it out and let me know if it works for you.

ackwell commented 8 years ago

Awesome!

I've not got the codebase on me at home, but I'll pull across the new version first thing Monday and see how it runs.

Again, thank you for your quick work!

ackwell commented 8 years ago

Yep, just updated the copy of classy in this project, and bindWatchToClass is working perfectly.

Perfect, thanks!