[BUG] `install --only=development` doesn't install needed dependencies if listed in production dependencies

msbit commented 4 years ago

Current Behavior:

If I have a list of dependencies and devDependencies such that devDependencies[x] depends on dependencies[y] and I run npm install --only=development, only devDependencies[x] is installed.

In a related twist, swapping the direction of dependencies (such that dependencies[x] depends on devDependencies[y]) and running npm install --only=development results in no packages being installed at all.

Expected Behavior:

When running npm install --only=development I would expect all required direct devDependencies and any transitive dependencies of any stripe (dependencies, devDependencies or unlisted) to be installed.

Steps To Reproduce:

A concrete example involves lodash and json-replace (the former with no dependencies, and the latter with only one dependency, on lodash):

$ cat package.json
{
  "name": "npm-dev-only",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "lodash": "*"
  },
  "devDependencies": {
    "json-replace": "*"
  }
}

$ rm -rf node_modules

$ npm install --only=development
npm WARN npm-dev-only@1.0.0 No description
npm WARN npm-dev-only@1.0.0 No repository field.

added 1 package from 1 contributor and audited 2 packages in 1.195s
found 8 vulnerabilities (4 low, 4 high)
  run `npm audit fix` to fix them, or `npm audit` for details

$ find node_modules -name package.json -type f
node_modules/json-replace/package.json

Swapping it around:

$ cat package.json 
{
  "name": "npm-dev-only",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "json-replace": "*"
  },
  "devDependencies": {
    "lodash": "*"
  }
}

$ rm -rf node_modules

$ npm install --only=development
npm WARN npm-dev-only@1.0.0 No description
npm WARN npm-dev-only@1.0.0 No repository field.

audited 3 packages in 0.464s
found 8 vulnerabilities (4 low, 4 high)
  run `npm audit fix` to fix them, or `npm audit` for details

$ find node_modules -name package.json -type f
find: node_modules: No such file or directory

Environment:

OS: macOS 10.13.6
Node: v14.5.0
NPM: 6.14.7

msbit commented 4 years ago

A bit of a script to reproduce:

mkdir npm-dev-only
pushd npm-dev-only
npm init -y
npm install --save-prod --save-exact lodash@1.3.1
npm install --save-dev --save-exact json-replace@0.0.1
rm -rf node_modules
npm install --only=development

This results in a package-lock.json as follows:

{
  "name": "npm-dev-only",
  "version": "1.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "json-replace": {
      "version": "0.0.1",
      "resolved": "https://registry.npmjs.org/json-replace/-/json-replace-0.0.1.tgz",
      "integrity": "sha1-mVKEpTnu1EjHgJhMf5S96HxuDpU=",
      "dev": true,
      "requires": {
        "lodash": "1.3.1"
      }
    },
    "lodash": {
      "version": "1.3.1",
      "resolved": "https://registry.npmjs.org/lodash/-/lodash-1.3.1.tgz",
      "integrity": "sha1-pGY7U2hriV/wdOK6UE37dqjit3A="
    }
  }
}

If you don't specify a version with the npm install --save-* commands, you get the following package-lock.json:

{
  "name": "npm-dev-only",
  "version": "1.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "json-replace": {
      "version": "0.0.1",
      "resolved": "https://registry.npmjs.org/json-replace/-/json-replace-0.0.1.tgz",
      "integrity": "sha1-mVKEpTnu1EjHgJhMf5S96HxuDpU=",
      "dev": true,
      "requires": {
        "lodash": "1.3.1"
      },
      "dependencies": {
        "lodash": {
          "version": "1.3.1",
          "resolved": "https://registry.npmjs.org/lodash/-/lodash-1.3.1.tgz",
          "integrity": "sha1-pGY7U2hriV/wdOK6UE37dqjit3A=",
          "dev": true
        }
      }
    },
    "lodash": {
      "version": "4.17.19",
      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.19.tgz",
      "integrity": "sha512-JNvd8XER9GQX0v2qJgsaN/mzFCNA5BRe/j8JN9d+tWyGLSodKQHKFicdwNYzWwI3wjRnaKPsGj1XkBjx/F96DQ=="
    }
  }
}

and the behaviour isn't exhibited.

tyler-laberge commented 4 years ago

We are seeing this issue too but in our case it involves transitive dependencies not being installed.

In our case we have https://www.npmjs.com/package/serverless-plugin-typescript declared in our dev dependencies, and https://www.npmjs.com/package/log4js declared in our regular dependencies. Each of these has a transitive dependency (regular not dev dependency) on the package https://www.npmjs.com/package/universalify.

package.json

{
  "name": "reproduce-npm-bug",
  "version": "1.0.0",
  "devDependencies": {
    "serverless-plugin-typescript": "^1.1.9",
    "typescript": "^3.9.7"
  },
  "dependencies": {
    "log4js": "^6.3.0"
  }
}

Regular install

npm install
npm ls universalify

reproduce-npm-bug@1.0.0 /example/reproduce-npm-bug 
├─┬ log4js@6.3.0
│ └─┬ streamroller@2.2.4
│   └─┬ fs-extra@8.1.0
│     └── universalify@0.1.2  deduped
└─┬ serverless-plugin-typescript@1.1.9
  └─┬ fs-extra@7.0.1
    └── universalify@0.1.2

Dev only install

rm -rf node_modules
npm install --only=dev
npm ls universalify

reproduce-npm-bug@1.0.0 /example/reproduce-npm-bug                                                                                                                                                       ├─┬ UNMET DEPENDENCY log4js@6.3.0
│ └─┬ UNMET DEPENDENCY streamroller@2.2.4
│   └─┬ UNMET DEPENDENCY fs-extra@8.1.0
│     └── UNMET DEPENDENCY universalify@0.1.2
└─┬ serverless-plugin-typescript@1.1.9
  └─┬ fs-extra@7.0.1
    └── UNMET DEPENDENCY universalify@0.1.2

npm ERR! missing: log4js@6.3.0, required by reproduce-npm-bug@1.0.0
npm ERR! missing: streamroller@2.2.4, required by log4js@6.3.0
npm ERR! missing: fs-extra@8.1.0, required by streamroller@2.2.4
npm ERR! missing: universalify@0.1.2, required by fs-extra@8.1.0
npm ERR! missing: universalify@0.1.2, required by fs-extra@7.0.1

I would expect that the universalify transitive dependency of serverless-plugin-typescript to be installed but it is not.

msbit commented 4 years ago

Thanks @tyler-laberge for the additional reproduction!

It does look like the same issue; if you're comfortable with it, could you try the patch from https://github.com/npm/cli/pull/1676 and see if that fixes it?

ruyadorno commented 4 years ago

Thanks to you all for the great work documenting all this here ❤️

Unfortunately as mentioned in the linked PR these changes might be too risky for v6 at this point in time.

@isaacs I'll keep this issue around in order to serve as a placeholder to remember to add these test cases to arborist/v7 - feel free to close it after that 😊

tyler-laberge commented 4 years ago

@msbit FWIW I just tried the patch and that does seem to fix the issue

msbit commented 4 years ago

Thanks @ruyadorno, getting this sorted out for v7 is still a good win.

ruyadorno commented 4 years ago

thanks @tyler-laberge for the handy reproduction case. I tested it with the latest beta version v7.0.0-beta.12 and validate it's working as expected there 😊

Thanks @msbit for bringing this one up to our attention!

sandstrom commented 2 years ago

@ruyadorno How about making a change like this?

// package.json
{
  …
  "dependencies": {
    "development": {
      "swipejs": "~2.2.14",
      "ua-parser-js": "~0.7"
    },
    "production": {
      "swipejs": "~2.2.14",
    },
    "linting": {
      "eslint": "~1.0.0"
    },
  },
  …
}

and in package-lock.json you'd have "groups" : ["development", "production", "linting"] instead of the current "dev": true flag.

That way, one could have arbitrary groups of dependencies, and you could easily resolve which ones to install during a given run, via e.g. npm install only=production,linting or npm install only=development.

A good use-case for this is to have a linting group that's only installed during certain CI-runs. But would also be backwards-compatible (functionality-wise, that is) with dev-dependencies.

ljharb commented 2 years ago

Using a "dependencies" name would utterly break every npm ecosystem tool that expects it to be a deps object.

sandstrom commented 2 years ago

@ljharb Yes, good point! This was mostly intended as a rough draft to illustrate the idea.

I agree with you that you wouldn't want to make this breaking.

What e.g. Ruby's Bundler did when they introduced dependency groups, was that the non-specified dependencies always belong to default, and you can then add additional groups on top of that.

A similar strategy could be used here, e.g. the devDependencies and dependencies groups could certainly remain.

Just from the top of my head, you could have a syntax where prefixDependencies would be the syntax to use for other groups.

dependencies -> production group [old]
devDependencies -> dev group [old]
lintingDependencies -> linting group [example]
`{prefix}Dependencies -> {prefix} group [syntax]

npm / cli