search

linnovate / meanio

meanio core module
MIT License
54 stars 73 forks source link

Decoupling DB Library from Modules Library #24

Closed fracmak closed 8 years ago

fracmak commented 9 years ago

Libraries should be isolated from each other unless absolutely necessary. The event emitter is a lot better way to communicate between the libraries. This removes Meanio.createModels and Meanio.applyModels functions and instead replaces them with listeners on the existing "modulesEnabled" event and introduces a new "moduleFound" event.

On a side note, is there a reason the lazy models are loaded on modulesEnabled instead of modulesFound? Seems like loading them there would cause fewer problems.

andrija-hers commented 9 years ago

The contribution is based on the following two premises:

  1. Libraries should be isolated from each other unless absolutely necessary.
  2. The event emitter is a lot better way to communicate between the libraries.

I'd appreciate if you could shed more light on this premise.

  1. Why should this be true, and,
  2. if it should be true, how can one measure the level of necessity when the libraries should not be isolated from each other?
  3. In what terms is communication over event emitters better?
  4. Better over what?

On Mon, Nov 24, 2014 at 11:25 PM, Jay Merrifield notifications@github.com wrote:

Libraries should be isolated from each other unless absolutely necessary. The event emitter is a lot better way to communicate between the libraries. This removes Meanio.createModels and Meanio.applyModels functions and instead replaces them with listeners on the existing "modulesEnabled" event and introduces a new "modelFound" event.

On a side note, is there a reason the lazy models are loaded on modulesEnabled instead of modulesFound? Seems like loading them there would

cause fewer problems.

You can merge this Pull Request by running

git pull https://github.com/fracmak/meanio feature/db_isolation

Or view, comment on, or merge it at:

https://github.com/linnovate/meanio/pull/24 Commit Summary

  • Utilize the 'modulesEnabled' event to register the lazy models instead of hard coding a reference between the modules library and the database library
  • Adding myself to Contributing list
  • Moving more code to event emiters and listeners instead of direct dependencies, makes the code a lot simpler

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/linnovate/meanio/pull/24.

andrija-hers commented 9 years ago

After studying the proposed contribution, I have a particular question on the fact that a new event 'modulesFound' event is proposed. Who should/might be the consumers of this event, except for the Meanio instance itself?

On Mon, Nov 24, 2014 at 11:25 PM, Jay Merrifield notifications@github.com wrote:

Libraries should be isolated from each other unless absolutely necessary. The event emitter is a lot better way to communicate between the libraries. This removes Meanio.createModels and Meanio.applyModels functions and instead replaces them with listeners on the existing "modulesEnabled" event and introduces a new "modelFound" event.

On a side note, is there a reason the lazy models are loaded on modulesEnabled instead of modulesFound? Seems like loading them there would

cause fewer problems.

You can merge this Pull Request by running

git pull https://github.com/fracmak/meanio feature/db_isolation

Or view, comment on, or merge it at:

https://github.com/linnovate/meanio/pull/24 Commit Summary

  • Utilize the 'modulesEnabled' event to register the lazy models instead of hard coding a reference between the modules library and the database library
  • Adding myself to Contributing list
  • Moving more code to event emiters and listeners instead of direct dependencies, makes the code a lot simpler

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/linnovate/meanio/pull/24.

fracmak commented 9 years ago

I would argue having each library be isolated from the other libraries is part of the Seperation of Concern design principal. Each library should have a single purpose, and communication through each library should be well defined, documented, and consistent. Right now you have 3 different mechanisms for communicating between libraries. Callbacks, Event Emitters, and Indirect References all facilitated by the Meanio singleton.

Now, why are event emitters beneficial over other forms of communication? I would argue that event emitters allow an unlimited number of listeners, and facilitate unlimited numbers of features added to the system, either by meanio or by other contributors. Check out what I just pushed up. I've created a "moduleFound" event emitter that broadcasts when a module is found. It does not make any assumptions on what a module is supposed to do. It just informs the universe that a module is found. The db library is listening for the moduleFound event, and now contains the utils.walk() code to find the models and initialize the schemas.

Further enhancements can be added to modules easily with this well defined interface and these enhancements are purely optional. It does not require changes to the modules library which is what happened with the database code as it currently sits.

andrija-hers commented 9 years ago

You provided strong and valid arguments. However, it is clear that events are way less efficient than calling methods on (in)direct references to instances. The mean core that we started modifying in the last months (striving for performance) had EventEmitter based code. It is not the event-based programming that I dislike. It is more of the use case of using such an approach (event emitters and event handlers). With event handlers you implicitly spatter references all around by setting event handlers (handlers are the holders of more or less implicit references). With such an approach one has to be careful so that a set event handler can later be removed. However, when it comes to event-based programming, my greatest concern is the very EventEmitter - the nodejs' implementation. I gave up on relying on EventEmitters almost 3 years ago when I encountered the following situation for the first time: (node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit. It is obvious that an implementation of EventEmitter asks for a lot of care. Me and my team have built a more performant implementation that is proprietary and relies on more proprietary code. However, for high performance event-based programming we might come up with a nice public mean-friendly version of the library. Until then we are amidst efforts to actually pull out all the event-based code from the core (more particularly, EventEmitter-based code). Our intention is that the core should not raise events, because there will be no software layers outside the core that would listen to these events. And the very core should be connected much tighter (for more performance) Back to your contribution: your code is clean, neatly packed and logical. Your grasp over javascript coding is shocking. I would ask you to have patience (we're overloaded with projects, but in several days we should be done) until we move the code forward to a "tighter" version, and I'm sure we'll have a lot to discuss and collaborate - to the benefit of the mean.io project. Just as an extra argument in favor of my approach: take a look at who sets event listeners to Meanio events. It is the Meanio itself. For example: Meanio.events.on('modulesFound', Meanio.onModulesFoundAggregate.bind(null, ext, options)); The code in the core is (obviously) redundant in some parts, but it is hard to "straighten out" the "mess" we found and keep the core functioning just the same (from the standpoint of the outer world, that is the Package universe).

fracmak commented 9 years ago

So I understand that event emitters are not as performant as direct function references, but most of this code is on server startup which is usually a one time cost and not usually required to be highly performant. That said, this was merely a suggestion on my part of an alternate way of organize the code and make it easier to understand and more flexible. Feel free to close the pull request if you want to go in a different direction.

ellman commented 9 years ago

Hi @fracmak and @andrija-hers

@liorkesos just asked me to jump in and have a look at the PR. I will try get to it later.

Firstly thanks @fracmak for the PR I see you put a lot of thought and effort into it.

I had a quick read through the thread and I see both points of view. I cant really comment too much now until I have had a better look at things. I must admit though at first glance I like decoupling when possible and I am a big events fan although I have to admin they have to be used in the right place. I agree with fracmak that performance is less important on a one time startup event but then again if is a unique event that will only be used for one purpose eg in meanio to call a function then events are less relevant as typically you would use them when there would be more than one 'listener'

In general @andrija-hers sorry about a lot of the 'mess' in the core, a lot of it my doing. But if not for messy coders like me life would be so boring ;)

This type of thread is incredibly important and is what will really help ensure that the mean core is solid , built well and will be an example of coding excellence some point down the line. Thanks for your efforts and keep it up.

fracmak commented 9 years ago

Another option would be to use the mediator design pattern.

http://en.wikipedia.org/wiki/Mediator_pattern

It's kind of a middle ground between event listeners and direct coupling between the libraries. You already have the mediator in place with the meanio singleton. You just need some delegation code. I could open another pull request done that way as a comparison if you'd like. It would still meet the goals of decoupling the db library

andrija-hers commented 9 years ago

@fracmak , the Mediator pattern is exactly the goal of our work! Mediator is Meanio, it will register core modules giving them a possibility to query interfaces to other core modules. The same (analogous) job it will do on the Package side. @ellman , so it is your code :) A huge thanks for tremendous work put into it! Of course, there is no code that cannot be refactored, the most important thing is to get the thing working. We learnt much more from your code compared to the benefit of the contribution we're making through our refactor. One cannot recognize the "mess" until the code is fully working, and that's what you achieved. We're just studying it in order to recognize points for simplification/optimization. It's always good to strive for the better, so - we'll be working:) Finally, on the event/method call doubt, we're always trying to recognize the "target audience" for a particular software layer. If an event may be handed over to a single recipient, we try to design our software without events. Event-based programming always implies maintenance of handlers packed into some container, which is a performance burden. Moreover, it was more than once that we encountered a situation when the event emitter should shape its behavior based on the behavior of listeners, which is a dirty situation. In other words, once you step into the world of event-based programming, be prepared for confusion that will come, but later :) Whatever one chooses, he needs to be aware that a circular reference situation will occur, and prepare mechanisms for breaking these links when needed (proper handler unregistration in event-based, proper "parent reference" nulling in "reference-based" approach), or memory leaks will destroy your performance. These mechanisms turn out to be the most time-consuming to develop.

liorkesos commented 9 years ago

Fantastic discussion! And valid points all around. I really connect to the "target audience" for a particular software layer. Instinctively I think we should strive for a lean (even if it slightly less abstracted) core. @fracmak suggestion of consolidating the interfaces and the types of communication is a great one - and I like @andrija-hers approach to simplify the core and question the existence of the emitters in the first place. The code has passed several hands with several styles so please question everything!

I understand the inter-core module communication - how do you guys see the package to core relationship now and what would be the added value of considering the emmiter approach in these "system call " like (user/package space to kernel/core space) ?

Simple core, More abstraction in the package layer is my intuition.

fracmak commented 9 years ago

