Nested optionalDependencies are not removed

[codyhatch]

If I try something like this on a Mac:

$ mkdir test
$ cd test
$ npm init
$ npm install --save chokidar
$ npm shrinkwrap --dev
$ shrinkpack

I believe that fsevents should be pruned from the rewritten npm-shrinkwrap.json file. However, it's not, which means that if I try to install this package on a linux server it will fail, because fsevents is still attempting to be installed.

This is with node v4.5.0, npm v3.10.7, and shrinkpack v0.17.0.

I believe this is the same issue which was raised in #17, and which should have been fixed in 75265ac. Am I doing something wrong, or is there a regression, or...?

My package.json:

{
  "name": "test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "chokidar": "^1.6.0"
  }
}

My initial npm-shrinkwrap.json: https://gist.github.com/codyhatch/56f559be6ef795f155ac2fe2bdf2ec91

My rewritten post-shrinkpack npm-shrinkwrap.json: https://gist.github.com/codyhatch/adbaa2d904e1ec5238f5ce861dddd3ba

[DrewML]

@codyhatch Looking at 75265ac, that fix only addresses removal of optionalDependencies that are specified in the top-level package.json for your project - it does not look to handle the optionalDependencies specified by your transitive deps.

As a work-around for the time being, you can add fsevents to the optionalDependencies in the package.json for your application.

I think the correct fix here would be to prune optional dependencies that exist anywhere in the tree, as @palmerj3 pointed out in #74.

@JamieMason thoughts?

[JamieMason]

Agree @DrewML that you'd have to pluck optionalDependencies from every package.json in the dependency graph to then prune it from the shrinkwrap.

I'll think about this some more, but I'm initially hesitant because I'm already not content with how much shrinkpack has needed to be a bag of patches for upsteam npm shrinkwrap issues. That said, npm shrinkwrap is shrinkpack's source of data, so it's pretty difficult to avoid.

[JamieMason]

I'll close #74 as a duplicate as there is more discussion in this thread.

According to NPM if you remove optionalDependencies in npm-shrinkwrap.json but they still exist in package.json NPM will install them anyways. I totally disagree with this behavior and will be working on a NPM PR to remove this behavior entirely. But I think you could handle this in shrinkpack.

Certainly removing optionalDeps from top level package.json would be doable but it's not clear to me right now if you will also need to handle this for lower level packages.
– @palmerj3

[JamieMason]

I've pushed a change to feat/75-nested-optional-deps which seems to work well, using the "optional": true properties found in shrinkwraps generated by newer versions of npm.

The test suite will need updating before it can be released though, as the pruning of optional dependencies results in different files in node_modules (the default behaviour when doing an npm install normally is to include optional dependencies).