Postal 0.8.2 CommonJS package broken

[mikemaccana]

Postal 0.8.2 CommonJS package broken

[mikemaccana]

Looks like this is actually just an error in the docs. The proper line should be:

var defaultChannel = postal().channel();

That said most modules aren't functions so this is a bit unexpected (eg, #27).

[ifandelse]

@mikemaccana ideally when you require postal, you should invoke the factory function it returns:

var postal = require('postal')(); // FYI - this takes an optional underscore/lodash argument require('postal')(_);
var defaultChannel = postal.channel();

That way you're only invoking that factory method once.

I realize this is unexpected compared to most commonjs modules - the decision was made because postal and it's add-on libraries would need to work with the same instances of each lib (including the possibility of the same instance of underscore or lodash, if you'd added your own mixins, etc.). In browser scenarios (using require, etc.) this isn't an issue, but using a factory method was the cleanest way I could come up with addressing this problem in commonjs. I'll check the docs and make sure they get fixed if they're incorrect, and then come back and close this issue. Thanks!

[ifandelse]

I've updated the docs here. Thanks!

[garaboncias]

It's a bad practice, actually it's broken if you want import in more module.
The right solution should be this in postal.js file:

if ( typeof module === "object" && module.exports ) {
        // Node, or CommonJS-Like environments
        module.exports = (function () {
            var _ = require( "underscore" );
            return factory( _ );
        })()
    } 

[ifandelse]

@garaboncias forgive me, but I'm not sure I understand what you mean by saying "it's broken if you want import in more module".

Also - if you're saying that returning a factory function on module.exports is bad practice, I disagree. node.js allows this particular approach with common.js modules for a reason - others have wanted it and found it useful as well. This is the pattern I've used with postal and it's sister libraries (the add-ons, etc.) for a specific reason: they all should share the same instance of any dependencies. AMD loaders (like require.js) typically manage all of this for you in the browser client - that's not the case with common.js modules in node.

If you can explain your concern in more detail, that would be helpful.

[garaboncias]

Sorry for my offensive style.
common.js modules do the same in node: share the exported module instances. If you return the factory method instead of instance, application modules need a global variable to access the same instance. I know no reason why you need to make more instances with factory method.

[dcneiner]

@ifandelse @garaboncias I didn't know it worked like that… I need to do more with node! Just to be sure, I worked up a quick local test and it shares the reference as @garaboncias explained. Here is what I tested: https://gist.github.com/dcneiner/5906901 - Just run node against the parent.js file.

[ifandelse]

@dcneiner @garaboncias I'm researching this issue. First - to address a point @garaboncias made:

"I know no reason why you need to make more instances with factory method."

I'm not creating 'new' instances. The exported factory function should only be run once, & that's the instance which should be used.

Not trying to quibble unnecessarily - but this comes down to following a pattern and determining what kind of control you relinquish to a framework/platform. The pattern in this case is dependency injection. I understand that the vast majority of node modules use in-line require calls - and that's fine - it's the author's choice, and there's nothing wrong with that. Going this route means giving control of managing the instance over to node's require cache. While I was ignorant of how the require.cache worked, the choice to maintain control of the instance management is just that, a choice. That choice was originally made for two reasons:

• guaranteeing the same instance of any postal-related module is used in all circumstances, and
• testability/mockability.

The key concern I have is with the postal add-ons (federation, xframe, etc.) since it's vital that multiple add-ons all get the same instance of postal, and any stacked add-on (postal.xframe, for ex) should get the same instance of postal.federation, etc. Node's docs state:

Modules are cached based on their resolved filename. Since modules may resolve to a different filename based on the location of the calling module (loading from node_modules folders), it is not a guarantee that require('foo') will always return the exact same object, if it would resolve to different files.

Assuming I can get a grasp on what guarantees I have from node on when the same instance will be used, then the only other concern in this case would be the testability/mockability standpoint. If I can resolve that without it being too ugly, then sure, I have no issues turning the control of instance management over to node and ditching the factory function. :-)

I will keep this thread updated once I determine that....

[ifandelse]

@dcneiner @garaboncias So this confirms what I was thinking: Loading from node_modules Folders

I have yet to test it, but this will break down as postal.federation and postal.xframe (for example) include postal and underscore as deps - given that the resolved file path for 'postal' from postal.xframe will not be the same as postal.federation, etc. (same goes for underscore). I'll try and verify this over the weekend.

[dcneiner]

@ifandelse I think that is the case – sorry for the useless test. I think I realized it the day after I posted that here… still so much to learn about node.

[dcneiner]

@ifandelse One more idea, is you could include a factory.js file, or whatever you want to call it, in the npm repo. Then people who wanted the factory, to share underscore instances,etc, would just require('postal/factory') instead of just require('postal') – Or switch it around, and keep the factory as the default, and have the lib available via the other method. Just an idea… if you decide to keep it as is, I am fine with that too.

[garaboncias]

@ifandelse https://github.com/ifandelse you are right, postal.federation
and postal.xframe modules needs factory methods to get the same (singleton)
postal object (if you want avoid the global object, and you want to
explicitly mention postal.js as a dependency in your modules), but If I
were you I would not use in the main postal.js.

This is because it's make more hard to use in an application, where you
definitely use the same cached postal.js node module in your files.
And because if you use more underscore instance, it nobody cares. it's more
important that your 3rd party module use the same library version as they
are tested with.

2013/7/4 Douglas Neiner [email protected]

@ifandelse https://github.com/ifandelse One more idea, is you could
include a factory.js file, or whatever you want to call it, in the npm
repo. Then people who wanted the factory, to share underscore
instances,etc, would just require('postal/factory') instead of just
require('postal') – Or switch it around, and keep the factory as the
default, and have the lib available via the other method. Just an idea… if
you decide to keep it as is, I am fine with that too.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/32#issuecomment-20456886
.

[ifandelse]

@garaboncias I'm not opposed to potentially modifying the postal module wrapper itself to remove the factory function. At the end of the day, though, this debate feels more like the 'bikeshed' argument. Dependency Injection is an established (and well known) pattern. JS devs in node should become aware of it if they aren't familiar with it. Even outside of concerns like postal + plugins, it's quite common to have app level modules that get passed into child module init functions, etc. - I certainly didn't invent this approach. Like I said - I'm open to changing postal's commonjs wrapper (not the plugins), but I consider it low priority at the moment, since it's a breaking API change for node users (thus I may need to wait until 0.9.x before introducing it - we'll see. I'll bump this thread if/when I make the change. Thanks.