New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove app-module-path-node dependency #15

Open
martijnwalraven opened this Issue Apr 29, 2016 · 33 comments

Comments

Projects
None yet
7 participants
@martijnwalraven

martijnwalraven commented Apr 29, 2016

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 comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko Apr 29, 2016

Owner

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.

Owner

juliancwirko commented Apr 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@martijnwalraven

martijnwalraven Apr 29, 2016

Great, thanks for looking into this!

martijnwalraven commented Apr 29, 2016

Great, thanks for looking into this!

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko Apr 29, 2016

Owner

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.

Owner

juliancwirko commented Apr 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@martijnwalraven

martijnwalraven Apr 29, 2016

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

martijnwalraven commented Apr 29, 2016

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

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko Apr 30, 2016

Owner

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.

Owner

juliancwirko commented Apr 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 2, 2016

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

tmeasday commented May 2, 2016

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

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 2, 2016

Owner

@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

Owner

juliancwirko commented May 2, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 2, 2016

@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

tmeasday commented May 2, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@joncursi

joncursi May 14, 2016

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 commented May 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@joncursi

joncursi May 14, 2016

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?

joncursi commented May 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 17, 2016

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

tmeasday commented May 17, 2016

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

@joncursi

This comment has been minimized.

Show comment
Hide comment
@joncursi

joncursi commented May 17, 2016

Thanks @tmeasday!

@tmeasday

This comment has been minimized.

Show comment
Hide comment

tmeasday commented May 17, 2016

See meteor/meteor#7073

@tmeasday

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 21, 2016

@juliancwirko do you want to try using the new inputFile.require(id) API in the 1.3.3 beta: meteor/meteor#7073 (comment) ?

Also, I think that there's a uglier way to do this that would work in older 1.3 versions too, is that correct @benjamn?

tmeasday commented May 21, 2016

@juliancwirko do you want to try using the new inputFile.require(id) API in the 1.3.3 beta: meteor/meteor#7073 (comment) ?

Also, I think that there's a uglier way to do this that would work in older 1.3 versions too, is that correct @benjamn?

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 21, 2016

Owner

@tmeasday that is awesome, I'll try it asap

Owner

juliancwirko commented May 21, 2016

@tmeasday that is awesome, I'll try it asap

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 21, 2016

Owner

@tmeasday @benjamn I don't know if I understand it corectly but I would like to import some modules installed in the app without access to input css file. Is there documentation or usage example somewhere?

Owner

juliancwirko commented May 21, 2016

@tmeasday @benjamn I don't know if I understand it corectly but I would like to import some modules installed in the app without access to input css file. Is there documentation or usage example somewhere?

@tmeasday

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 21, 2016

@juliancwirko why do you not have access to an input css file?

tmeasday commented May 21, 2016

@juliancwirko why do you not have access to an input css file?

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 21, 2016

Owner

@tmeasday I probably misunderstood this. I'll try to explain. I'll use your reproduction repo.

So, You add aws-sdk here so it should be installed on the application level. Then you want to import it in the plugin (in the package) here. Here at the beginning of the file you aren't in the context of the input file. Am I right?

Could you rewrite your reproduction repo with these changes? Or maybe you will be able to explain it a a little bit? It would be very appreciated. Many thanks.

Owner

juliancwirko commented May 21, 2016

@tmeasday I probably misunderstood this. I'll try to explain. I'll use your reproduction repo.

So, You add aws-sdk here so it should be installed on the application level. Then you want to import it in the plugin (in the package) here. Here at the beginning of the file you aren't in the context of the input file. Am I right?

Could you rewrite your reproduction repo with these changes? Or maybe you will be able to explain it a a little bit? It would be very appreciated. Many thanks.

@tmeasday

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 21, 2016

I mean, sure it doesn't directly solve the problem as stated, but a build plugin doesn't need to import until it has some files to operate on, right?

tmeasday commented May 21, 2016

I mean, sure it doesn't directly solve the problem as stated, but a build plugin doesn't need to import until it has some files to operate on, right?

@tmeasday

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 21, 2016

The reason it works like this is because a build plugin can also be set to operate on a package, so you can't just import the one time blindly.

tmeasday commented May 21, 2016

The reason it works like this is because a build plugin can also be set to operate on a package, so you can't just import the one time blindly.

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 21, 2016

Owner

Ok, I think I understand it now. So I should require all plugins in the loop with every single css file. I probably could rewrite it that way. I wonder if it will be fast enought. Let's supose we have 15 PostCSS plugins and 30 css files. It gives 450 require calls each app rebuild ;) But maybe it is still performant. I am not sure.

Owner

juliancwirko commented May 21, 2016

Ok, I think I understand it now. So I should require all plugins in the loop with every single css file. I probably could rewrite it that way. I wonder if it will be fast enought. Let's supose we have 15 PostCSS plugins and 30 css files. It gives 450 require calls each app rebuild ;) But maybe it is still performant. I am not sure.

@tmeasday

This comment has been minimized.

Show comment
Hide comment
@tmeasday

tmeasday May 21, 2016

I'm not sure what makes the most sense, but FYI if you require something with the exact same path, it's a no-op.

tmeasday commented May 21, 2016

I'm not sure what makes the most sense, but FYI if you require something with the exact same path, it's a no-op.

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 21, 2016

Owner

Ok, cool. So I try to rewrite it soon. Thank you :)

Owner

juliancwirko commented May 21, 2016

Ok, cool. So I try to rewrite it soon. Thank you :)

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 22, 2016

Owner

@tmeasday one more problem ;) I think that this is implemented only in compiler-plugin.js and not in the minifier-plugin.js

Unfortunatelly I need it in the minifier-plugin.js

Owner

juliancwirko commented May 22, 2016

@tmeasday one more problem ;) I think that this is implemented only in compiler-plugin.js and not in the minifier-plugin.js

Unfortunatelly I need it in the minifier-plugin.js

@v3rron

This comment has been minimized.

Show comment
Hide comment
@v3rron

v3rron Jul 30, 2016

any updates on this issue? We're still not able to make a build with Meteor 1.3.5.1 :/

v3rron commented Jul 30, 2016

any updates on this issue? We're still not able to make a build with Meteor 1.3.5.1 :/

@emilbryggare

This comment has been minimized.

Show comment
Hide comment
@emilbryggare

emilbryggare Aug 12, 2016

I can confirm that npm install shelljs@0.5.3 seems to put a bandaid over this problem.
@v3rron Try installing shelljs@0.5.3. This allowed me to build again.

emilbryggare commented Aug 12, 2016

I can confirm that npm install shelljs@0.5.3 seems to put a bandaid over this problem.
@v3rron Try installing shelljs@0.5.3. This allowed me to build again.

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar May 4, 2018

Contributor

This is addressed by #32, because it uses another regular NPM package to do importing. This in fact is then just a fancy requirer @tmeasday was talking about.

The issue is that I am observing some random failures to find a node module. Restarting Meteor helps. But it is just unclear why it happens that this package then cannot find it. I do not modify node_modules at all. It is just random.

Contributor

mitar commented May 4, 2018

This is addressed by #32, because it uses another regular NPM package to do importing. This in fact is then just a fancy requirer @tmeasday was talking about.

The issue is that I am observing some random failures to find a node module. Restarting Meteor helps. But it is just unclear why it happens that this package then cannot find it. I do not modify node_modules at all. It is just random.

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 4, 2018

Owner

@mitar ok, I'll test it and release new major version with your changes, thanks!

Owner

juliancwirko commented May 4, 2018

@mitar ok, I'll test it and release new major version with your changes, thanks!

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 4, 2018

Owner

I'll close it. Feel free to comment here if needed. Thanks!

Owner

juliancwirko commented May 4, 2018

I'll close it. Feel free to comment here if needed. Thanks!

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar May 5, 2018

Contributor

I think I managed to break this package. Sorry. It worked when I had fork inside packages, but now that it is published, it does not really work. The issue is that @tmeasday's idea does not really work. I made the NPM package print module.paths before require call and you will see why it does not:

[ '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/minifier-postcss/node_modules/postcss-load-plugins/lib/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/minifier-postcss/node_modules/postcss-load-plugins/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/minifier-postcss/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/node_modules',
  '/home/mitar/.meteor/packages/node_modules',
  '/home/mitar/.meteor/node_modules',
  '/home/mitar/node_modules',
  '/home/node_modules',
  '/node_modules' ]

See, search path is from installed module inside home directory, not from inside my package app. So this is why require does not find packages inside node_modules inside the app.

Contributor

mitar commented May 5, 2018

I think I managed to break this package. Sorry. It worked when I had fork inside packages, but now that it is published, it does not really work. The issue is that @tmeasday's idea does not really work. I made the NPM package print module.paths before require call and you will see why it does not:

[ '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/minifier-postcss/node_modules/postcss-load-plugins/lib/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/minifier-postcss/node_modules/postcss-load-plugins/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/minifier-postcss/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules/meteor/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/npm/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/plugin.minifier-postcss.os/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/.2.0.1.1h3wuhm.m6do++os+web.browser+web.cordova/node_modules',
  '/home/mitar/.meteor/packages/juliancwirko_postcss/node_modules',
  '/home/mitar/.meteor/packages/node_modules',
  '/home/mitar/.meteor/node_modules',
  '/home/mitar/node_modules',
  '/home/node_modules',
  '/node_modules' ]

See, search path is from installed module inside home directory, not from inside my package app. So this is why require does not find packages inside node_modules inside the app.

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar May 5, 2018

Contributor

I made a fix by moving to use peer dependencies instead of Npm.depends: #34

Contributor

mitar commented May 5, 2018

I made a fix by moving to use peer dependencies instead of Npm.depends: #34

@juliancwirko

This comment has been minimized.

Show comment
Hide comment
@juliancwirko

juliancwirko May 5, 2018

Owner

Yes, I've checked it only locally too ;)
Ok, I'll merge that. Thanks.

Owner

juliancwirko commented May 5, 2018

Yes, I've checked it only locally too ;)
Ok, I'll merge that. Thanks.

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar May 6, 2018

Contributor

After more battling with this, it seems this is not possible at the moment because of the limitation in Meteor. I opened an issue about this: meteor/meteor#9865

My proposal is that we revert for now to using app-module-path-node and reopen this issue and wait for better times when this will be fixed in Meteor.

Contributor

mitar commented May 6, 2018

After more battling with this, it seems this is not possible at the moment because of the limitation in Meteor. I opened an issue about this: meteor/meteor#9865

My proposal is that we revert for now to using app-module-path-node and reopen this issue and wait for better times when this will be fixed in Meteor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment