Skip to content
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

install: Don't omit any deps of dev deps if --only=dev #69

Merged
merged 2 commits into from
Mar 4, 2019
Merged

install: Don't omit any deps of dev deps if --only=dev #69

merged 2 commits into from
Mar 4, 2019

Conversation

jviotti
Copy link
Contributor

@jviotti jviotti commented Sep 14, 2018

At the moment, npm will NOT install a dependency that is both a
production and a development dependency in the same tree if
--only=dev.

Consider the following scenario:

  • package1 has prod-dependency in dependencies
  • package1 has dev-dependency in devDependencies
  • prod-dependency has sub-dependency in dependencies
  • dev-dependency has sub-dependency in dependencies

So both prod-dependency and dev-dependency directly depend on
sub-dependency. Since sub-dependency is required by at least one
production dependency, then npm won't consider it as "only a dev
dependency", and therefore running npm install --only=dev will result
in the following tree:

package1/
    node_modules/
        dev-dependency

Notice that sub-dependency has ben completely omitted from the tree,
even though dev-dependency clearly requires it, and therefore it will
not work.

This commit makes --only=dev always install required dependencies of
dev dependencies, so you never end up with a broken tree.

For a more real-world reproducible example, try to run npm install --only=dev in
https://github.com/resin-io/etcher/tree/a338c6e60a10aad00008febfe92e2556dcf586c6.
The resulting tree will be completely broken, but we would not have
identified this if several install scripts wouldn't fail as a result.

Signed-off-by: Juan Cruz Viotti jv@jviotti.com

@jviotti jviotti requested a review from a team as a code owner September 14, 2018 12:48
At the moment, npm will NOT install a dependency that is both a
production and a development dependency in the same tree if
`--only=dev`.

Consider the following scenario:

- `package1` has `prod-dependency` in `dependencies`
- `package1` has `dev-dependency` in `devDependencies`
- `prod-dependency` has `sub-dependency` in `dependencies`
- `dev-dependency` has `sub-dependency` in `dependencies`

So both `prod-dependency` and `dev-dependency` directly depend on
`sub-dependency`. Since `sub-dependency` is required by at least one
production dependency, then npm won't consider it as "only a dev
dependency", and therefore running `npm install --only=dev` will result
in the following tree:

```
package1/
    node_modules/
        dev-dependency
```

Notice that `sub-dependency` has ben completely omitted from the tree,
even though `dev-dependency` clearly requires it, and therefore it will
not work.

This commit makes `--only=dev` always install required dependencies of
dev dependencies, so you never end up with a broken tree.

For a more real-world reproducible example, try to run `npm install
--only=dev` in
https://github.com/resin-io/etcher/tree/a338c6e60a10aad00008febfe92e2556dcf586c6.
The resulting tree will be completely broken, but we would not have
identified this if several install scripts wouldn't fail as a result.

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti
Copy link
Contributor Author

jviotti commented Oct 30, 2018

Ping @iarna @isaacs

@zkat zkat added semver:patch semver patch level for changes needs-discussion labels Nov 13, 2018
@zkat
Copy link
Contributor

zkat commented Nov 13, 2018

I'd like a review from @iarna on this one.

@jviotti
Copy link
Contributor Author

jviotti commented Nov 28, 2018

Re-ping @iarna :)

@nazrhom
Copy link

nazrhom commented Jan 30, 2019

@iarna @zkat any progress on this?

@aeschright aeschright requested a review from iarna February 7, 2019 23:02
@iarna iarna merged commit 13f77b8 into npm:latest Mar 4, 2019
zkat pushed a commit that referenced this pull request Mar 5, 2019
* install: Don't omit any deps of dev deps if --only=dev

At the moment, npm will NOT install a dependency that is both a
production and a development dependency in the same tree if
`--only=dev`.

Consider the following scenario:

- `package1` has `prod-dependency` in `dependencies`
- `package1` has `dev-dependency` in `devDependencies`
- `prod-dependency` has `sub-dependency` in `dependencies`
- `dev-dependency` has `sub-dependency` in `dependencies`

So both `prod-dependency` and `dev-dependency` directly depend on
`sub-dependency`. Since `sub-dependency` is required by at least one
production dependency, then npm won't consider it as "only a dev
dependency", and therefore running `npm install --only=dev` will result
in the following tree:

```
package1/
    node_modules/
        dev-dependency
```

Notice that `sub-dependency` has ben completely omitted from the tree,
even though `dev-dependency` clearly requires it, and therefore it will
not work.

This commit makes `--only=dev` always install required dependencies of
dev dependencies, so you never end up with a broken tree.

For a more real-world reproducible example, try to run `npm install
--only=dev` in
https://github.com/resin-io/etcher/tree/a338c6e60a10aad00008febfe92e2556dcf586c6.
The resulting tree will be completely broken, but we would not have
identified this if several install scripts wouldn't fail as a result.

PR-URL: #69
Credit: @jviotti
Reviewed-By: @iarna
@jviotti jviotti deleted the only-dev-production-dep branch March 12, 2019 08:24
petershin pushed a commit to petershin/cli that referenced this pull request Mar 21, 2019
* install: Don't omit any deps of dev deps if --only=dev

At the moment, npm will NOT install a dependency that is both a
production and a development dependency in the same tree if
`--only=dev`.

Consider the following scenario:

- `package1` has `prod-dependency` in `dependencies`
- `package1` has `dev-dependency` in `devDependencies`
- `prod-dependency` has `sub-dependency` in `dependencies`
- `dev-dependency` has `sub-dependency` in `dependencies`

So both `prod-dependency` and `dev-dependency` directly depend on
`sub-dependency`. Since `sub-dependency` is required by at least one
production dependency, then npm won't consider it as "only a dev
dependency", and therefore running `npm install --only=dev` will result
in the following tree:

```
package1/
    node_modules/
        dev-dependency
```

Notice that `sub-dependency` has ben completely omitted from the tree,
even though `dev-dependency` clearly requires it, and therefore it will
not work.

This commit makes `--only=dev` always install required dependencies of
dev dependencies, so you never end up with a broken tree.

For a more real-world reproducible example, try to run `npm install
--only=dev` in
https://github.com/resin-io/etcher/tree/a338c6e60a10aad00008febfe92e2556dcf586c6.
The resulting tree will be completely broken, but we would not have
identified this if several install scripts wouldn't fail as a result.

PR-URL: npm#69
Credit: @jviotti
Reviewed-By: @iarna
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants