-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Revert "install/dedupe: fix hoisting of packages with peerDeps (#147)" #152
Conversation
Please don't revert a fix without having another fix ready. Until @sokra's fix landed in npm@6.8.0-next.0 we've been literally forced to use Yarn to install our Next.js project; there were no workarounds for npm getting it totally wrong resulting in bundle errors. If this fix is reverted, I will have no choice but to use Yarn again. But for how long? |
We're not going to ship a non-prerelease with this patch, because it causes exactly the same problem for a different set of people who haven't had it up till now. We also aren't going to let it completely halt releases just for this issue. That doesn't mean it isn't an important issue, but the solution is more complicated than it appears at first. I don't want to just play whack-a-mole swapping which group of people we break with each release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll be reverting #147 (accepting this) to prevent shipping with a breaking change.
A lot of peoples can't use Any ETA for this fix? It will be a month soon since issue was reported. |
@evilebottnawi We've been aware of this for since shortly after the npm@3 launch. Of course, the situation prior to that, with npm@2, which triggered installation of peer deps, was even worse. This isn't so much a bug as a structural problem with peer dependencies. I do think we have an approach to resolve this, but it will take rewriting the tree resolver to be two pass from the current single pass. It's on our longer term roadmap, but not yet scheduled for our small team. However, this discussion does make me think that getting an RFC with our intended approach out sooner rather than later would be valuable. |
This reverts commit 91314e7.
Fixes: https://npm.community/t/npm-6-8-0-next-0-regression-in-maximally-flat-install/5118
This reverts @sokra's previous patch in #147 -- as we suspected, it fixed one set of use-cases by breaking another, and the actual fix for the peerDeps issue we've had since npm@3 is going to need to be bigger and more involved. The issue comes down to things with peerDeps having to be mutually requireable, and the shape of the tree isn't really determined by the first time we run into a peerDep, so we basically would need to reshape the tree every time we run into one, or a package that requires one. That's my understanding, at least. /cc @iarna