symlink the metadeps with binaries into the topmost node_modules/.bin

[snagi]

Issue #3059

Summary
When running yarn install on a dependency tree with a nested child dependency that exposes a CLI binary, the CLI bin is not symlinked into node_modules/.bin as expected. Current behavior of yarn is to symlink the script to parent module's .bin folder.

$ git clone https://github.com/probablyup/sample_repo_a.git
$ cd sample_repo_a
$ yarn install
yarn install v0.21.3
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
✨  Done in 16.16s.
$ ls node_modules/.bin
ls: node_modules/.bin: No such file or directory
$ ls node_modules/sample_repo_b/node_modules/.bin/
eslint

This pull request adds the functionality to symlink the binaries/scripts in both locations; current yarn behavior as well as symlink to root node_modules folder.

Test plan

1. Checkout https://github.com/probablyup/sample_repo_a.git
2. Install dependencies using yarn
3. A symlink to node_modules/eslint/bin/eslint.js should be created in node_modules/.bin. This is npm's behavior.

$ git clone https://github.com/probablyup/sample_repo_a.git
$ cd sample_repo_a

$ yarn install
yarn install v0.21.3
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
✨  Done in 12.99s.

$ ls -al node_modules/.bin/
total 64
drwxr-xr-x   10 snagi  users   340 Apr 28 16:19 .
drwxr-xr-x  139 snagi  users  4726 Apr 28 16:19 ..
lrwxr-xr-x    1 snagi  users    41 Apr 28 16:19 acorn -> ../acorn-jsx/node_modules/acorn/bin/acorn
lrwxr-xr-x    1 snagi  users    23 Apr 28 16:19 eslint -> ../eslint/bin/eslint.js
lrwxr-xr-x    1 snagi  users    25 Apr 28 16:19 esparse -> ../esprima/bin/esparse.js
lrwxr-xr-x    1 snagi  users    28 Apr 28 16:19 esvalidate -> ../esprima/bin/esvalidate.js
lrwxr-xr-x    1 snagi  users    25 Apr 28 16:19 js-yaml -> ../js-yaml/bin/js-yaml.js
lrwxr-xr-x    1 snagi  users    20 Apr 28 16:19 mkdirp -> ../mkdirp/bin/cmd.js
lrwxr-xr-x    1 snagi  users    16 Apr 28 16:19 rimraf -> ../rimraf/bin.js
lrwxr-xr-x    1 snagi  users    19 Apr 28 16:19 shjs -> ../shelljs/bin/shjs

$ ls -al node_modules/sample_repo_b/node_modules/.bin/
total 8
drwxr-xr-x    3 snagi  users  102 Apr 28 16:19 .
drwxr-xr-x    3 snagi  users  102 Apr 28 16:19 ..
lrwxr-xr-x    1 snagi  users   29 Apr 28 16:19 eslint -> ../../../eslint/bin/eslint.js

[rally25rs]

Related issues: #2874 #3201 #1867 (there is some discussion in 2874 as to how this was once fixed, then reverted, and how it might be expected to function going forward)

[rally25rs]

The copyModules() function that this change is in is called for each package in the "deterministic" order that they are extracted (i think?). However, the order does not take into account the depth or priority of packages.

If you have something like:

"dependencies": {
  "eslint": "3.13.1",
  "standard": "1.2.3"
}

and assume that the standard package also depends on a different version of eslint eslint@3.7.1

There is nothing in the current implementation of this PR that would guarantee that node_modules/.bin/eslint was a link to the 3.13.1 version of eslint, not 3.7.1

I believe result should be:

node_modules/eslint -- version 3.13.1
node_modules/.bin/eslint -> ../eslint/bin/index.js -- version 3.13.1
node_modules/standard/node_modules/eslint -- version 3.7.1
node_modules/standard/node_modules/.bin/eslint -> ../eslint/bin/index.js -- version 3.7.1

but with this solution, the node_modules/.bin/eslint would simply be linked to whichever was extracted last.

---

Someone please correct me if I'm wrong... which I very well could be. All this package looping in the code is hard to follow sometimes 😄

[rally25rs]

Found a second problem; during unit testing, this fix will write the symlinks to the node_modules/.bin directory of the Yarn project itself, not the location where the tests are being sandboxed.

When this code executes during unit tests:

      await promise.queue(flatTree, async ([dest, {pkg}]) => {
        const binLoc = path.join(dest, this.config.getFolder(pkg));

        await this.linkBinDependencies(pkg, binLoc);
        await this.linkBinDependencies(pkg, this.config.getFolder(pkg));
        tickBin(dest);
      }, 4);

binLoc is a full path to a test sandbox, like /var/folders/yf/gcmnw1y96k31lh9ttjhfm8v9ssxkkq/T/yarn-install-nested-bin-1493746906830-0.5174607616738938/node_modules/...

but the 2nd call to linkBinDependencies() just passes this.config.getFolder(pkg) which is the relative node_modules so is relative to where the test command was executed.

---

I'm trying to circle back and start by writing some unit tests around what behavior we actually expect. I'm not sure this will be a solvable problem in just package-linker though.

---

I think we also need to figure out what the "correct" behavior is if we have a dependency graph like:

myPkg -> pkgA -> eslint@1
myPkg -> pkgB -> eslint@2

Which eslint is in node_modules/.bin? First one resolved? last one resolved? (I'm assuming questions like this are why this bug has been open for months).

[voxsim]

@rally25rs you are awesome! could you take some time and copy everything you write here to the original issue #2874?

@snagi do you want to work on this? Right now this pull request it is not enough to address the problem, I am sorry :(

[snagi]

@rally25rs @voxsim I agree with the comments. I am comparing the behaviors with npm and will identify how npm is catering to these scenarios. post that will update this PR to cater these scenarios.

[voxsim]

@snagi I think #1210 and #1867 are both useful for your purpose :)

[rally25rs]

Hey gang, check out #3310 - this is my 1st shot at resolving this issue. I made a new PR instead of reworking this one just because I had a branch hanging out on my laptop that I had already started...

[rally25rs]

This is now fixed in #3310 so this PR can be closed.