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/dedupe: fix hoisting of packages with peerDeps #147

Merged
merged 8 commits into from
Jan 29, 2019

Conversation

sokra
Copy link
Contributor

@sokra sokra commented Jan 24, 2019

fixes https://npm.community/t/packages-with-peerdependencies-are-incorrectly-hoisted/4794

This PR changes earliestInstallable to respect peerDependencies in the package. It hoists packages with peer deps no further than the package containing dependencies on the peer dependencies.

It also changes dedupe to start looking for earliest installable location in the current tree instead of the parent. This ensures that break conditions (which return tree in earliestInstallable) are correctly followed. To avoid finding itself, the package is marked as removed (this could be solved differently with an argument on earliestInstallable to ignore itself).

lib/install/deps.js Outdated Show resolved Hide resolved
Co-Authored-By: sokra <[email protected]>
@zkat
Copy link
Contributor

zkat commented Jan 24, 2019

Hi! Thanks for submitting this PR. Me and @iarna are gonna be doing a more thorough review of this tomorrow when we can pair on it. The initial response is that we're pretty sure we've tried this approach before and it broke a different set of people, but we're gonna look at it closely and see if this addresses the other breakage.

@sokra
Copy link
Contributor Author

sokra commented Jan 25, 2019

Thanks for considering it 😀

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this! Go figure that it'd be such a small patch to resolve this issue. I guess our assumptions about needing that rewrite weren't quite there, though that's coming anyway ;)

One last thing we'd like to see before landing this is a test to verify the behavior, and it looks like you're most of the way there because you have a repro repo, especially one that uses file:. A good way to make a test for this is to clone the repo and call scripts/maketest on it, which will give you a nice test template to work off of.

Thanks again, and let me know if you have any questions! 🎉

var peerDeps = pkg.peerDependencies
if (peerDeps) {
if (Object.keys(peerDeps).some(function (name) {
return deps[name] || devDeps[name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a check where we only check devDeps if tree.isTop (see the var devDeps ... section just above this one). We think this is otherwise fine and actually a pretty nice patch!

lib/dedupe.js Outdated
// child is marked as removed so it's ignored when finding a location
child.removed = true
var hoistTo = earliestInstallable(tree, tree, child.package, log)
child.removed = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd rather add an argument here than patch a child statefully like this. This otherwise makes sense.

@zkat zkat added semver:patch semver patch level for changes in-progress labels Jan 25, 2019
@sokra
Copy link
Contributor Author

sokra commented Jan 28, 2019

@zkat ready for review again

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks wonderful! Thank you so much again! This was such a pain for people (and us). 😅

@aeschright aeschright changed the base branch from latest to release-next January 29, 2019 23:43
@aeschright aeschright merged commit 91314e7 into npm:release-next Jan 29, 2019
@sokra sokra deleted the bugfix/hoist-peer-dependencies branch January 30, 2019 18:32
@paulmelnikow
Copy link

Hi! Do you know which version of npm includes / will include this fix? Thanks!

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.

7 participants