yarn install does not remove extraneous nested node_modules folders

[chancancode]

Bug description

Note: I don't believe this issue is specifically related to the --frozen-lockfile and --check-files flags. I am just including them here in the reproduction because that seems to be the most strict version that would rule out a bunch of other factors.

General high-level description of the problem

When starting with an existing node_modules directory with incorrect packages, yarn install --frozen-lockfile --check-files does install the correct versions of the packages, but sometimes fail to remove the nested node_modules directories deep within the previous versions of the packages.

Let's say, you started on the master branch with the following package.json and simplified yarn.lock

{
  "dependencies": {
    "foo": "2.0.0", // depends on bar 2.0.0
    "bar": "1.0.0"
  }
}

"[email protected]":
  version "2.0.0"
  dependencies:
    "bar" "2.0.0"

"[email protected]":
  version "1.0.0"

"[email protected]":
  version "2.0.0"

After a yarn install --frozen-lockfile --check-files, you will end up with this node_modules structure:

node_modules
├─ [email protected]
│  └─ node_modules
│     └─ [email protected]
└─ [email protected]

This is all expected. Because our package and foo both requires incompatible versions of bar, there will ended up being a copy of bar nested inside the node_modules folder of foo.

Seems reasonable.

Now we will switch to and older branch with git checkout, which has the following package.json and simplified yarn.lock`:

{
  "dependencies": {
    "foo": "1.0.0", // depends on bar 1.0.0
    "bar": "1.0.0"
  }
}

"[email protected]":
  version "1.0.0"
  dependencies:
    "bar" "1.0.0"

"[email protected]":
  version "1.0.0"

After a yarn install --frozen-lockfile --check-files, you will expect to end up with this node_modules structure:

node_modules
├─ [email protected]
└─ [email protected]

However, in some situations it is possible that yarn install --frozen-lockfile --check-files will leave you will the following node_modules structure instead:

node_modules
├─ [email protected]
│  └─ node_modules
│     └─ [email protected]
└─ [email protected]

The problem here is that, while it correctly "patched" node_modules/foo to be 1.0.0, it didn't remove the nested node_modules folder. This is inconsistent with the lockfile and ultimately incorrect, because foo will get the wrong version of bar at runtime.

The actual description of the real-world problem we encountered

I tired to recreate a minimal reproduction, but I failed at capturing the exact condition that triggers this bug. The actual bug probably requires more permissive version ranges (using ^, and just variation of compatible minor versions), deeper nesting and so on.

I do have a reliable reproduction of the problem from ember.js, just not a very minimal one. See the Command section for the exact reproduction steps.

This is a description of the actual problem we encountered. On our master branch, running yarn install --frozen-lockfile --check-files will result in the following node_modules structure per the lockfile:

node_modules
├─ @babel
│  ├─ [email protected]
│  │  └─ node_modules
│  │     └─ @babel
│  │        ├─ [email protected] (depends on @babel/helper-validator-identifier ^7.9.5)
│  │        └─ ...
│  ├─ [email protected]
│  ├─ [email protected] (depends on @babel/helper-validator-identifier ^7.9.5)
│  └─ ...
└─ ...

We run our tests on Travis CI. Upon a successful build, it will package up the node_modules folder, along with the yarn cache folder into a cache tarball for subsequent builds to use. Normally, these caches are isolated to the branch they were made from.

We also have a LTS branches that we support but they get new commits very infrequently. Yesterday, we opened a new PR against one of the LTS branches. Since the last build was months ago, its own cache has already been deleted (LRU up to some storage limit). So, instead, it fallback to the cache from the master branch, which means the build started off with the exact same node_modules structure we saw above.

This is not really that helpful, as the packages needed on the LTS branches are much older than the master branch, so this cache didn't really give us a very useful starting point. But no problem, we are going to run yarn install --frozen-lockfile --check-files, which can savage what it can from this and should still get us to the state we want, which is this according to the lockfile:

node_modules
├─ @babel
│  ├─ [email protected]
│  ├─ [email protected] (has no dependency on @babel/helper-validator-identifier)
│  └─ ...
└─ ...

Note that there shouldn't be a nested @babel/types inside @babel/core, and the @babel/helper-validator-identifier package is not needed here.

However, instead, this is what we got after a successful install:

node_modules
├─ @babel
│  ├─ [email protected]
│  │  └─ node_modules
│  │     └─ @babel
│  │        ├─ [email protected] (depends on @babel/helper-validator-identifier ^7.9.5)
│  │        └─ ...
│  ├─ [email protected] (has no dependency on @babel/helper-validator-identifier)
│  └─ ...
└─ ...

What it did right:

✅ Installed @babel/core 7.4.5 at the root
✅ Installed @babel/types 7.4.4 at the root
✅ Removed @babel/helper-validator-identifier from the root since nothing uses it anymore

What it did not do right:

❌ Remove @babel/core/node_modules/@babel/types which is at version 7.9.6

This is incorrect because 1. it is inconsistent with what the lockfile asked for, and 2. at runtime, @babel/core will get the wrong copy of @babel/types, 3. at runtime, that wrong copy of @babel/types will fail to find @babel/helper-validator-identifier.

Here is a screenshot of it happening in action:

Our current workaround is to remove node_modules before installing. Obviously that is not ideal, but more concerning is the fact that we don't know what are the circumstances that triggers this bug, and if it wasn't for the missing @babel/helper-validator-identifier dependency, if the packages are "compatible enough", it could have "worked" silently without anyone noticing, and that could have caused us to publish invalid artifacts. For example, we may not have transpiled away syntaxes that are illegal per the support matrix of the LTS version.

Unfortunately, I ran out of time to reduce this down to see what the causes are, but hopefully the information here will help you find it.

Command

$ git clone https://github.com/emberjs/ember.js
$ cd ember.js
# The latest master commit as of this bug report
$ git checkout 42b1a8f17726047af364ab9e9f67a88c1b6294ae
# Some warnings are 
$ yarn install --frozen-lockfile --check-files
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
(Some unmet peer dependency warnings)
[5/5] 🔨  Building fresh packages...
✨  Done in 48.07s.
$ cat node_modules/@babel/core/package.json | jq .version
"7.9.6"
$ cat node_modules/@babel/types/package.json | jq .version
"7.9.5"
$ cat node_modules/@babel/core/node_modules/@babel/types/package.json | jq .version
"7.9.6"
# Now switch to the latest commit of lts-3-12 branch as of this bug report
$ git checkout 3af9fca4f78a435fe3a3d3302c353fad9c55f214
$ yarn install --frozen-lockfile --check-files
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
(Some unmet peer dependency warnings)
[5/5] 🔨  Building fresh packages...
✨  Done in 32.24s.
$ cat node_modules/@babel/core/package.json | jq .version
"7.4.5"
$ cat node_modules/@babel/types/package.json | jq .version
"7.4.4"
$ cat node_modules/@babel/core/node_modules/@babel/types/package.json | jq .version
"7.9.6"

What is the current behavior?

node_modules/@babel/core/node_modules/@babel/types was not removed. The last command showed version "7.9.6" was kept around erroneously.

What is the expected behavior?

node_modules/@babel/core/node_modules/@babel/types should be removed. The last command should fail due to node_modules/@babel/core/node_modules/@babel/types/package.json not existing.

Steps to Reproduce

(See Command)

Environment

• Node Version: 10.20.1
• Yarn v1 Version: 1.22.4
• OS and version: Linux Ubuntu 16.04 Xenial on Travis CI, macOS 10.14.6 locally

[merceyz]

Closing as fixed in v2 where the node_modules linker has been greatly improved

https://yarnpkg.com/getting-started/migration