Can't npm shinkwrap because of extraneous deps #92

Closed
margh opened this Issue Dec 31, 2013 · 14 comments

Comments

Projects
None yet

margh commented Dec 31, 2013

Side effect of the osx post-install script.

Thoughts feelings emotions?


npm ERR! Error: Problems were encountered
npm ERR! Please correct and try again.
npm ERR! extraneous: fsevents@0.1.6 /Users/nathanr/webclient/node_modules/chokidar/node_modules/fsevents
npm ERR! extraneous: recursive-readdir@0.0.2 /Users/nathanr/webclient/node_modules/chokidar/node_modules/recursive-readdir
...
npm ERR! System Darwin 11.4.2
npm ERR! command "node" "/usr/local/bin/npm" "shrinkwrap"
npm ERR! node -v v0.10.24
npm ERR! npm -v 1.3.21

Owner

paulmillr commented Dec 31, 2013

@izs any idea what's this? We have a postinstall script that installs deps only on os x

nrako added a commit to nrako/chokidar that referenced this issue Jan 6, 2014

Collaborator

es128 commented Jan 6, 2014

I think adding --save-optional to the npm command in the postinstall script would be the best bet.

nrako commented Jan 6, 2014

oh good idea!

kleinsch commented Jan 8, 2014

Agreed with @es128 . Running postinstall script on a Mac adds packages that aren't in package.json, which breaks npm shrinkwrap. Either the dependencies need to be in package.json (which sounds like a bad call based on discussion to #94 ) or they need to be added to it as part of the postinstall script.

@raine raine referenced this issue in npm/npm Jan 9, 2014

Closed

shrinkwrap error #4435

Contributor

vjpr commented Jan 22, 2014

+1. Breaks shrinkwrap for me too. The post install script is a bad idea.

A post install message should be shown to the user that they need to install these libraries manually in their app.

Or when the module is first used check for the library and throw an error that they need to install libs.

There should also be a fallback if possible.

Workaround

npm install fsevents recursive-readdir --save and rm -rf node_modules/chokidar/node_modules

Collaborator

es128 commented Jan 22, 2014

At this point, I agree that we have not been successful in finding a clean way to install fsevents conditionally, but I think it should remain possible for individual users to enable it. Even with --save-optional it appears #95 will still be an issue. OTOH, being able to easily enable fsevents may be a good solution for users like the one at brunch/brunch#783.

I suggest switching the postinstall to something arbitrary, like fsevents, and provide instructions in the readme and possibly in the console during install for users to manually npm run fsevents if they want these features. Dependents of chokidar like karma and brunch, if they decide they want to, could also mention this in their docs and/or set up their own npm script so users don't have to dig down into node_modules for it.

If I find the time today I'll prepare a PR.

jagill commented Jan 22, 2014

I would love some way to be able to instal fsevents when useful, and remove it when not. It dramatically improves performance on OS X, but we also need to make builds on other architectures.

It seems safer to not have it by default, but allow OS X users to install it and use it.

Contributor

quicksnap commented Feb 11, 2014

We're having a build issue with the 0.1.6 version of fsevents that was resolved in 0.2.0. I was hoping to shrinkwrap, but I was brought here.

Would love to avoid forking chokidar for this little issue!

Contributor

vojtajina commented Mar 11, 2014

@paulmillr can you push this to NPM?

Contributor

IgorMinar commented Mar 12, 2014

This will still be an issue even with optionalDependencies because of an npm bug: npm/npm#2679 (comment)

(just fyi, I still think that declaring these as optional deps is a good thing)

wenzowski pushed a commit to wenzowski/derby that referenced this issue Mar 12, 2014

Workaround: If you don't need fsevents and recursive-readdir in your deployment env. Just go into node_modules/chokidar/node_modules and rm -rf those directories. Then do npm_shrinkwrap. if you do need them (if your env is OSX??). Go into node_modules/chokidar/package.json and add them as dependecies.

Contributor

IgorMinar commented Mar 25, 2014

@startswithaj you have to do this every time you upgrade which is far from ideal. My PR #106 fixes the problem by using optionalDependencies and fsevent 0.2.0 with "os": ["darwin"] declaration in package.json.

@paulmillr paulmillr closed this in #106 Mar 25, 2014

@IgorMinar totally agree. Nice work on the fix.

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