mariocasciaro / scatter

IoC container and out-of-the-box extensibility for Node.js applications
MIT License
154 stars 14 forks source link

Generate `__module` implicitly? #20

Closed bebraw closed 10 years ago

bebraw commented 10 years ago

Hi,

Do you think it would be reasonable to generate that __module declaration implicitly based on args?

Ie. instead of this:

module.exports = function(hello) {
    return {
        saySomething: function() {
            hello();
        }
    }
};

module.exports.__module = {
    args: ["hello"]
};

You would use this:

module.exports = function(hello) {
    return {
        saySomething: function() {
            hello();
        }
    }
};

It is entirely possible I'm missing something obvious here and that there is a valid reason for the current solution. It just feels like this would be a nice shortcut for small scale development.

Of course this wouldn't work in all cases (ie. relative paths) and there may be other gotchas I'm not aware of (introspecting those params can be a little tricky).

mariocasciaro commented 10 years ago

For the sake of curiosity, one early versions of Scatter was using exactly this method for injecting dependencies, but when I introduced namespaces a naming problem arose. How can someone inject a dependency like namespace/sub/module using just argument names? That's why I preferred to keep things linear and just allow explicit dependency declaration.

Possible alternatives were to use a special character as namespace separator, as for example namespace_sub_module or namespace$sub$module, but both _ and $ are valid character for a file name (and hence a module name), so it requires a slightly more complicated algorithm for naming and resolving modules both to implement and to document.

I'm open to proposal, if they can solve elegantly the namespace problem.

On Thu, Jan 9, 2014 at 11:34 AM, Juho Vepsäläinen notifications@github.comwrote:

Hi,

Do you think it would be reasonable to generate that __module declaration implicitly based on args?

Ie. instead of this:

module.exports = function(hello) { return { saySomething: function() { hello(); } }}; module.exports.__module = { args: ["hello"]};

You would use this:

module.exports = function(hello) { return { saySomething: function() { hello(); } }};

It is entirely possible I'm missing something obvious here and that there is a valid reason for the current solution. It just feels like this would be a nice shortcut for small scale development.

Of course this wouldn't work in all cases (ie. relative paths) and there may be other gotchas I'm not aware of (introspecting those params can be a little tricky).

— Reply to this email directly or view it on GitHubhttps://github.com/mariocasciaro/scatter/issues/20 .

bebraw commented 10 years ago

Possible alternatives were to use a special character as namespace separator, as for example namespace_sub_module or namespace$sub$module, but both _ and $ are valid character for a file name (and hence a module name), so it requires a slightly more complicated algorithm for naming and resolving modules both to implement and to document.

Sticking to underscores could be a good choice.

For me it really comes down to having control over which deps to inject and when. From module point of view this means there are some entry points (those module.exports parameters) that I can inject dependencies into (whole point as you know :) ).

Let's say I'm trying to inject configuration depending on where I use the module (ie. production vs. testing). In this case I would like to do something like this (imaginary syntax):

main.js

...

// 1. choose dependencies
scatter.choose('./config/production');

// 2. load api using the dependencies chosen
scatter.load('./api').then(function(api) {
    // just do something here now
    api.countries('Great Britain', 'France').then(...);
});

That ./config/production could be just module.exports = {}. I'm not sure if anything else is really needed. Of course it gets more complex if these modules need to consume dependencies but that goes beyond this conversation at least for now.

For me it seems intuitive it should go this way. First you define your configuration (that choose) and then load the module you want to use. In case some dependency doesn't get resolved, you should obviously get an error.

I think in ideal case we should be able to mix regular and enhanced modules side by side. As far as I understand that should be more or less possible already.

I hope I'm making sense here. I haven't used Node DI solutions a lot yet. :)

mariocasciaro commented 10 years ago

So your problem is not related to the __module but rather to the specific issue of loading different components based on your config. Well I've got good news for you, that's exactly one of the point of using Scatter, and it is already possible today do what you are describing here. Your

// 1. choose dependencies

scatter.choose('./config/production');

actually correspond to adding a particle instead of another during the creation of Scatter, something like this:

var scatter = new Scatter();
scatter.registerParticles(['./commonParticle']);
if(process.env.NODE_ENV === 'production') {
  scatter.registerParticles(['./productionParticle']);
} else {
  scatter.registerParticles(['./devParticle']);
}

Don't forget to make the devParticle and productionParticle override the commonParticle (cods here https://github.com/mariocasciaro/scatter/wiki/API-Documentation#particlejson )

If you look at the basic Scatter example you can have a better idea https://github.com/mariocasciaro/scatter/blob/master/examples/helloworld

bebraw commented 10 years ago

actually correspond to adding a particle instead of another during the creation of Scatter, something like this: ...

Ok, makes sense. Is there some particular reason why you are using __dirname at your example? Just wondering whether it would be possible to eliminate.

Just to summarize some of my earlier thoughts... It could be a good goal to aim for a compact, minimal set of features that's fast to learn (minimal syntax). In addition what we discussed earlier, you could perhaps eliminate new Scatter quite easily. Of course this is a matter of taste to some extent.

mariocasciaro commented 10 years ago

I use __dirname just because I prefer to have absolute directories, so ideally I can run my app with different cwd. But this is just a detail, it should work with relative dirs too.

Can you please be more specific when you say "compact, minimal set of features that's fast to learn"? It is a little bit too broad and it doesn't help me to understand what needs to be improved.

new Scatter allows you to create multiple containers for the same app (it's undocumented for now because not perfect, but it is already more than useful in unit testing).

I understand I'm not doing a good job at documenting Scatter or creating tutorial and examples to make it easier to understand. In this regard, any help with code and/or documentation is more than appreciated. Code and wiki is open to everyone to improve.

bebraw commented 10 years ago

I use __dirname just because I prefer to have absolute directories, so ideally I can run my app with different cwd. But this is just a detail, it should work with relative dirs too.

Ok, great.

Can you please be more specific when you say "compact, minimal set of features that's fast to learn"? It is a little bit too broad and it doesn't help me to understand what needs to be improved.

Sure. Maybe it makes sense to split the functionality conceptually in "basic" and "advanced". You should be able to achieve most tasks with basic whereas advanced deals with more special cases. Maybe an approach like this is useful on documentation level?

In some cases advanced functionality is pushed into plugins. I noticed you have done this to some extent already. Perhaps the approach can be pushed further? Then you could have documentation per plugin in the right place and so on. Generally the Node way seems to be to favor a lot of small modules over monolithic ones.

new Scatter allows you to create multiple containers for the same app (it's undocumented for now because not perfect, but it is already more than useful in unit testing).

Ok. It would be very nice to have an example available that illustrates the usage and benefits. :)

I understand I'm not doing a good job at documenting Scatter or creating tutorial and examples to make it easier to understand. In this regard, any help with code and/or documentation is more than appreciated. Code and wiki is open to everyone to improve.

I'll keep an eye on the project. I just came up with a little injector of my own. If/when I get fed up with that, I know where to look. :)

mariocasciaro commented 10 years ago

The idea of splitting the docs in basic + advanced definitely makes sense, as I said I already know that Scatter is lacking a good documentation. Beside that I created #21 to take the plugins out of the core and create their own npms, as you said this might help the user to bootstrap with Scatter and then gradually use more advanced features.

For examples about creation of multiple containers, please take a look at the Scatter unit tests.

I'm closing this issue, please feel free to reopen if you think there are still matters to discuss on this topic.

bebraw commented 10 years ago

Beside that I created #21 to take the plugins out of the core and create their own npms, as you said this might help the user to bootstrap with Scatter and then gradually use more advanced features.

Ok. You might want to consider creating an organization as a namespace for the project. It is free and makes it easier to manage multiple repositories. You can also have a site below the organization. Looks like "scatterjs" is available so that might be ideal.

For examples about creation of multiple containers, please take a look at the Scatter unit tests. I'm closing this issue, please feel free to reopen if you think there are still matters to discuss on this topic.

Sure. Thanks for the discussion. It helped to clear out some conceptual problems of mine. I'll link you to my little tool once it's out. It's tiny but does what I need for now. :)

bebraw commented 10 years ago

@mariocasciaro The first version of my tool, annoinject, is available now. It's obviously missing bells and whistles but it covers basic needs of mine. Just thought to link in case you want to take a look. :)

mariocasciaro commented 10 years ago

I was definitely curious to see it. I'm seeing you took a totally different and relatively simple approach than the one used in Scatter, interesting.