jaredhanson / electrolyte

Elegant dependency injection for Node.js.
MIT License
564 stars 61 forks source link

version 0.3.0 returns objects while 0.4.0 returns promises #58

Open nilaysaha opened 7 years ago

nilaysaha commented 7 years ago

Can someone explain why the new version of electrolyte breaks the way of working. On doing ioc.create(<component>)it is returning promise in version 0.4.0 while in 0.3.0 it returns the object exported via the component. Nested promises as well....

Any pointers on how to use the new version of electrolyte and reconcile old code with new version of electrolyte?

rohdef commented 7 years ago

Isn't this choice basically breaking a quite important use case of the electrolyte? Say I'm coding a library, and I want DI internally in it. A logical choice here is to have an index.js to expose all of my things, doing something along the lines of:

"use strict";

const IoC = require("electrolyte");
IoC.use(IoC.dir("."));

module.exports = exports = {
    myLib1: IoC.create("myLib1"),
    myLib2: IoC.create("myLib2");
}

now my client doing:

const awesomeLib = require("./index.js");

would have to know that awesomeLib.myLib1 is a promise. Also I did consider (although I did not try):

IoC.create("myLib1").then((lib) => exports.myLib1 = lib);
IoC.create("myLib2").then((lib) => exports.myLib2 = lib);

but I'm fairly certain that won't work, due to JavaScript being single threaded, so the promises will not have been fulfilled when the client uses it.

nilaysaha commented 7 years ago

Precisely ...that is what is happening I see....Question is then should this version 0.4.0 be considered an usable version or not. It really should be backwards compatible....Any pointers why this was done is desirable....

rohdef commented 7 years ago

A slightly related issue: Failure to uphold semver expectations

From a semver point of view (which I might is a pretty good point of view), it is definitely broken.

For my needs - which is precisely a library like the one above - it is definitely broken, unless there is a way to do it that I am not aware of. However, like @nilaysaha I am also curious to why this is desireable? Isn't this the exact point of having both the async and non-async version? I generally think async is desired when possible, so if there is a good way that I know not, I'd be happy to learn it :)

nilaysaha commented 7 years ago

What I feel is that exports['@async'] = true; has been made as default behaviour. And this is the reason why the latest 0.4.0 release is broken as far as non-async export is concerned. It should be left to the user to decide whether he wants to export async functions /objects or not ....

And exports['@async'] = false; does not help either...

@jaredhanson : Could you help us out...? So basically the whole module is now broken...or are we getting something wrong here.. .? Thanks in advance...

jaredhanson commented 7 years ago

I left my comments related to this on the PR here: https://github.com/jaredhanson/electrolyte/pull/46#issuecomment-250999409

I think the ability to create and inject dependencies in an async fashion has a lot of benefits. But, it needs to be an all-or-nothing approach. If some injections are async and others are not, how does the caller creating/injecting the dependency know ahead of time?

Since the async abilities offer benefits, I think its worth adopting that as the default and removing the ambiguity. I'm certainly open to discussing this further and finding out if there are better alternatives.

nilaysaha commented 7 years ago

@jaredhanson : I know the problem that you are trying to solve using the asyncCreate namely that the action of the module which is being imported needs to be completed (and hence the promise part). But why don't we leave it upto the module to return a promise and not let this be handled by electrolyte. Already there is a large user base who have built their whole software around the 0.3.0 version. Most importantly there is no clear explanation of transition between old and new way of working.

The most common structure which is used for electrolyte for large code base( I have seen in commercial use) is like this:

exports = module.exports = function(path,shortid,beaconModel){ } exports['@singleton'] = true; exports['@require'] = [ 'path','shortid','models/beaconRouter' ];

And this kind of structure allows nice merger between require and importing other modules. Now what happens in 0.4.0 is that we have nested promises, which are not easily resolved on the client side. So a lot of extra work needs to be done to get it working.

In the end we have landed up with complications more than a solution introduced by the framework. Correct me if I am wrong.

wingedfox commented 7 years ago

Hi,

I've done my part of migration to Electrolyte 0.4.0 and need to say that it offers much more than asks in return.

The only major issue I need to mention is I have to do some extra coding to integrate with 3rd-party tools that ask for inline/synchronous callback provisioning, i.e. swagger-tools, where I have to use something like

const ioc = require('electrolyte');

module.exports = {
    get: (req, res, next) => {
        ioc.create('controllers/service/get')
            .then(ctrl => ctrl(req, res, next))
            .catch(console.log);
    }
};

But on other hand my services better handle startup errors to prevent broken start if something went wrong

const ioc = require('electrolyte');
ioc.use(ioc.node_modules());
ioc.use(ioc.dir('src'));

ioc.create('lib/log/log').then($log => {
    return Promise.all([
        ioc.create('controller/queue/listen'),
        ioc.create('controller/http/serve'),
        ioc.create('controller/job/do')),
    ])
    .then(() => $log.info("started"))
    .catch((err) => {$log.error("failed to start", { error: JSON.stringify(err) }); process.exit(100)})
}).catch((err) => { console.log(err); process.exit(100)});
nilaysaha commented 7 years ago

@wingedfox :thanks for your comments. So if I understand you correctly, for all components which were returning objects and now returns promises you resolved them right at the beginning using Promise.all ...right ? And then in other parts of the application where they were used it continued to be used as earlier ...right ?

wingedfox commented 7 years ago

@nilaysaha yep, you're right in both statements.

ashleydavis commented 7 years ago

@jaredhanson I really like this but I can't use it with promises as the default. I'm going to stick with version 3 for the moment and hope that you change it back in the future.

norbertkeri commented 6 years ago

The documentation in the README is very misleading, it still has the 3.x way of working, it still describes 3.x