davej / angular-classy

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

Extending controllers #10

Closed crysfel closed 10 years ago

crysfel commented 10 years ago

Hi there!! This project is so great, Im already using it on one of my apps, my code looks beautiful with classy.

I would like to be able to extend a controller, just define a parent controller with a few common methods and then extend that controller so all the children controllers inherit all properties and methods from the parent. Something like:

MyApp.classy.controller({
    name    : 'ParentController',
    inject  : ['$routeParams','CommonService'],

    init    : function() { },

    commonMethod : function(){}
});

And then extend from that controller.

MyApp.classy.controller({
    name    : 'ChildController',
    extend  : 'ParentController',  //extending from parent
    inject  : ['anything'],

    init    : function() {
          this.parent.init.call(this);  //calling the init on super class

         this.CommonService.something(); //super class injects this service
    },

    other : function(){
         this.commonMethod(); //calling a method from the super class
    }
});

This will allow us to reuse code between controllers. Do you have any plans to add a feature like this? If not, is it something you'd accept a PR for? Looking forward to hear from you.

ruud commented 10 years ago

That would be interesting! At the moment I am using $injector.invoke to do this. But your suggestion is better

Next to controller inheritance, mixins could be useful too to share common methods.

davej commented 10 years ago

This will be possible in some form.

My only issue with this particular solution is that the name property will register the controller. If you create a base class that you inherit from then you may not want it to be registered as a controller.

ruud commented 10 years ago

Something like the following would be useful too: mixins : ['mixin1', 'mixin2'],

chiefy commented 10 years ago

:+1:

arsuceno commented 10 years ago

I'm working in a mixins approach using $controller, quite similar to watchers, but I don't know if it should be the way to do it. In construct function:

        if (options.watchObject && angular.isObject(parent.watch)) {
            this.registerWatchers(parent);
        }
        if (options.useMixins && angular.isObject(parent.mixins)) {
            this.registerMixins(parent);
        }

And registerMixins looks like this:

    registerMixins: function (parent) {
        var $scope, mixins, $controller;
        $scope = parent[parent.constructor.prototype.__options._scopeName];
        if (!$scope) {
            throw new Error("You need to inject `$scope` to use the mixins");
        }
        mixins = parent.mixins;
        $controller = angular.element(document).injector().get('$controller');
        if (angular.isArray(mixins)) {
            for (var i = 0; i < mixins.length; i++) {
                $controller(mixins[i], { '$scope': $scope });
            }
        }
    }

So in my controller I can do:

mixins: [
    'MixedCtrl'
],

In my tests this work OK so maybe it can be useful for others.

davej commented 10 years ago

@arsuceno Getting the injector for the document element may not work as it depends on how ng-app is defined in the markup.

In 0.5, Classy will become modular and you will be able to create plugins for this sort of thing. In fact you can do it now if you use the develop branch. See the last comment on #5 for details on how to create a plugin. You should be able to just use the classy-watch plugin as a template. You can use localInject as a safe way to get a local instance of $controller.

I haven't decided exactly what features will go in Classy core yet but mixins would definitely be a candidate. :-)

wuxiaoying commented 10 years ago

I wanted this too. I took a shot at it, but I'm not really happy with it. I wrote what arsuceno had and added function binding, but it has to be included after bindDependencies and it doesn't get the base class dependencies (see below). Also, I would much rather have inheritance, which should be easy in coffeescript...if I had the controller class, but I think it's hard to get that from the $injector... Any suggestions?

initScope_module = angular.module 'classy-mixins', ['classy-core']

initScope_module.classy.plugin.controller
name: 'mixins'

options:
    enabled: true
    ignore: ['constructor', 'init', 'initScope']

isActive: (klass, deps) ->
    if @options.enabled and klass.mixins
        if !deps.$scope
            throw new Error "You need to inject `$scope` to use the watch object"
            return false

        return true

preInit: (classConstructor, classObj, module) -> 
    if classObj.mixins and classObj.inject.indexOf '$injector' == -1
        classObj.inject.push '$injector'
    return

init: (klass, deps, module) ->
    if @isActive klass, deps
        $controller = deps.$injector.get '$controller'
        for mixin in klass.mixins
            mixin = $controller mixin, 
                $scope: deps.$scope
                $element: deps.$element
                $attrs: deps.$attrs
            for key, fn of mixin.constructor::
                if angular.isFunction(fn) and !(key in @options.ignore)
                    klass[key] = angular.bind(klass, fn)
    return
davej commented 10 years ago

Thanks @wuxiaoying, looks good.

I would much rather have inheritance, which should be easy in coffeescript...if I had the controller class, but I think it's hard to get that from the $injector... Any suggestions?

The classConstructor param in the preInit function will give you the controller's class. That should give you what you need?

Also, you should be able to use localInject instead of manually pushing it to the controller's inject array.

#...
localInject: ['$controller']

