YahooArchive / mojito

[archiving soon] Yahoo! Mojito Framework
BSD 3-Clause "New" or "Revised" License
1.57k stars 215 forks source link

Allow mojits to extend YUI modules in other mojits #1385

Closed aljimenez closed 10 years ago

aljimenez commented 10 years ago

This pull request allows mojits to easily extend YUI modules of other mojits. For example:

mojits/Vehicle/controller.server.js

YUI.add('VehicleController', function (Y, NAME) {
    var Vehicle = function () {
        this.type = 'Vehicle';
        this.transmission = 'automatic';
    };

    Y.namespace('mojito.controllers')[NAME] = Vehicle;
    Y.extend(Vehicle, Y.Base, {
        index: function () {
            ac.done(ac.params.body('brand') + ' ' + this.getType());
        },
        getType: function () {
            return this.type;
        }
    });
}, '0.0.1', {
    requires: {
        'oop',
        'mojito-params-addon'
    }
});

mojits/Motorcycle/controller.server.js

YUI.add('MotorcycleController', function (Y, NAME) {
    var Vehicle = Y.mojito.controllers.VehicleController,
        Motorcycle = function () {
            Motorcycle.superclass.constructor.call(this);
            this.type = 'Motorcycle';
            this.transmission = 'manual';
        };

    Y.namespace('mojito.controllers')[NAME] = Motorcycle;
    Y.mojito.util.extend(Motorcycle, Vehicle);
}, '0.0.1', {
    requires: {
        'mojito-util',
        'VehicleController'
    }
});

In this example, the Motorcycle mojit extends the Vehicle mojit, which extends the Y.Base class (extending Y.Base is not a requirement, the example just illustrates multiple level of extensions). Notice that the controllers can be defined as a function; this allows mojits to extend using Y.extend, which ensures that the prototype chain is maintained regardless of how many levels of extensions. Also notice that the Motorcycle controller does not have to re-specify the 'mojito-params-addon' requirement; it just has to specify the Vehicle controller requirement, and its addon will automatically include any addon required by Vehicle controller.

To ensure that mojit dependencies work with resourceStore > lazyMojits on, mojits depending on other mojits should specify their dependencies in defaults.json:

mojits/Motorcycle/defaults.json

[{
    "settings": [ "master" ],
    "dependencies": [ "Vehicle" ]
}]
yahoocla commented 10 years ago

CLA is valid!

gomezd commented 10 years ago

IMHO we need to do extensive performance testing to determine any possible impact of this. And i'm not a big fan of having to declare the dependencies in another file (defaults.json), seems to me confusing and error prone.

aljimenez commented 10 years ago

This pull request does not change the execution flow for any existing application. It only adds a new feature, which is to allow a mojit controller to be specified as a function with a prototype. This has no performance impact, since the execution flow remains the same. The only thing that changes is that instead of mojito using Y.mojito.util.heir to create the controller object (see heir), the controller object is created directly from the controller function. This means that we don't have to create a new function every time, which obviously wouldn't degrade performance or memory usage. Specifying the dependency somehow is necessary only if using lazyMojits, there is no way around this, and shouldn't be a huge issue for users since extending the modules of another mojit should be rare. In fact without the pull request mojits extending another mojit's modules would have to specify the dependency through YUI().applyConfig (see ShakerPipelineFrame), in case lazyMojits is on. Specifying the dependencies in default.json is probably the cleanest most intuitive way (and probably good practice anyways since in general mojits are self contained, so its a good idea to mention that it depends on another mojit in its configuration).

caridy commented 10 years ago

This PR is misleading, it is not extending mojits, it is extending individual modules within a mojit, and that's supported today as far as I can tell, by relying on the YUI module definition, I wonder why are you trying to mix the concepts of a mojit and modules in this PR.

aljimenez commented 10 years ago

Thats right, you can extend any YUI module of another mojit, including controllers, binders, models... This pull request enhances 3 things:

  1. When you require a controller, you also get the addons of that controller. Before if you required an external controller you would also need to copy the list of addons required by that controller. (See this example)
  2. The pull request allows you to use the Y.extend directly on the controller, since now controllers can be defined as functions. This is important in order to maintain the prototype chain regardless of how deep the level of extending. For example mojit c's controller can extend mojit's b, which extends mojit's a's controller. Previously you would have to use Y.merge since Y.extend only works on functions not object literals. And if you used Y.merge you would lose the prototype chain and mojit c's controller would not properly inherit mojit's a controller methods.
  3. When using lazyMojits, mojits are loaded as they are needed, however if a mojit depends on the YUI module of another mojit, then those YUI modules would not be available. This is currently solved by having a Y.applyConfig statement before doing YUI.add (see ShakerPipelineFrame). Defining the mojit's external dependencies in defaults.json, is a cleaner solution since it doesn't require the applyConfig statement and is also an easy way to see if this mojit depends on another mojit.
aljimenez commented 10 years ago

In the latest commit, Y.mojito.util.heir accepts functions in addition to object literals. This allows controllers/binders/models to be defined as functions, which can be useful when extending other YUI modules. The commit also adds Y.mojito.util.extend, a wrapper around Y.extend, in order to accept either functions or object literals as arguments.

caridy commented 10 years ago

need some time to go over this tomorrow.

aljimenez commented 10 years ago

Thanks Caridy, let me know if I should clarify anything.

aljimenez commented 10 years ago

@caridy can you review this when you get a chance; we have some application code that depend on this pull request. Thanks.

caridy commented 10 years ago

@aljimenez let's have a chat tomorrow about this PR, I have few specific questions and some suggestions, but this is a very complex PR, hard to make sense of all pieces.

aljimenez commented 10 years ago

@caridy sounds good. The pull request did get a bit complicated due to some refactoring and some other fixes that I slipped in, somewhat unrelated to the original pull request. I'll make sure to separate the distinct features in different branches in the future, thanks.

caridy commented 10 years ago

ok, I will not hold back on this: +0.9

aljimenez commented 10 years ago

Thanks @caridy I will merge this soon, and I'll keep in mind your suggestions.