My understanding of the mean library as it currently stands is it's mainly a bootstrap and asset serving/consolidation library. Mean connects to the database, starts express, searches for modules, which register routes/assets/models/(middleware?) and then mostly takes a back seat letting express more or less run the show. Cross package communication skips mean mainly due to the dependable library by directly injecting references from one package into another (which is really cool IOC btw)

I'm having trouble coming up with any events mean would need to broadcast beyond "we've finished launching the server" that anyone would really care except internal message passing. This is one of those examples where we mix and match callbacks and events. Mean.serve() has a callback when it's done, but there's also a "modulesEnabled" event you could listen to as well.

I like the distinction of user/package space vs kernel/core space and I feel like there's a larger discussion that should be had about what features are optional and could be implemented in user space and what features are absolute necessities and should be in kernel/core space. There should be a nicer way of registering functionality with mean in a plugin/addon fashion

But trying to stay on track here, I would propose a reorganization of the existing code around the following core pieces of code:

I would do something like this

mean.serve = function(callback){
    database.connect(function(){
        server.create();
        modules.discover(function(module){
            // callback for each module found
            database.addModule(module);
            server.addModule(module);
            mean.addModule(module); // aggregate assets go here?
        });
        server.done();
        database.done();
        callback();
    });
}

Thoughts?

veljkopopovic commented 9 years ago

@fracmak, you are very close to what we want to propose very soon as a part of a bit bigger discussion: how to redesign mean to be more framework less boilerplate. Most of events should be removed (@andrija-hers had nice race condition bug yesterday), if there is a need for them there have to be a really good reason for that, core packages should be removed from meanio package as well and moved to a standard path (like packages dir, there should be something like core_modules). each core module should implement an interface for communication with the rest of the world, not only extend prototype (our experience showed that prototype extension introduces code confusion: where to hell this method was implemented). Some of client code should be reorganized as well, system package for example, should be divided into two packages: a 'library' package and 'visuals' package (system lib should never be modified by mean users, while everyone should update those visuals, login, register page etc).

liorkesos commented 9 years ago

I like this direction and the fact these notions are being discussed. Maybe we can schedule a core conversation chat on gitter https://gitter.im/linnovate/mean , or we can use hangout.mean.io

andrija-hers commented 9 years ago

That would be nice; I kind of prefer this gitter thingy...

On Friday, November 28, 2014, Lior Kesos notifications@github.com wrote:

I like this direction and the fact these notions are being discussed. Maybe we can schedule a core conversation chat on gitter https://gitter.im/linnovate/mean , or we can use hangout.mean.io

— Reply to this email directly or view it on GitHub https://github.com/linnovate/meanio/pull/24#issuecomment-64861893.

fracmak commented 9 years ago

I'm down for having a conversation, I'm traveling the next couple of days but next week I'm free

liorkesos commented 9 years ago

Cool. What timezone are you in? I'll try to schedule something... Lior

On Sat, Nov 29, 2014 at 10:43 PM, Jay Merrifield notifications@github.com wrote:

I'm down for having a conversation, I'm traveling the next couple of days but next week I'm free

— Reply to this email directly or view it on GitHub https://github.com/linnovate/meanio/pull/24#issuecomment-64964607.

Lior Kesos - http://www.linnovate.net Linnovate - Community Infrastructure Care mail: lior@linnovate.net office: +972 722500881 cell: +972 524305252 skype: liorkesos

fracmak commented 9 years ago

EST

— Jay

On Sat, Nov 29, 2014 at 4:19 PM, Lior Kesos notifications@github.com wrote:

Cool. What timezone are you in? I'll try to schedule something... Lior On Sat, Nov 29, 2014 at 10:43 PM, Jay Merrifield notifications@github.com wrote:

I'm down for having a conversation, I'm traveling the next couple of days but next week I'm free

— Reply to this email directly or view it on GitHub https://github.com/linnovate/meanio/pull/24#issuecomment-64964607.

Lior Kesos - http://www.linnovate.net Linnovate - Community Infrastructure Care mail: lior@linnovate.net office: +972 722500881 cell: +972 524305252

skype: liorkesos

Reply to this email directly or view it on GitHub: https://github.com/linnovate/meanio/pull/24#issuecomment-64965681

liorkesos commented 9 years ago

@fracmak - can you send me your mail to lior@linnovate.net ?

fracmak commented 9 years ago

Done

— Jay

On Sun, Nov 30, 2014 at 3:04 AM, Lior Kesos notifications@github.com wrote:

@fracmak - can you send me your mail to lior@linnovate.net ?

Reply to this email directly or view it on GitHub: https://github.com/linnovate/meanio/pull/24#issuecomment-64978664

clowenhg commented 8 years ago

From what I can see of the current state of the code, the core idea of separating the DB code from Module code has been done, just with slightly different naming and structure.

Given it's been 2 years and the concept looks to have been already implemented in master I'm closing the PR.