Fix: handle empty bin strings

[rhburrows]

Summary

This is a followup PR for #6512. It prevents yarn from creating a link to a dependency's root directory within the .bin/ directory if the dependency has an empty string for "bin" in its package.json.

Test plan

After the change:

rhburrows$ yarn-local add file-loader --cache-folder ./cache
yarn add v1.13.0-0
info No lockfile found.
... OUTPUT ...
✨  Done in 1.17s.
rhburrows$ ls -al node_modules/.bin/
total 0
drwxr-xr-x   3 rhburrows  staff   96 Oct  7 16:59 .
drwxr-xr-x  18 rhburrows  staff  576 Oct  7 16:59 ..
lrwxr-xr-x   1 rhburrows  staff   19 Oct  7 16:59 json5 -> ../json5/lib/cli.js

[Gudahtt]

Looks great, thanks!

Could you add an entry in CHANGELOG.md describing this change as well?

[rally25rs]

I think there should be a test added to __tests__/commands/install/bin-links.js which tests the actual existence of the links. The test that was added originally in this PR doesn't really test that a bin link was not created.

---

edit

I don't think I can make a push to your branch / this PR, but here is a diff of the test that I would recommend adding:

diff --git a/__tests__/commands/install/bin-links.js b/__tests__/commands/install/bin-links.js
index 851b72f37..7fe74d2fd 100644
--- a/__tests__/commands/install/bin-links.js
+++ b/__tests__/commands/install/bin-links.js
@@ -175,6 +175,15 @@ test('can use link protocol to install a package that would not be found via nod
   });
 });

+test('empty bin string does not create a link', (): Promise<void> => {
+  return runInstall({binLinks: true}, 'install-empty-bin', async config => {
+    const binScripts = await fs.walk(path.join(config.cwd, 'node_modules', '.bin'));
+    expect(binScripts).toHaveLength(1);
+
+    expect(await linkAt(config, 'node_modules', '.bin', 'depB')).toEqual('../depB/depb.js');
+  });
+});
+
 describe('with nohoist', () => {
   // address https://github.com/yarnpkg/yarn/issues/5487
   test('nohoist bin should be linked to its own local module', (): Promise<void> => {
diff --git a/__tests__/fixtures/install/install-empty-bin/depA/package.json b/__tests__/fixtures/install/install-empty-bin/depA/package.json
new file mode 100644
index 000000000..f333589ee
--- /dev/null
+++ b/__tests__/fixtures/install/install-empty-bin/depA/package.json
@@ -0,0 +1,5 @@
+{
+  "name": "depA",
+  "version": "0.0.0",
+  "bin": ""
+}
diff --git a/__tests__/fixtures/install/install-empty-bin/depB/depb.js b/__tests__/fixtures/install/install-empty-bin/depB/depb.js
new file mode 100644
index 000000000..95230d617
--- /dev/null
+++ b/__tests__/fixtures/install/install-empty-bin/depB/depb.js
@@ -0,0 +1,2 @@
+#!/usr/bin/env node
+console.log('depB');
diff --git a/__tests__/fixtures/install/install-empty-bin/depB/package.json b/__tests__/fixtures/install/install-empty-bin/depB/package.json
new file mode 100644
index 000000000..c2832b922
--- /dev/null
+++ b/__tests__/fixtures/install/install-empty-bin/depB/package.json
@@ -0,0 +1,5 @@
+{
+  "name": "depB",
+  "version": "0.0.0",
+  "bin": "./depb.js"
+}
diff --git a/__tests__/fixtures/install/install-empty-bin/package.json b/__tests__/fixtures/install/install-empty-bin/package.json
new file mode 100644
index 000000000..7e59f4f34
--- /dev/null
+++ b/__tests__/fixtures/install/install-empty-bin/package.json
@@ -0,0 +1,9 @@
+{
+  "name": "test",
+  "version": "0.0.0",
+  "bin": "",
+  "dependencies": {
+    "depA": "file:depA",
+    "depB": "file:depB"
+  }
+}

This creates a root package install-empty-bin that has an empty bin, and adds a dependency depA that also has an empty bin, and verifies that neither is created.

[rhburrows]

Thanks! I wasn't sure if there was a better way to test the change and your patch does a much better job verifying the intent of this PR.

I applied your patch to the branch.

[Gudahtt]

This should have length 2 on Windows, as two files are created per binary on Windows (one Bash script for linux-like environments like Cygwin, and one .cmd Batch script). This is why the appveyor tests are failing.

[Gudahtt]

Great, that seemed to work. The remaining failing test is unrelated to this PR; that test is known to be flaky and will occasionally timeout.

[rally25rs]

I kicked off a new appveyor build. Hopefully it passes this time...