init: (klass, deps, module) ->
    if @isActive klass, deps
        for mixin in klass.mixins
            mixin = @$controller mixin, 
                $scope: deps.$scope
                $element: deps.$element
                $attrs: deps.$attrs
            # ...

Here's some plugin documentation if you haven't seen it already: https://gist.github.com/davej/6d5abc80d6098362749e

Also feel free to ask questions if anything is unclear :+1:

wuxiaoying commented 10 years ago

Oh well wow thats helpful; I had not seen the docs before. Well pre init gives the current class but i would actually need the base class, which i cant find an easy way to get.

On Saturday, June 21, 2014, Dave Jeffery notifications@github.com wrote:

Thanks @wuxiaoying https://github.com/wuxiaoying, looks good.

I would much rather have inheritance, which should be easy in coffeescript...if I had the controller class, but I think it's hard to get that from the $injector... Any suggestions?

The classConstructor param in the preInit function will give you the controller's class. That should give you what you need?

Also, you should be able to use localInject instead of manually pushing it to the controller's inject array.

...localInject: ['$controller']

init: (klass, deps, module) -> if @isActive klass, deps for mixin in klass.mixins mixin = @$controller mixin, $scope: deps.$scope $element: deps.$element $attrs: deps.$attrs

...

Here's some plugin documentation if you haven't seen it already: https://gist.github.com/davej/6d5abc80d6098362749e

Also feel free to ask questions if anything is unclear [image: :+1:]

— Reply to this email directly or view it on GitHub https://github.com/davej/angular-classy/issues/10#issuecomment-46766646.

davej commented 10 years ago

Yes but if the base class is a classy controller then it will pass through the preInit function at some stage so you should be able to keep some kind of internal register.

baseClasses: []

preInit: (classConstructor, classObj, module) ->
    if classObj.isBaseClass
        # Record internal register of base classes
        @baseClasses.push classObj.name, classConstructor

Remember though, that if a Classy Controller has a name then it will be registered as a controller, I'm not sure if that is desirable, perhaps I can add a dontRegister flag to Classy?

Another possibility is that you could reference the classy controller directly, in this case you wouldn't need to keep an internal register and you wouldn't have to give it a name property (which would prevent it from being registered):

baseController = app.classy.controller
    init: ->
        alert('I am the base controller')

app.classy.controller
    name: 'MyCtrl'
    inheritsFrom: [baseController]
wuxiaoying commented 10 years ago

Oh, I see what you mean. I actually tried that and the problem I had was that the base class wasn't initialized, but I'm sure I can inject it first somewhere. Both are good options, I'll play around with them tomorrow ^.^ Thank you.

davej commented 10 years ago

I actually tried that and the problem I had was that the base class wasn't initialized

That's a good point. I can add some functionality to Classy that would make this possible.

I could possibly add more phases to the lifecycle of plugins.

So at the moment it looks like this: preInit -> init -> postInit

I could add some steps, perhaps something like: preInitBefore -> preInit -> preInitAfter -> initBefore -> init -> initAfter -> postInitBefore -> postInit -> postInitAfter.

You could then register the base classes in preInitBefore and you would be sure that they would all be available in preInit and later.

Would that give you what you need?

davej commented 10 years ago

Made a commit to the develop branch that adds more phases to the plugin lifecycle. This should help you do what you need @wuxiaoying.

wuxiaoying commented 10 years ago

Hmm, still having trouble with it, because I don't have local inject dependencies at the time of preinitBefore, so it's hard to initialize it

davej commented 10 years ago

If you want to post your code to a gist then I can take a look and see if I can make any suggestions.

wuxiaoying commented 10 years ago

This is what I was thinking of doing:

https://gist.github.com/wuxiaoying/f118fd6bbfbb89e497d9

odiak commented 10 years ago

I wrote code to extending controllers, inspired by @wuxiaoying's gist https://gist.github.com/odiak/5df72e172ee312e96b0a

wuxiaoying commented 10 years ago

thanks @odiak that looks awesome :)! very excited to try it out

juangabreil commented 10 years ago

I've forked @odiak gist and extended it to support call to superclass methods, it's based on @jeresig class.js

https://gist.github.com/juangabreil/af377b0f3e66d123bfd9

wuxiaoying commented 10 years ago

Thanks @juangabreil. That looks great :). Maybe we should start a repository for all the plugins?

I made a quick one here: https://github.com/wuxiaoying/angular-classy-plugins. Hopefully we can unit test them too ^^.

chiefy commented 10 years ago

Any chance @davej will include this in the next release? Seems pretty useful.

davej commented 10 years ago

Cheers guys, this is great.

My plan is to list these as plugins on the classy site when 0.5 is released, if there is an obviously "best" implementation I will look at moving it into core Classy for 0.6 or later.

I think it is good that there are competing implementations for the moment (e.g.. extends vs. mixin) and it is too early to decide which should be in core.

I plan to release a more comprehensive plugin development guide soon, with more information on best practices and testing.

davej commented 10 years ago

Closing. Please see: https://github.com/wuxiaoying/classy-extends