Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Extend modules #837

Closed
jamesplease opened this issue Jan 5, 2014 · 26 comments
Closed

Feature request: Extend modules #837

jamesplease opened this issue Jan 5, 2014 · 26 comments

Comments

@jamesplease
Copy link
Member

As it stands it is very difficult to extend Modules to create a new base type in your app. This makes Modules unique, in that all of the other components of Marionette (and Backbone) are easily expanded upon. For instance, if I wanted to make a new 'base' type of ItemView, I could easily accomplish this with by Backbone extending the class to make a new one, then just instantiate that new one whenever I wished.

This isn't possible with Modules for a number of reasons.

  1. A method is used to add them to your application, Application.module, which forces you to use Marionette.Module.create... (Line 86). This prevents you from defining a new 'base' module from which to extend from.
  2. The two ways to initialize a Module are limiting in their ability to be extended. If you pass a function then, well, you can't extend it and there's nothing you can do. The second way is an object literal, but it's impossible to add initializers to this, or anything that executes 'behind the scenes.' For example, I were to add a 'customMethod' function that I wished to run whenever the module was initialized, the user would need to hardcode the call to that function in an initializer on their own. This also isn't ideal.

I didn't make a PR on this issue just yet because I want to make sure whichever solution I code up is acceptable to the maintainers of Marionette.

The best option, I think, would be to allow users to override that line 86 of Application programmatically. This allows the initialization of modules to remain the same so that backwards compatibility isn't lost. One way to do this would be to pass an optional third argument to the Application.module method which is the class to instantiate on line 86. If it's missing it defaults to the current class.

Thoughts?

@cobbweb
Copy link
Member

cobbweb commented Jan 5, 2014

Edit: Some discussion below on me failing to understand the requirements of the request, jump to this comment for a TLDR version :).


What functionality are you looking to add in a custom module class?

@jamesplease
Copy link
Member Author

Since my initial post, I've discovered the same issue applies to Regions so I've edited the issue accordingly.

To answer your question, I'm building a collection of reusable components to use across different applications. These components are groups of views and a module that can be inserted into a page to add some functionality. For instance, there might be a pie graph component that I want to abstract from a given app. You add it as a module to your app to include it. To make it work, all you need to do is pass it a model to get the data from and the region to put it in, and it does the rest for you (like magic!)

To accomplish my goal I need to extend the functionality of every Marionette class in each components (the item views, the controllers, and so on). One thing I want to add to everything is a local wreqr to handle events internal to the components. This is easy enough with things like ItemViews; I can just extend the base class and instantiate those instead of the Marionette ones. But I'm unable to do this with modules and regions.

I got around the issue of attaching the wreqr directly to the module by attaching it to a controller instead, but my regions are still left out of the communication loop. There's no way to give the regions in my components a way to share with the rest of the component that it's been rendered.

An example of why this is important is the following: consider that my pie graph is added to the DOM at some time, but takes a second to fully animate its introductory animation. I would like to internally share both of these events with my component using the wreqr shared among its components, because both of these things are important. Eventually, for instance, they are intended to be shared on the application's wreqr channel.

Maybe there's some way to get around this that I'm not seeing. If you have any ideas I'd love to hear them.

@cobbweb
Copy link
Member

cobbweb commented Jan 5, 2014

You can specify custom region class when declaring them on the app, take a looks at the docs. Hopefully that sorts you out for Regions.

As for modules, I'm still not entirely understanding what sort of custom functionality you might attach to a module. Do you think you could provide a brief concrete example on how you'd leverage custom Module classes? Sometimes times code is worth a thousand words :)

@jamesplease
Copy link
Member Author

Ah, look at that. I should have taken another look at the docs before updating regarding regions.

In regards to modules, it seems that switching things over to a controller has solved my issues with needing a custom module type for now. I'll go ahead and mark this issue as closed.

Thanks, cobbweb!

@cobbweb
Copy link
Member

cobbweb commented Jan 6, 2014

Awesome, glad I could help. :) Yea I had a feeling you might have been stretching the purpose of Marionette.Module a little far. They're more like code organisation modules rather than UI/widget type modules. That's an interesting concept though so thanks for bringing it up.

@jamesplease jamesplease reopened this Jan 6, 2014
@jamesplease
Copy link
Member Author

I'm reopening this because as I'm writing more code I think the issue may still be unsolved, actually. I am trying to use modules as a UI/widget module with this structure, and I think this makes more sense than using any other type of Marionette object. Modules can be plugged in, started, and stopped directly from the application. This is ideal for UI/widgets! It's hard for me to think of any other Marionette class that fits the bill for how I'd want a UI/widget module to behave.

To show some code, most modules I make will need the following code in an initializer:

