-
Notifications
You must be signed in to change notification settings - Fork 118
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
Depedencies: node-pre-gyp drags in a whole lot of new of dependencies #93
Comments
It isn't a devDependency. It's necessary for downloading and installing the pre-built binary instead of compiling locally - which is way faster and should resolve the issues many people have had with a messed up local compile chain.
What is the impact? |
I was always compiling fsevents from source and never had issues. So you say because other users have issues setting up their environment to build fsevents, I have to depend on 30+ new modules? This is bad, I would prefer if fsevents was rather shipping the prebuilt binaries for each platform if I had to choose. The impact is as I said, 30+ new dependencies. Depending on where fsevents is being used this can have a number of issues (overall product size, LCA approvals for open source node modules, etc). |
I hear you registering your disapproval of this change, and I'm sorry you're unhappy with it. But I do believe it is a net benefit to chokidar's userbase. If complex/deep dep trees give you pause, the npm registry must be a pretty perilous place for you. It would be nice if this could be configurable so you could opt out, but I don't see a clean way to provide that with npm. You can lock to an earlier version obviously, but you knew that. A manipulated shrinkwrap file maybe, but it will probably lead to maintenance headaches eventually. I suppose we could consider a PR that converts node-pre-gyp to a devDependency and adds scripting or some alternate means to handle installation of the pre-built binaries. Is that something you'd be interested in working on? Any other suggestions? (aside from just reverting) @indieisaconcept do you have any perspective to share on this by any chance? |
@es128 I am trying to understand the code dependency to node-pre-gyp in fsevents.js, I assume that one could be replaced with something else or does it need all of node-pre-gyp to be there? If the code dependency could go away, then I think it would be nicer to give a user the choice of node-gyp vs. node-pre-gyp. E.g. if the normal install fails, provide a script that installs node-pre-gyp and runs the install again. I see the value of granular dependencies, even if there are many for one module but I cannot really understand a dependency to 30+ modules that are only being used during npm install, and not as a code dependency. |
It's not a code dependency, it's an install dependency. See https://github.com/strongloop/fsevents/blob/master/package.json#L17 What if you were to add some scripting to your project (
Defeats the gains we've achieved in installation time using this method. |
Ah, good point. The only thing it's doing there is finding the installed binary, which would probably be possible to shim with something simpler, but obviously this idea is getting more complicated. It all depends on how much motivation there is to trim down the dependencies. |
I'll review later today On Tue, Oct 6, 2015 at 9:42 PM, Elan Shanker notifications@github.com
|
@es128 Some projects use node-pre-gyp as optionalDependency. Maybe that's the way? |
Or maybe a peerDependency, and allow user to install it by himself if he wants it. |
Example? I don't see how to do that without defeating some major benefits and/or not having any positive impact on the dep tree concerns. |
node-pry-gyp is not working for me again for electron, and I don't want to use it at all. I want source builds.. |
Is there any open source electron app using chokidar/fsevents someone could point me to? |
Also, can you describe the failure better? Breakage is different front hand-wringing about too many deps and maybe should be discussed in a separate issue. |
Popular OS projects using chokidar are listed in their README :) For me it's notably webpack and babel (but also less popular xpc-connection). Specifically it happens when using The error shows as follows:
(mapbox/node-pre-gyp#175 somewhat solves it, when using As a consumer of such open source modules I'd like to choose whether to compile them with node-pre-gyp and node-gyp (with fallback), but I don't feel like ping each and every project that uses them when something breaks.. So I guess ideally fsevents wouldn't depend on either of them, but only require either of them. This can be checked directly in install step. No need to list it in either |
@sheerun I asked if there were any projects I could look at using electron and chokidar. |
Personally I know only non-public projects, but I see that https://github.com/kitematic/kitematic uses electron and babel (that depends on chokidar). I bet half of the projects from https://github.com/sindresorhus/awesome-electron use either webpack, babel, or browserify. |
If that's the case then why aren't a lot more people having the problems you've described? |
@sheerun see if fsevents 1.0.3 improves the situation for you |
@bpasero I attempted a fix for your concern in the [fix-93 branch](https://github.com/strongloop/fsevents/tree/fix-93 branch), but it has a fatal flaw in that removing node-pre-gyp breaks But perhaps just adding pre-gyp-find and eliminating the node-pre-gyp runtime dependency is good enough, allowing you to add your own Please take a look and let me know what you think. |
It would be great to see this dependency removed. I also get the warning |
I use chokidar (1.2.0) which uses fsevents (1.0.5) in an atom editor package. After upgrading to Atom 1.2.0 I get this error message when trying to build the package:
I assume that this is related to this issue? |
@es128 tried out the branch and it seems to work nicely for me! The only warning I get during npm install is:
|
@bpasero the branch I had made would create unacceptable side effects, but if you can confirm that you'd be able to add your own |
I can give that a try, are the side effects going to impact us as well or are we ok once we have built chokidar? |
My proposal creates no ill side effects other than adding one more small dependency. The suggestion I'm making is that you take that and uninstall |
Right, we do not run npm rebuild ever as far as I know. |
I'm having the same issue as @stdavis with my Atom package https://github.com/ensime/ensime-atom. I've seen the error before and asked on atom channel first: https://discuss.atom.io/t/apm-using-ancient-versions-of-node-npm-and-i-cant-seem-to-fix-it/26400 Stuff did seem to work anyway even though
Running
Just like @stdavis above. To reproduce using Atom, you just need:
which fails as I reported here atom/apm#531 I think the problem stems from the fact that apm bundles ancient node/npm as described here: I'm super-stuck and don't know what I could do to release a new version of my package. I guess I should look into what @stdavis did to fix his problem since were in the same space… Any tips highly appreciated! |
Oh, @stdavis removed chokidar agrc/atom-amdbutler@288f01c |
Yep. Removing chokidar was my fix. |
@stdavis Thanks! I went with gaze too https://github.com/ensime/ensime-node/commit/463caa2c59c85879874fcceeb23a073f4560726b |
@es128 I tried to When I manually run Also I am hitting a blocker with Electron and fsevents: #131 |
Would break other users' attempts at Are you using How about "postinstall": "cd node_modules/fsevents; npm uninstall node-pre-gyp" |
@es128 we recently switched to using npm@3. Interestingly all dependencies of Actually this might not be a blocker for us after all because we already have custom code that only takes the @es128 can you confirm that deleting the entire |
Must be because of the bundled dependencies that it's not fully flattening, although I thought I was installing from the registry when I tested this... No, you cannot delete all of |
Ah, so I'm seeing cases where |
@es128 interestingly when I run |
Ah, because the other deps have been flattened. So maybe your earlier suggestion actually will work for you. |
Yup, I can verify this once I get fse.node to be loaded properly. |
Bundling this dependency is now a security issue, it bundles minimatch@3.0.0 (vuln details: https://snyk.io/vuln/npm:minimatch:20160620) via sub dependencies, with no maintainable way (beyond shrinkwrap hacking) to fix it at my project level (as far as I'm aware):
I've checked all the package.json version patterns in the above tree, and they would allow resolving to a patched version of minimatch with a fresh npm install, but I can't work out how to override the bundling to allow that. |
Maybe you can take a look at https://github.com/sheerun/npm-packer I've made. It preserves all paths inside package, and bundles properly even for old npm versions that re-install even if they are marked as in bundledDependencies. |
I'm aware of the minimatch issue but is there reason for concern here? If you have a credible attack that would affect fsevents, I'd like to hear it. Have you contacted the node-pre-gyp project? What did they say? |
I'm not aware of a credible attack in this case, but it does worry me that patched vulnerabilities can exist without the ability to apply patches in a maintainable way, especially given the dependency tree version patterns would specifically allow it. |
To be clear, the node-pre-gyp dependency tree isn't vulnerable. A fresh npm install would pull in the patched version of minimatch. |
I’ve had some conversations at NodeConf.EU which lead to me porting to N-API. However that requires a few experimental things to be backported to 6.x and 8.x. Once that is done, the prebuilt should only be needed once per version (since OS X and nose are then ABI compatible across versions). At that point it would be worth while to remove node-pre-gyp and ship the binary directly. On install we could then try loading that and on failure recompile. |
It's being removed in #237 |
Would it be possible to put node-pre-gyp as devDependency? Since 1.0.0 fsevents drags in more than 30 (just a guess) new modules via the dependency to node-pre-gyp. Since chokidar updated to fsevents recently, this has quite a big impact on shipping with chokidar 1.2.0.
The text was updated successfully, but these errors were encountered: