-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Comments
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. |
Great, thanks for looking into this! |
From
So the path is not resolved as it should here in module.js :/ Any ideas how to Npm.require from main node_modules in the package's minify-css.js plugin? :) |
I've tried it in many different ways, I can only think of relative path to And of course with app-module-path it is possible because it overwrites Unfortunatelly for me it's hard to say what we could do with this problem. |
@juliancwirko can you provide instructions on how to reproduce what you are seeing (maybe push a branch and I'll play with it?) |
@benjamn it seems that passing an absolute path into 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, |
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. |
I can confirm that |
Hmm, this seems to have dropped off the radar. Will surface it again. |
Thanks @tmeasday! |
@juliancwirko do you want to try using the new 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 that is awesome, I'll try it asap |
@juliancwirko why do you not have access to an input css file? |
@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. |
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? |
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. |
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. |
I'm not sure what makes the most sense, but FYI if you |
Ok, cool. So I try to rewrite it soon. Thank you :) |
@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 |
any updates on this issue? We're still not able to make a build with Meteor 1.3.5.1 :/ |
|
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 |
@mitar ok, I'll test it and release new major version with your changes, thanks! |
I'll close it. Feel free to comment here if needed. Thanks! |
I think I managed to break this package. Sorry. It worked when I had fork inside
See, search path is from installed module inside home directory, not from inside my package app. So this is why |
I made a fix by moving to use peer dependencies instead of |
Yes, I've checked it only locally too ;) |
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. |
We tracked down an issue with the Todos app to the use of
app-module-path-node
in this package. Becauseapp-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'snode_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.
The text was updated successfully, but these errors were encountered: