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

Revert "install/dedupe: fix hoisting of packages with peerDeps (#147)" #152

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

zkat
Copy link
Contributor

@zkat zkat commented Feb 4, 2019

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

@zkat zkat requested a review from a team as a code owner February 4, 2019 23:28
@aeschright aeschright requested a review from iarna February 5, 2019 20:09
@jaydenseric
Copy link

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?

@iarna
Copy link
Contributor

iarna commented Feb 7, 2019

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.

Copy link
Contributor

@aeschright aeschright left a 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.

@alexander-akait
Copy link

A lot of peoples can't use webpack with npm, sorry but is is shame for npm organization.
I do not like to say such things, but you are a package manager, you are basis for many developers and projects.

Any ETA for this fix? It will be a month soon since issue was reported.

@iarna
Copy link
Contributor

iarna commented Feb 16, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants