Module instance lacks "paths" property

[apendua]

Usually, a NodeJS module will have a paths property. Unfortunately, the current implementation of Module class does not have it, and since some npm packages may assume its existence, this results in runtime errors, e.g.

https://github.com/mochajs/mocha/blob/master/lib/mocha.js#L28

In this case import 'mocha' results in:

TypeError: Cannot read property 'push' of undefined
     at meteorInstall.node_modules.mocha.lib.mocha.js (node_modules/mocha/lib/mocha.js:29:1)
     at fileEvaluate (packages/modules-runtime/.npm/package/node_modules/install/install.js:153:1)

[benjamn]

Is it sufficient if the .paths property exists but doesn't do anything? Or is Mocha relying on something happening when it calls module.paths.push?

[benjamn]

For what it's worth, the CommonJS spec seems to say .paths is optional (cf. 1.6.1-6), so this may be a case of Mocha being overdependent on the particular CommonJS implementation used in Node.

[apendua]

@benjamn You're probably right that mocha should not expect that property to exist in the first place. Do you think that this issue fits better in their repository?

[boneskull]

Hi. module.paths is not part of the CJS standard. module.paths (and alternatives like the NODE_PATH envrionment variable) are undocumented and not "officially" supported. These are both in wide use, and are unlikely to be removed from Node.js core.

However, maybe a greater issue here is that Node.js modules are written with the expectation of running in Node.js and its CJS implementation.

Mocha is not "overdependent"--nor are any modules written for Node.js--on Node.js' implementation. I have yet to see a Node.js module that specifically advertises Meteor support or runs integration tests against Meteor in its CI. Mocha is in this bucket; I was surprised to learn recently that it had any compatibility with Meteor at all.

I don't know what Meteor's CJS implementation looks like--or if it has its own non-standard features--but If Meteor wishes to advertise full support for Node.js modules, its CJS implementation should be a superset of Node.js', at minimum.

In this case, Mocha should be able to address this particular issue, but please understand Meteor and its users shouldn't expect this of other projects. :smile:

[apendua]

Shortly after reading @boneskull answer I was not sure why he is not so positive about fixing this problem in mocha, but I think I am starting to understand it now.

@benjamn The problem - as I see it - is mocha is a library that's intended to be run in node environment and apparently has no interest in staying compatible with every possible CommonJS environment out there, which is sad but understandable. On the other hand, the very reason of install - as far as I understand it - is to port as many node packages to meteor environment. With that goal in mind, I think it's reasonable to have things like module.paths even if it's not directly expressed by the CommonJS standard. At least it's not forbidden.

@benjamn I am happy to provide a PR if you agree with my thinking. If not I will try to help @boneskull fixing the issue on mocha's side but to be honest I'd rather do it here. Thanks!

[boneskull]

Indeed, adding module.paths would increase Meteor's compatibility with Node modules. Don't know how difficult this would be to implement, or if there are other concerns about it.

[boneskull]

FWIW, if a project like Mocha has the necessary resources, ensuring compatibility with other CommonJS implementations could be on the table. Mocha supports Node.js 0.10.x and greater across Windows and POSIX OSes, in addition to IE7 and newer, PhantomJS 1.9.7 and newer, and all other major desktop browsers. It's also likely working in other environments that I don't know about.

That's a full plate for any project. If anyone is interested in adding--and maintaining--compatibility with e.g. Meteor, I'd appreciate the help, but don't have the time nor expertise to do so otherwise.

[apendua]

@benjamn Would you mind sharing your opinion with us?

@boneskull As far as I know the only blocker that prevents mocha being compatible with Meteor's CommonJS environment is this issue here.

[boneskull]

@apendua That's my assessment as well. But it's not fixed yet, and we aren't guaranteed future compatibility without adding and maintaining Meteor in our build matrix.

[benjamn]

Indeed, adding module.paths would increase Meteor's compatibility with Node modules. Don't know how difficult this would be to implement, or if there are other concerns about it.

My concern is that the install library doesn't read any files from disk when loading modules, because the goal is to implement a bundle format that works outside of Node (e.g. in browsers), so a meaningful module.paths property doesn't really make sense.

That leaves two options:

1. Add a module.paths property that doesn't do anything.
2. Let code that tries to modify modules.paths deal with the possibility that it might not be defined.

The second option is effectively what's happening now, so I guess I'm wondering if the first option is worthwhile? Will mocha work if what it adds to module.paths is ignored?