-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Extended module
functionality
#845
Conversation
var module = app; | ||
|
||
// get the custom args passed in after the module definition and | ||
// get rid of the module name and definition function | ||
var customArgs = slice(arguments); | ||
customArgs.splice(0, 3); | ||
customArgs.splice(0, 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backwards compatibility is broken with this line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to break compatibility you could try checking if ModuleClass extends the module prototype. This is ugly but it should work:
if (ModuleClass && ModuleClass.prototype instanceof Marionette.Module) {
customArgs.splice(0, 4);
}
else {
customArgs.splice(0, 3);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but it doesn't seem to actually resolve the issue; it only reduces the likelihood of problems. This solution prevents the user from passing a Module as the first argument to their module. Attempts to do this would instantiate the module as that argument instead. I would agree if you were to suggest this to be an improbable situation, but it's a worrisome 'bug' to have floating around nonetheless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea not sure what the best way to do this is, but the way it stands now won't work. Having to pass null
if you want to add custom dependencies without a custom module class isn't good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. That's just another conditional, right? Getting even more hideous, one might try:
if (ModuleClass && (ModuleClass.prototype instanceof Marionette.Module) && !(ModuleClass instanceof Marionette.Module)) {
customArgs.splice(0, 4);
}
else {
customArgs.splice(0, 3);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, taking @cobbweb's observation further, you need to apply this check more broadly, otherwise you'll end up attempting to new
up the 4th parameter - which can be anything - in _getModule
. Perhaps it's more appropriate to leave ModuleClass
out of the signature and define it locally IFF arguments[3]
is a Module
definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ccamarat, I think any attempts to determine whether to slice at 3 or 4 will be fallible in principle; no amount of conditionals can determine if the user is trying to pass an argument or the object to be instantiated. While I wish it was possible, I think @cobbweb's solution to use the object literal definition is the way to go here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
@samccone, I removed the |
Could you please fix up the commit message? :) |
@cobbweb |
One idea to retain backwards compatibility would be to change the second argument of
This prevents one from adding an initializer at the time of creation, but maybe it's worth it for saving backwards compatibility. |
Do we not want a way to specify a default module class in an Application? I was thinking something along the lines of: var app = new Marionette.Application({ moduleClass: CustomModule })
app.module('Foo', function(Foo) {
console.log(Foo instanceof CustomModule); // true
}); |
That's fine, but not something I had been thinking about. If you'd like me to add something like that I can go ahead and do it. Just to clarify, for my purposes what I've included in this PR is necessary; the custom module class needs to be a per-add basis. The way I plan to use this is to make those components I mentioned before. I might, for instance, have a pie graph component and a menu component. The user can install these whenever/however you'd like by adding the appropriate custom module to the application.
The passed-in region tells the application where to put it. The custom module tells the application what to add. But to return to your comment would you like me to add that default module class functionality? |
Module definitions can also be a plain object so you can toggle app.module('Foo', {
moduleClass: FooModule,
define: function(Foo) {
// Foo instanceof FooModule
}
}); Doesn't read or type as well, but is a safer code change. |
This new commit implements your idea @cobbweb, which doesn't break backwards compatibility. It seems like a sound implementation to me, but I'd like to add another parameter to the object literal definition, which is |
|
||
// Overwrite the module class if the user specifies one | ||
if ( moduleDefinition ) { | ||
ModuleClass = moduleDefinition.customModule || ModuleClass; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if customModule
is the right name, I was thinking moduleClass
might be better. ping @samccone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Yep this is looking much better now :) I think we need to do some refactoring around how we handle the module definition being a function or a plain object but we can do this after the fact. @samccone Do we not want a way to specify a default module class in an Application? |
@@ -77,13 +77,21 @@ _.extend(Marionette.Application.prototype, Backbone.Events, { | |||
|
|||
// Create a module, attached to the application | |||
module: function(moduleNames, moduleDefinition){ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary extra line
Could you update the commit message to something along the lines of 'Allow Modules to be extended'? |
I've done my best to remove all of the extra lines you mentioned and updated the commit message. I haven't added the |
Awesome, thanks @jmeas Yea just waiting for more input re: default module class. |
Ok this looks really great. I am still concerned about this breaking something that I am not thinking of. The tests for modules are a bit light, so maybe before we merge this we should backfill a few tests according to https://github.com/marionettejs/backbone.marionette/blob/master/docs/marionette.application.module.md For example, will this still work with this new code? Also, we need to document all of these in the readme, and provide examples. (this can be a commit ontop of all of this) Basically this is close, I just want to make sure this is non breaking. 👍 |
Being concerned about it breaking backwards compatibility is important, and I'm willing to write more tests to show that it won't. But I'll need more examples, I think, of situations where you're worried about it breaking the compatibility. The example you gave seems to me to be covered by 'when configuring a parent module with the object-literal 'startWithParent' to false, adding a child module, and starting the app' in the I'm also fine writing up the documentation once we've settled on the final implementation. @samccone, do you have any thoughts on whether the Also, do either of you have any thoughts regarding the |
I do not think we need a default moduleClass for now, let's try and get a skinny version of this feature out of the door and then we can iterate. and +1 for the initialize param |
Some details about this latest PR
The fifth option there is one that's only slightly questionable to me, but I'm unsure if changing it makes any more sense. Thoughts? |
And I've somehow completely messed up the rebase, including previous commits in my rebase. Yikes. Update: Actually, I'm not even sure if it's messed up. The other commits are still listed separately, and my attempts to undo things isn't being successful. If any has time to look into this it'd be much appreciated; I feel a bit at a loss here. |
git reflog then reset onto your last valid SHA then push it up, I can help from there :) |
Hm, part of the issue is that any SHA I select and |
That may be the best I can do. Is that enough for you? It's still listing the 6 commits. Update: Even if I rebase all the way back to when I cloned the thing, it still lists my commit + the other six commits under my un-pushed commits (via |
I'm no Git Wizard but try this (assuming
In the future it's good practice to make a branch for your PR :) |
I made a first pass at the docs. Let me know what you think. |
MyApp.module("Foo", { | ||
startWithParent: false, | ||
initialize: function( options ) { | ||
// This code is immediately executed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you maybe demonstrate setting a property on this
, and then reference it in in the define
function below? This shows how they're both called with the module as the context.
I accounted for your suggestions, @cobbweb. If there's nothing else I can squash this into 2 commits (docs & code) or one to be merged. |
This PR has been squashed to have just two commits: one for the code, and the other for the README. From my perspective it looks ready to be merged. If you'd rather it be one commit just say the word. |
|
||
``` | ||
MyApp.module("Foo", { | ||
moduleClass: CustomModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an example define
option here too? So readers can see you can specify a custom module class and a definition.
✓ Sinon spies utilized |
Can you rollback and rebase off master instead so there's no merge commit? |
}); | ||
|
||
it("should only run the latest initialize function once, and not the prototype initialize", function(){ | ||
expect(initializer.callCount).toEqual(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(initializer).not.toHaveBeenCalled()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Got rid of the merge, and accounted for this comment |
Can you squash the two doc commits too please? |
You got it. Did you want me to do anything with the |
Yea sorry, can you squash that too please |
Do you want two commits total? Original + docs/sinon, or just one with everything? |
Everything in one sounds good :) |
✓ |
Out of curiosity, what happens if you do something like this? var CustomeModuleOne = Marionette.Module.extend({});
var CustomeModuleTwo = Marionette.Module.extend({});
App.module('TestModule', {
moduleClass: CustomeModuleOne,
define: function() {
this instanceof CustomeModuleOne
}
});
App.module('TestModule', {
moduleClass: CustomeModuleTwo,
define: function() {
this instanceof what // ????
}
}); |
At first the situation you proposed might seem interesting, but I don't think it is, really. The previous behavior of adding modules was to ignore instantiating a new module when it already exists, and nothing has changed with this update. What this means in this situation is that the Take a gander at this line of code to see what I mean. Everything in the Update: It wouldn't be hard to add a unit test showing this, but, again, I don't think it's all that surprising a behavior. |
@jmeas Yea I figured something like that would happen. As long as it doesn't fall over that's all good. |
Custom modules can now be created using the extend method, and then attached to an Application. Documentation and tests were added for this new functionality.
@samccone I adjusted the source according to your suggestions, with a few tweaks that I thought were necessary. Namely, |
Is there anything else I can do for this PR? 😄 |
nope, going to get this version up on a few projects I have for testing, then 🚢 |
Extended `module` functionality
💃 |
👏 Thanks, you two! |
Fixes the botched PR #844, which in turn adds the functionality described in #837.
-Modules now have the functionality of Marionette.extend.
-Upon creation, Modules will fire the
initialize
function, if it exists-You may also pass a custom class to be instantiated as the Module when adding a new Module to the Application.
It breaks backwards compatibility in its current implementation, and has questionable unit tests.