this.controller = new Components.Controller({
  app: app,
  module: this,
  view: options.view
  region: options.region
});

where Components.Controller is a special controller for these types of modules. But on some modules, I might not want this at all. Or maybe I'll want to use a different controller. Or maybe I'll pass more options in. In any event, I need a way to overwrite this initializer. The best way to do this, of course, is to extend the base module class and add another initializer. But I'm unable to do this right now.

It seems like adding a way to attach modules like regions are attached would improve the utility of Modules and allow them to be used as a UI/widget class. But perhaps it'd be better to just use a controller and its init function for these components?

@cobbweb
Copy link
Member

cobbweb commented Jan 6, 2014

Modules can be plugged in, started, and stopped directly from the application. This is ideal for UI/widgets!

Yes you're right, I take my words back. You're aware that you can add Module Initializers right?

@jamesplease
Copy link
Member Author

Yeah, I'm familiar with that feature. And I might be wrong here, but I don't think it can be used for extending classes that's as 'behind the scenes' as extending, say, an ItemView.

Let's say I have some base module, say, Components.ModuleBase, that has an initializer, startUp. Is it possible for me to create a Components.ModuleOne that overwrites startUp of the base module before being attached to the application? The way the docs are written suggests that this is only possible after the application has been involved, like down here. Essentially, I'd like to split it across multiple files before attaching it to the app.

@cobbweb
Copy link
Member

cobbweb commented Jan 6, 2014

A bit sleep deprived at the moment, thanks for bearing with me. Could you disable startWithParent, then you can dynamically add an initializer and start the module as you please?

If you wanted something generic to occur with each module, you could move that code to a shared function which you might attached to your app. Then just add that as an initializer, if you wanted to override it, simple don't at it as an initializer for that module and register a custom one.

If that's the case, in does feel like we're working around limitations with the module class and I'm starting to warm up to your idea of allowing custom modules.

@jamesplease
Copy link
Member Author

Yeah, sorry, it's probably partially my fault at not clearly expressing my thoughts here. But you're definitely on the right track with the generic code occurring at the start of each module. However, I want to go even further than that. I want it to have two important properties: I want it to be automatic (the user shouldn't need to manually add the initializer), and I want it to be overwrite-able. These are the exact same properties that Backbone's extend method gives us, which is why the default Backbone objects (and many of the default Marionette ones) are so useful.

To explain the extend bit more, what I'm aiming for is very similar to the relationship between Views and ItemViews in Marionette. There's a base thing, a View, that the user can initialize. When they do, some generic code runs which the user doesn't need to call, or write, or anything. It's just built into views. But the user can go ahead and extend the View and make something new (like an ItemView), that also has generic code that runs, and is also automatic.

I'm thinking that if modules were this same way, then Marionette would be in a good place for a UI/widget framework to be built on it.

One important thing to note is that I don't think the addInitializer has the same functionality as the constructor function, as addInitializer just seems to append another function, rather than giving overwrite capabilities.

Does this help explain it better? Sorry if I'm being unclear, or just repeating myself over and over.

@cobbweb
Copy link
Member

cobbweb commented Jan 6, 2014

Yep starting to make sense now, I'll think this over today :)

@cobbweb
Copy link
Member

cobbweb commented Jan 7, 2014

TLDR: Need a way to add generic start up functionality to every module by default, which could be overridden/disabled on a per module basis.

The request seems viable, and I can't see a way to do this without monkey-patching the core... what do you think @samccone?

Two ways I can see this happening:

  • Allow custom module classes (i.e. var MyAppModule = Marionnete.Module.extend({ ... })) and then you could specify that class when doing MyApp.module(..., ..., MyAppModule)
  • Allow some sort of cascading onModuleStart property on modules. When a module starts, it will look for this on itself, then keep looping up through it's parents until it finds one (top level being the Application). It would stop once its found one, or do nothing if doesn't find one. I don't think this is the way to go, but its an interesting idea nonetheless.

@samccone
Copy link
Member

samccone commented Jan 7, 2014

hmm @cobbweb or @jmeas can you provide a real world use case and example code :)

@jamesplease
Copy link
Member Author

Use case: Building reusable "modules" that can be plugged in to, and removed from, applications at will. These modules are self-contained groups of classes that have an API to interact with them, but otherwise they require minimal set up to add to your app. They might involve a view, in which case you'd pass it a name (the name of the instance of the module), a region (where to render it), and a model (the data used by the module).

I want to use modules because of their nice properties of plugging them into and out of applications. And I want to assume a default structure to my modules in a way very much like how Marionette assumes a default structure to its ItemViews. But I also want the user the ability to completely change everything about a module by extending the class, just like how Marionette has implemented its ItemViews.

