Proposal for DeftJS enhancements needs review

[jacobg]

This message is mainly for John...

I'd like to enhance DeftJS to do the following:

• Support view controller behaviors (basically mix-ins) where each behavior may define controls, injections and observers that get merged into the view controller object.
• Deft.mvc.ViewController will iterate through each subclass, and do the following for each subclass:
  • merge controls into object
  • perform injections
  • merge unique behaviors

Below is basically what the code changes to Deft.mvc.ViewController look like. Please let me know what you think. Is this the right approach, or do you recommend something different? Will you accept a pull request if I go with this?

One note is that I took a different approach to merging controls than is already done with observers. That is, although observers are merged at class definition time, I chose to merge controls at class creation time. I made that decision, because in my own application, I find this to be more useful to be able to set/customize controls in the constructor.

Thanks.

Ext.define( 'Deft.mvc.ViewController',

    constructor: ( config = {} ) ->

        @mergedBehaviors = []
        @mergeParents()
        if @behaviors then @mergeBehaviors( @behaviors )

        if config.view
            @controlView( config.view )
        @initConfig( config ) # Ensure any config values are set before creating observers.
        if Ext.Object.getSize( @observe ) > 0 then @createObservers()

        return @

    ###*
    * @private
    * This is a specialized clone algorithm that goes two levels deep, and maintains object references.
    ###
    cloneControl: (control) ->
        if !Ext.isObject(control) then return control # probably null or undefined

        clone = {}

        for key, value in control
            if Ext.isObject(value)
                subClone = {}
                for key2, value2 in value
                    subClone[key2] = value2
                clone[key] = subClone
            else
                # probably true or a string selector
                clone[key] = value

        return clone

    ###*
    * @private
    ###
    mergeControls: ( target, source ) ->
        if !Ext.isObject( target ) then return source
        if !Ext.isObject( source ) then return target

        # clone target, so we can apply the source to it
        # without affecting the class prototype
        merged = @cloneControl( target )

        for key, value in source
            if key in merged and Ext.isObject( merged[key] )
                if Ext.isObject(value)
                    Ext.apply(merged[key], value)
                # else do nothing, because target value is object and source is not
            else
                merged[key] = value

        return merged

    ###*
    * @private
    ###
    mergeBehaviors: ( behaviors ) ->

        control = @control

        if Ext.isArray( behaviors ) or Ext.isObject( behaviors )

            for behaviorName in behaviors when behaviorName not in @mergedBehaviors
                behavior = Ext.ClassManager.get( behaviorName )

                if not behavior
                    # Doing syncRequire for each behavior in the contructor is not the most efficient way to
                    # load classes, but Deft is already doing this anyway for the controller classes. The
                    # optimal scenario is where the developer pre-loads what he needs.
                    Ext.syncRequire( behaviorName )
                    behavior = Ext.ClassManager.get( behaviorName )

                prototype = behavior.prototype

                #
                # Now do merge behavior introl class, which involves:
                #    * merge controls (controller class takes precedence)
                #    * merge observers (controller class takes precedence)
                #    * apply behavior injections
                #    * merge rest of behavior class (controller class takes precedence)
                #
                control = @mergeControls( prototype.control, control )
                @observe = Deft.mvc.Observer.mergeObserve( prototype.observe, @observe )

                if Ext.isObject( prototype.inject ) then Deft.Injector.inject( prototype.inject, @, false )

                Ext.mergeIf( @, prototype )

                @mergedBehaviors.push(behaviorName)

        @control = control

    ###*
    * @private
    ###
    mergeClass: ( Class ) ->

        # this object takes precedence over Class, because Class is a parent class
        @control = @mergeControls( @control, Class.control )

        # note that we do not merge observers, because normally that gets done directly
        # in the constructor for each class in the class hierarchy

        if Ext.isObject( Class.inject ) then Deft.Injector.inject( Class.inject, @, false )

        @mergeBehaviors( Class.behaviors )

    ###*
    * @private
    ###
    mergeParents: ->

        superclass = @superclass

        while Ext.isObject( superclass ) and superclass.$className != arguments.callee.$owner.$className
            @mergeClass(superclass)
            superclass = superclass.superclass

[johnyanarella]

I'd be very interested in this contribution.

With regard to Behaviors - I don't see them as mix-ins. I would prefer to see Behaviors implemented as standalone objects that encapsulate their own controls, injections and observers. The idea is that the ViewController "has" Behaviors (composition) that can be added and removed dynamically at runtime, and do not pollute the ViewController's namespace with potentially conflicting properties or methods.

[johnyanarella]

Discussing some of the class-creation time vs. runtime instance creation time merging concerns with Brian.

As Brian points out, the next thing everyone will need is support for extending Behaviors.

[johnyanarella]

In your example code you are manually initiating injection. Could you elaborate on why you are doing this? The inject preprocessor already merges inject directives and performs injection before executing the constructor of the leaf class.

[johnyanarella]

inject has to do class-time merging because it executes before the constructor.

I'm starting to think the ViewController and Behavior control, observe and behavior configs should be merged at runtime. And if the implementation is to be shared between ViewController and Behavior, perhaps that logic could be implemented in mixins.

[brian428]

I now remember the problem with trying to do it at runtime and it was the merged inheritance. Observe, for example, lets you specify observers in parent and child classes. Since we don't want the child config to just override the parent, they have to be merged into a combined config when the extend processor is doing its work. :-/

[jacobg]

Responses:

(1) Behaviors: I can see some of the benefits of decoupling the Behavior from the ViewController, rather than make it a mix-in, although that actually makes it quite more complex because it doesn't just copy its "stuff" into the ViewController and then disappear. It would seem then that there should be an abstract class that implements the shared functionality of a ViewController and a Behavior. The only difference is that the ViewController is tied to the view, whereas the Behavior is tied to the ViewController. I suppose if it were abstracted properly, perhaps Behavior could be extended just as ViewController can be.

(2) Injection: I presume you are asking only about manual injection of parent class injectors in the mergeClass method. I must have made a mistake in that, and should remove the manual injection there. However, since mergeBehaviors takes place in the constructor, I would have to manually inject there. That is, unless Behaviors is implemented as described above.

Takeaway:

Personally, although I can see the benefits of making Behavior decoupled, I don't have the time to implement it. It would require broader familiarity of the Deft codebase than I have, as I tried to narrow the scope of the code above to just changing the ViewController class.

Let me know if/how you would like me to proceed with the mix-in approach.

Thanks.

[brian428]

If I follow what John is saying, I do assume there would have to be a base Behavior class that these would extend. To add the to a VC, I imagine it would either look like:

\\ Behavior aliases are mapped to classes during app startup, like Injector.configure() does
behavior: [ "userDetail", "orderDetail" ]

or

\\ Just specify classes, like requires does
behavior: [ "MyApp.behaviors.UserDetail", "MyApp.behaviors.OrderDetail" ]

If either of those is on target, I'm not sure which would be better. I could see using aliases to keep it in line with how inject works. On the other hand, it would be an extra configuration step and I'm not sure there's much benefit to aliasing behaviors...

[jacobg]

Hi Brian - That's how I declare behaviors it my app. I did not implement alias resolution for it. Also as apparent from the code above, I mix-in behaviors in the constructor. That lets me decide whether to include a behavior at run-time, which gives me more flexibility.

[brian428]

Again, the only issue with processing the behaviors at runtime and not at extends time means behaviors defined in a subclass will nullify any defined in a superclass and replace them.

[jacobg]

I'm starting to see that it would be okay as you say: to process behaviors when the class is extended. I'm looking at my own code at why I thought I wanted them at runtime, and the it really was just a side-effect of not giving the behaviors their own init method. If we did that (through making them encapsulated), it should be fine.