Remove app-module-path-node dependency

[martijnwalraven]

We tracked down an issue with the Todos app to the use of app-module-path-node in this package. Because app-module-path-node patches Node internals to modify module resolution, the Cordova code ends up with a faulty version of a package installed in the app's node_modules directory instead of the local one.

Modifying module resolution in this way could potentially break other parts of the build tool too, as it allows apps to override versions of packages we ship in the dev bundle.

Is there any way you can get rid of this dependency? I'm not sure what made you rely on this behavior, but we may be able to help you find a better way to accomplish this without globally modifying Node internals.

[juliancwirko]

This is a remnant of the old version based on meteorhacks:npm If I'll be able to read from /node_modules it shouldn't be a problem to remove it. And I think this is possible without this dependency. I'll try it asap. Thanks for the info.

[martijnwalraven]

Great, thanks for looking into this!

[juliancwirko]

From minify-css.js plugin I try to get to the main node_modules in the app. So I try to do something like: path.resolve(process.cwd(), 'node_modules'); to be able to require PostCSS plugins installed in the app. And in the minify-css.js I want to do: Npm.require(appsNodeModulesPath + '/' + pluginName). It seems to be good path, but it stucked at:

While minifying app stylesheet:
   module.js:338:15: Cannot find module
   '/home/jul/workspace/meteor/postcss-demo/packages/meteor-postcss/.npm/plugin/minifier-postcss/node_modules/home/jul/workspace/meteor/postcss-demo/node_modules/postcss-safe-parser/'

So the path is not resolved as it should here in module.js :/
Also I wonder how will it behave when downloaded from Atmosphere.

Any ideas how to Npm.require from main node_modules in the package's minify-css.js plugin? :)
Thanks.

[martijnwalraven]

@tmeasday @benjamn: Do you know what the best way would be to require PostCSS plugins installed in the app?

[juliancwirko]

I've tried it in many different ways, I can only think of relative path to node_modules but then it will not work when bundled. I thought that it will be simpler :/ It appears that Node can't resolve this in its module.js.

And of course with app-module-path it is possible because it overwrites Module._nodeModulePaths, anyway this whole approach to PostCSS isn't a very good solution. It is rather a hacky way to achieve something which is really useful in the Front-end world, although underestimated ;)

Unfortunatelly for me it's hard to say what we could do with this problem.

[tmeasday]

@juliancwirko can you provide instructions on how to reproduce what you are seeing (maybe push a branch and I'll play with it?)

[juliancwirko]

@tmeasday ok, so I've created a brach with changes. I've removed app-module-path dependency and here I need to resolve path to node_modules in the app, and it's ok. But when minifier runs there is a problem in module.js

Oh, and I require plugins like here and here

[tmeasday]

@benjamn it seems that passing an absolute path into Npm.require or our require() doesn't work. I'm not sure if that's by design or otherwise, but it'd be an easy workaround for this problem if there was a way to do so.

One way would probably be to create an npm package that is simply a requirer:

// in the requirer npm package
module.exports.require = require;

// in the app / build plugin
require('requirer').require(absolutePath);

Why would that work (I think)? Because inside an npm package, require is just npm's default

[joncursi]

I've been scratching my head over non-informative Cordova build errors and through a ton of trial and error I've been able to isolate the errors to this package. Removing this package from my project results in reliable, successful builds on all platforms. For more information on the issues I ran into, please refer to this issue posted on the Meteor issue tracker.

If you need any additional information from my end, I am happy to oblige.

[joncursi]

I can confirm that npm install shelljs@0.5.3 seems to put a bandaid over this problem. Any updated guidance on how to handle this issue?

[tmeasday]

Hmm, this seems to have dropped off the radar. Will surface it again.

[joncursi]

Thanks @tmeasday!

[tmeasday]

See meteor/meteor#7073