davej / angular-classy

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

New structure for class object #32

Closed davej closed 10 years ago

davej commented 10 years ago

I'm hoping to push Angular Classy 1.0 in the next couple of weeks. The major feature will be support for plugins (see https://github.com/davej/classy-computed for an example).

This has got me thinking about object namespace collisions between controller methods and object keys used by plugins. Up to this point the only object namespaces that are used are 'name', 'el', 'selector', 'inject', 'init' and 'watch'. With plugins this could be set to expand and could potentially get a bit messy.

One solution for this is a standalone 'methods' object for storing controller methods.

So a Classy 1.0 controller might look something like this:

app.classy.controller({

  name: 'TodoCtrl',

  inject: ['$scope', 'todoStorage', 'filterFilter'],

  init: function() {
    this.$.todos = this.todoStorage.get();
  },

  watch: {
    '{object}todos': '_onTodoChange'
  },

  methods: {
    addTodo: function() {
      var newTodo = this.$.newTodo.trim();
      this.$.todos.push({
        title: newTodo,
        completed: false
      });
    },

    _onTodoChange: function(newValue, oldValue) {
      this.$.remainingCount =
        this.filterFilter(this.todos, { completed: false }).length;
    }
  }

});

I'd like to get some opinions and thoughts on this approach.

odiak commented 10 years ago

That's nice. :+1:

davej commented 10 years ago

Any thoughts or feedback on this? @wuxiaoying @davemo @bwu-aa @MiracleBlue @anthony-ism @MikeAK @jeffbcross @madbarron @crysfel @dcramer @checketts @chrisnicola

Sorry about being spammy, please ignore if you're not interested.

davemo commented 10 years ago

Could you expand on a problem scenario with an example in code?

At first glance it seems like it would work but I find myself hesitant about expanding the API with explicitly naming the methods key. When I think back to the Backbone.View API it was fairly simple to override builtins with _.extend and plugins never seemed to have any issues with the potential for namespace collisions.

If you're that worried about the potential for the blessed classy object keys to be overridden by plugins you could implement some sort of detection scheme given you are just passing the object to app.classy.controller so that it would warn the user if a namespace and/or key got clobbered; my hunch is that this sort of defensive coding isn't necessary though.

davej commented 10 years ago

Could you expand on a problem scenario with an example in code?

A contrived example might be something like:

app.classy.controller({
  name: 'VideoCtrl',
  inject: ['$scope', '$routeParams'],
  init: function() {
    this.watch($routeParams.videoUrl)
  },
  watch: function(url) {
    // Loads a youtube video for you to watch
    var video = this.load(url);
    this.play(video);
  },
  load: function(url) { },
  play: function(video) { }
});

This wouldn't work because Classy expects the watch property to be special and not a controller method. It's also conceivable that there would be a plugin that would do things with the load property.

I think my issue is not only with namespace collisions but also with the fact that there will be special properties (like watch, init etc..) and controller methods mixed together and the distinction is not obvious from looking at the code. This feels messy and unstructured to me and these are two things I don't want for Classy.

wuxiaoying commented 10 years ago

Maybe prefixing like angular does with $ would be another option?

davej commented 10 years ago

@wuxiaoying: That's an interesting solution, just to play devil's advocate, a few issues:

  1. AngularJS already uses some of these namespaces for dependencies ($filter, $log etc..), a quick look through the api reveals 40+ of these. In the way that Classy currently works this could cause a collision between the dependency and the plugin.
  2. Is it all getting a bit confusing? this.$watch (Classy plugin), this.$.$watch ($scope.$watch), this.watch (controller method) and this.$.watch (controller method reference on scope).
davej commented 10 years ago

I've pushed beta 2 of Angular Classy 1.0. There's a page detailing the main changes here: http://davej.github.io/angular-classy/beta.html

I think I'm going to push ahead with the methods property, it feels like the right solution. I've also introduced a data property for keeping simple assignment out of the init method (similar to @wuxiaoying's initScope plugin but it can also use Angular expressions).

I've been using the methods object with Classy for a few weeks now and I have to say that I really like it, it feels much more structured and clean. If you're currently using pre-1.0 controllers, I've created a simple plugin for maintaining compatibility with Classy 1.0 without changing your controllers: https://github.com/davej/classy-compat

By the way, if anybody wants help with creating a plugin then just create an issue. Or, if you prefer, I'd be happy to jump on a google hangout or skype call to give a few pointers :-).