Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Fix branch pruning bugs in move/remove actions #10655

Closed
wants to merge 13 commits into from

Conversation

iarna
Copy link
Contributor

@iarna iarna commented Dec 1, 2015

This is prooooobably the rest of the ENOENT / missing module / etc errors in 3.x:

So npm@3 generates a list of actions to take against the tree on disk. With the exception of lifecycle scripts, it expects these all to be able to act independently without interfering with each other.

This means, for instance, that one should be able to upgrade b in a→b→c without reinstalling c.

That works fine btw.

But it also means that the move action should be able to move b in a→b→c@1.0.1 to a→d→b→c@1.0.2 without moving or removing c@1.0.1 and while leaving c@1.0.2 in place if it was already installed.

That is, the move action moves an individual node, replacing itself with an empty spot if it had children. It's not a graft operation.

So, we already took care to leave c@1.0.1 in place, but were stomping on the destination and so c@1.0.2 was being removed.

There was also a bug w/ remove where it was pruning the entire tree at the remove point, prior to running moves and adds.

This was fine most of the time, but if we were moving one of the deps out from inside it, kaboom.

Really, there was no need for this. I suspect this dates to a point where move was not able to handle its targets already existing.

The reason we didn't see any of these bugs before in Travis was that all of these actions were applying to deps bundled with npm. Previously bundled deps didn't go through the normal installer actions.

Really though, we shouldn't be using moves to try to optimize away bundled deps 'cause we're going to have to extract those regardless so it just adds complexity without any benefit.

@iarna
Copy link
Contributor Author

iarna commented Dec 3, 2015

@zkat sez 🐑

iarna added a commit that referenced this pull request Dec 3, 2015
iarna added a commit that referenced this pull request Dec 3, 2015
iarna added a commit that referenced this pull request Dec 3, 2015
What is the world coming to?!

Credit: @iarna
Reviewed-By: @zkat
PR-URL: #10655
iarna added a commit that referenced this pull request Dec 3, 2015
iarna added a commit that referenced this pull request Dec 3, 2015
iarna added a commit that referenced this pull request Dec 3, 2015
And actually, just get rid of the remove commit phase, it's unhelpful as
written.

Credit: @iarna
Reviewed-By: @zkat
PR-URL: #10655
iarna added a commit that referenced this pull request Dec 3, 2015
iarna added a commit that referenced this pull request Dec 3, 2015
iarna added a commit that referenced this pull request Dec 3, 2015
@othiym23
Copy link
Contributor

othiym23 commented Dec 4, 2015

Landed as a whole bunch of commits for npm@3.5.2. Excellent work, @iarna!

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

Successfully merging this pull request may close these issues.

None yet

2 participants