I'm beginning to create this idea over at this incredibly nascent repository, Puppets. Here's an example 'base module', which is using controllers now with a shim to imitate the plug-and-play ability of Marionette modules.

Here's an example Puppet that adds a bit of functionality to the base one – not too much, but enough to get the idea, I think.

@samccone
Copy link
Member

samccone commented Jan 7, 2014

cool @jmeas I get what you are going for.

Can you make like a super simple example just so we have an idea of scope of what it can touch / interact with along with a mock interface API?

@jamesplease
Copy link
Member Author

Sure. Clone this repository and run bower install to pull down the puppets core. Then open up the HTML file under tests/fixtures.

By default, the puppets are closed. You can open them up by executing the command [name]:show. There are four instances of the same puppet on that page, and their names are: scheduleInfo, preTweets, summary, and tester. As an example, you you can use app.commands.execute('scheduleInfo:show'); to display it.

The command [name]:close will close it down. These puppets open and close differently based on the options passed to them in the init file.

So open and close is part of the default command API for puppets. They also have some default requests, which you can see here.

Of course, by extending this base puppet class you can see how it's be easy to add more API commands and requests.

@jamesplease
Copy link
Member Author

If you'd like, I could make a PR implementing the first suggestion in this comment for review. That suggestion seems more powerful than the other suggestion, seems easier to implement, and would allow for a more consistent development process for extended modules.

The consistency would come from giving modules the same Backbone.extend functionality that the other components have.

@samccone
Copy link
Member

samccone commented Jan 8, 2014

that would be a great start @jmeas

@ccamarat
Copy link
Contributor

I'm a bit late to the game here, but this thread caught my eye because I've had to work around this exact issue. The technique I used to extend base functionality in Marionette modules was to supply a defaultDefinition function, something like this:

ModuleDefaults.definition = function (module, app, Backbone, Marionette, $, _) {
    module.homePath = '';
    module.hasStylesheet = function() {
        return !!module.cssPath;
    };
    module.addInitializer(function () {
        if (_.isFunction(module.initialize)) {
            module.initialize();
        }
    });
};

Because multiple calls to Application.Module() end up mimicking the extend capability, we create our modules by doing the following:

App.module(_moduleName, ModuleDefaults.definition);
App.module(_moduleName, function (Module, App, Backbone, Marionette, $, _) {
    Module.initialize = function() {
        // do something wild
    };
};

This all has worked great for us, but I think the Module.extend capability in the PR is a much more elegant solution. So I guess what I'm trying to say here is I look forward to backing out the above hack. Thanks @jmeas! 👍

@rhubarbselleven
Copy link
Contributor

👍

Interestingly from the 1.0 post: http://lostechies.com/derickbailey/2013/03/25/marionettejs-v1-0-now-with-stickers/

There are still more than 50 tickets open in the Marionette repository, and a lot of things that I want to do to help re-stucture some of the ugly parts. I want to tear apart Modules, for example, and create separated namespacing, sub-applications and components.

@jamesplease
Copy link
Member Author

I'm glad that this issue has progressed into a nearly-ready PR, but I still think share the same concerns about Modules that Derick points out in that post (nice link, by the way!). It seems weird to me that some people will be using them to namespace their code, while I'll be building a UI/widget/component library with them. I think these thoughts are what led me to make this recent issue of mine, but I'm still undecided on whether I actually think modules are doing too much or not. Would it make more sense to separate them out as potentially three things: namespace objects, sub-apps, and components? Any thoughts anyone?

@cobbweb
Copy link
Member

cobbweb commented Jan 13, 2014

Yes something isn't right with modules. You can see why I came up with my falsy statement earlier about them being for code management.

Trying to split code management/dependency injection away from sub-apps is definitely something that needs to happen; though I'm not sure what the way forward is :/

Thanks for your commitment to this PR/issue @jmeas, really appreciate your efforts :)

@samccone
Copy link
Member

v2 we can flag modules for a rewrite, since they are pretty crazy right now

@jamesplease
Copy link
Member Author

@samccone Cool. Sounds good.

"Thanks for your commitment to this PR/issue @jmeas, really appreciate your efforts :)"

No problem at all. You and @samccone have put a lot of time and energy into it as well – thanks to you both!

@cobbweb
Copy link
Member

cobbweb commented Jan 17, 2014

Funnily enough, it's come to my attention this afternoon that I actually need this functionality myself. I've got a rip-off of Module as a standalone project shameful plug.

I wanted to make every module have a Views property. So I manually extended the Ocky.prototype in my app to override how it creates submodules. I've added extend functionality directly to the project now :)

@jamesplease
Copy link
Member Author

Original issue fixed by #845. For discussion on further updates to this functionality see #859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants