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

feat(arborist)!: install optional peer dependencies #5301

Draft
wants to merge 1 commit into
base: latest
Choose a base branch
from

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Aug 11, 2022

current versions of npm do not attempt to install optional peer dependencies, this feature changes that. one caveat of this is that in the event of an optional peer and a regular peer conflicting, we will allow the optional peer to become invalid (meaning the non-optional peer takes precedence). we opted to take this approach in the hopes that it would minimize new ERESOLVE errors due to these new dependencies being installed.

for the moment we're considering this as a breaking change due to the potential for package-lock.json churn, as well as the potential for new bugs to surface. as such, it is unlikely that we will land this change before the first release of npm 9.

closes #4859

@nlf nlf requested a review from a team as a code owner August 11, 2022 23:13
@arcanis
Copy link
Contributor

arcanis commented Aug 12, 2022

Optional peer dependencies are intended to be optional, so they shouldn't be installed by default (and in fact you even have open bugs suggested you're already installing them, against their semantics).

With this change, what difference do you see between peer deps and optional peer deps, in npm? 🤔

@lukekarrys lukekarrys added the semver:major backwards-incompatible breaking changes label Aug 22, 2022
@nlf nlf marked this pull request as draft August 26, 2022 00:36
@nlf
Copy link
Contributor Author

nlf commented Aug 26, 2022

With this change, what difference do you see between peer deps and optional peer deps, in npm? 🤔

the same as the difference between dependencies and optionalDependencies, attempt to install but if it fails ignore it.

tbh i think i'd rather stop installing optional dependencies than start installing optional peers by default, but there's definitely more thinking and talking to be done before we land on a decision.

@nlf
Copy link
Contributor Author

nlf commented Aug 26, 2022

i've converted this pull request back to a draft to convey that it is only a proposal. we have a lot of things to consider before changing something like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm can install an invalid tree with transitive optional peer deps
3 participants