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

Find any path to top in isOnlyDev #1676

Closed
wants to merge 1 commit into from
Closed

Find any path to top in isOnlyDev #1676

wants to merge 1 commit into from

Conversation

msbit
Copy link

@msbit msbit commented Aug 14, 2020

When determining whether a node is considered 'only' a development
dependency of the top-level, check whether there are any paths
from the specific node to the top-level through a direct
development dependency of the top-level to avoid issues where the
shortest path from the node to top-level is via a direct
production dependency, thus causing the node to be considered a
production dependency.

This comes into play particularly when issuing:

npm install --only=development

as this will end up in a situation where transitive dependencies
of a development dependency may not be installed.

References

Fixes #1669

When determining whether a node is considered 'only' a development
dependency of the top-level, check whether there are any paths
from the specific node to the top-level through a direct
development dependency of the top-level to avoid issues where the
shortest path from the node to top-level is via a direct
production dependency, thus causing the node to be considered a
production dependency.

This comes into play particularly when issuing:

  npm install --only=development

as this will end up in a situation where transitive dependencies
of a development dependency may not be installed.
@isaacs
Copy link
Contributor

isaacs commented Aug 17, 2020

While this is a correct fix to a legitimate flaw, unfortunately the risk of making any changes in the Installer in v6 is just too great at this point. There are too many people relying on these kinds of edge cases and "incorrect" behaviors without realizing it, and we don't want to break their projects in a legacy version.

Primary development is focused right now on the v7 beta. If you have the same problem in that version, then please post an issue and we can discuss the best approach to fixing it. Regardless, I'll take the test case you provided and make sure that v7 does the right thing.

I'm sorry for having to deliver a disappointing response on this PR. I'm sure it wasn't easy to track this down, and I do appreciate the engagement and thoughtfulness that went into this patch.

@isaacs isaacs closed this Aug 17, 2020
@ruyadorno ruyadorno added the Wontfix this will not be worked on label Aug 17, 2020
@msbit
Copy link
Author

msbit commented Aug 19, 2020

Thanks @isaacs, I understand the point about existing v6 users depending on the quirks of the existing behaviour.

When you're replicating the tests on arborist, it would be good to also include a test for the transitive issues @tyler-laberge had in #1669 too (if that's not already on your radar 😄 ).

@msbit msbit deleted the is-only-dev-find-any-path branch April 25, 2021 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Wontfix this will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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