Skip to content

Commit 1b17064

Browse files
rally25rsbestander
rally25rs
authored and
bestander
committed
link bins of transitive deps to top level (#3310)
* #2874 link bins of transitive deps to top level * fixing bin link location checks for windows, where they are not symlinks * WIP: adding debug line to debug appveyor build * fix link path regex * fix Flow type * move install unit tests related to bin linking to own file * added tests for NPM bin link behavior mentioned in PR #2874 comments. Changed bin linking to match this behavior
1 parent fff1686 commit 1b17064

File tree

17 files changed

+1200
-29
lines changed

17 files changed

+1200
-29
lines changed

‎__tests__/commands/install/bin-links.js

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/* @flow */
2+
3+
import * as fs from '../../../src/util/fs.js';
4+
5+
jasmine.DEFAULT_TIMEOUT_INTERVAL = 150000;
6+
7+
const request = require('request');
8+
const path = require('path');
9+
import {runInstall} from '../_helpers.js';
10+
11+
async function linkAt(config, ...relativePath): Promise<string> {
12+
const joinedPath = path.join(config.cwd, ...relativePath);
13+
const stat = await fs.lstat(joinedPath);
14+
if (stat.isSymbolicLink()) {
15+
const linkPath = await fs.readlink(joinedPath);
16+
return linkPath;
17+
} else {
18+
const contents = await fs.readFile(joinedPath);
19+
return /node" +"\$basedir\/([^"]*\.js)"/.exec(contents)[1];
20+
}
21+
}
22+
23+
beforeEach(request.__resetAuthedRequests);
24+
afterEach(request.__resetAuthedRequests);
25+
26+
test('install should hoist nested bin scripts', (): Promise<void> => {
27+
return runInstall({binLinks: true}, 'install-nested-bin', async (config) => {
28+
const binScripts = await fs.walk(path.join(config.cwd, 'node_modules', '.bin'));
29+
// need to double the amount as windows makes 2 entries for each dependency
30+
// so for below, there would be an entry for eslint and eslint.cmd on win32
31+
const amount = process.platform === 'win32' ? 20 : 10;
32+
expect(binScripts).toHaveLength(amount);
33+
34+
expect(await linkAt(config, 'node_modules', '.bin', 'standard'))
35+
.toEqual('../standard/bin/cmd.js');
36+
expect(await linkAt(config, 'node_modules', '.bin', 'eslint'))
37+
.toEqual('../eslint/bin/eslint.js');
38+
});
39+
});
40+
41+
// dependency tree:
42+
// [email protected]
43+
// standard
44+
// standard -> [email protected]
45+
// result should be:
46+
// eslint 3.7.0 is linked in /.bin because it takes priority over the transitive 3.7.1
47+
// eslint 3.7.1 is linked in standard/node_modules/.bin
48+
test('direct dependency bin takes priority over transitive bin', (): Promise<void> => {
49+
return runInstall({binLinks: true}, 'install-duplicate-bin', async (config) => {
50+
expect(await linkAt(config, 'node_modules', '.bin', 'eslint'))
51+
.toEqual('../eslint/bin/eslint.js');
52+
expect(await linkAt(config, 'node_modules', 'standard', 'node_modules', '.bin', 'eslint'))
53+
.toEqual('../eslint/bin/eslint.js');
54+
});
55+
});
56+
57+
test.concurrent('install should respect --no-bin-links flag', (): Promise<void> => {
58+
return runInstall({binLinks: false}, 'install-nested-bin', async (config) => {
59+
const binExists = await fs.exists(path.join(config.cwd, 'node_modules', '.bin'));
60+
expect(binExists).toBeFalsy();
61+
});
62+
});
63+
64+
65+
// Scenario: Transitive dependency having version that is overridden by newer version as the direct dependency.
66+
// Behavior: [email protected] is symlinked in node_modeules/.bin
67+
// and [email protected] is symlinked to node_modules/sample-dep-eslint-3.10.1/node_modules/.bin
68+
test('newer transitive dep is overridden by older direct dep', (): Promise<void> => {
69+
return runInstall({binLinks: true}, 'install-bin-links-newer', async (config) => {
70+
expect(await linkAt(config, 'node_modules', '.bin', 'eslint'))
71+
.toEqual('../eslint/bin/eslint.js');
72+
expect(await linkAt(config, 'node_modules', 'sample-dep-eslint-3.10.1', 'node_modules', '.bin', 'eslint'))
73+
.toEqual('../eslint/bin/eslint.js');
74+
});
75+
});
76+
77+
// Scenario: Transitive dependency having version that is overridden by older version as the direct dependency.
78+
// Behavior: [email protected] is symlinked in node_modeules/.bin
79+
// and [email protected] is symlinked to node_modules/sample-dep-eslint-3.12.2/node_modules/.bin
80+
test('newer transitive dep is overridden by older direct dep', (): Promise<void> => {
81+
return runInstall({binLinks: true}, 'install-bin-links-older', async (config) => {
82+
expect(await linkAt(config, 'node_modules', '.bin', 'eslint'))
83+
.toEqual('../eslint/bin/eslint.js');
84+
expect(await linkAt(config, 'node_modules', 'sample-dep-eslint-3.12.2', 'node_modules', '.bin', 'eslint'))
85+
.toEqual('../eslint/bin/eslint.js');
86+
});
87+
});
88+
89+
// Scenario: Transitive dependency having version that is overridden by newer version as the dev dependency.
90+
// Behavior: [email protected] is symlinked in node_modeules/.bin
91+
// and [email protected] dependency for sample-dep-eslint-3.10.1 module is not linked.
92+
// SKIPPED because this seems like an NPM bug more than intentional design.
93+
// Why would it matter if the direct dependency is a dev one or not when linking the transient dep?
94+
test.skip('transitive dep is overridden by dev dep', (): Promise<void> => {
95+
return runInstall({binLinks: true}, 'install-bin-links-dev', async (config) => {
96+
expect(await linkAt(config, 'node_modules', '.bin', 'eslint'))
97+
.toEqual('../eslint/bin/eslint.js');
98+
expect(await linkAt(config, 'node_modules', 'sample-dep-eslint-3.10.1', 'node_modules', '.bin', 'eslint'))
99+
.not.toBeDefined();
100+
});
101+
});
102+
103+
// Scenario: Transitive dependency having version that is conflicting with another transitive dependency version.
104+
// Behavior: [email protected] is symlinked in node_modeules/.bin
105+
// and [email protected] is symlinked to node_modules/sample-dep-eslint-3.12.2/node_modules/.bin.
106+
// Here it seems like NPM add the modules in alphabatical order
107+
// and transitive deps of first dependency is installed at top level.
108+
test('first transient dep is installed when same level and reference count', (): Promise<void> => {
109+
return runInstall({binLinks: true}, 'install-bin-links-conflicting', async (config) => {
110+
expect(await linkAt(config, 'node_modules', '.bin', 'eslint'))
111+
.toEqual('../eslint/bin/eslint.js');
112+
expect(await linkAt(config, 'node_modules', 'sample-dep-eslint-3.12.2', 'node_modules', '.bin', 'eslint'))
113+
.toEqual('../eslint/bin/eslint.js');
114+
});
115+
});
116+
117+
// Scenario: Transitive dependency having version that is conflicting with another dev transitive dependency version.
118+
// Behavior: [email protected] is symlinked in node_modeules/.bin
119+
// and [email protected] is symlinked to node_modules/sample-dep-eslint-3.12.2/node_modules/.bin.
120+
// Whether the dependencies are devDependencies or not does not seem to matter to NPM.
121+
test('first dep is installed when same level and reference count and one is a dev dep', (): Promise<void> => {
122+
return runInstall({binLinks: true}, 'install-bin-links-conflicting-dev', async (config) => {
123+
expect(await linkAt(config, 'node_modules', '.bin', 'eslint'))
124+
.toEqual('../eslint/bin/eslint.js');
125+
expect(await linkAt(config, 'node_modules', 'sample-dep-eslint-3.12.2', 'node_modules', '.bin', 'eslint'))
126+
.toEqual('../eslint/bin/eslint.js');
127+
});
128+
});

‎__tests__/commands/install/integration.js

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -526,25 +526,6 @@ test.concurrent(
526526
},
527527
);
528528

529-
// disabled to resolve https://github.com/yarnpkg/yarn/pull/1210
530-
test.skip('install should hoist nested bin scripts', (): Promise<void> => {
531-
return runInstall({binLinks: true}, 'install-nested-bin', async (config) => {
532-
const binScripts = await fs.walk(path.join(config.cwd, 'node_modules', '.bin'));
533-
// need to double the amount as windows makes 2 entries for each dependency
534-
// so for below, there would be an entry for eslint and eslint.cmd on win32
535-
const amount = process.platform === 'win32' ? 20 : 10;
536-
expect(binScripts).toHaveLength(amount);
537-
expect(binScripts.findIndex((f) => f.basename === 'eslint')).toBeGreaterThanOrEqual(0);
538-
});
539-
});
540-
541-
test.concurrent('install should respect --no-bin-links flag', (): Promise<void> => {
542-
return runInstall({binLinks: false}, 'install-nested-bin', async (config) => {
543-
const binExists = await fs.exists(path.join(config.cwd, 'node_modules', '.bin'));
544-
expect(binExists).toBeFalsy();
545-
});
546-
});
547-
548529
test.concurrent('install should update a dependency to yarn and mirror (PR import scenario 2)', (): Promise<void> => {
549530
// [email protected] is gets updated to [email protected] via
550531
// a change in package.json,

‎__tests__/fixtures/install/install-bin-links-conflicting-dev/package.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"sample-dep-eslint-3.10.1": "file:sample-dep-eslint-3.10.1"
5+
},
6+
"devDependencies": {
7+
"sample-dep-eslint-3.12.2": "file:sample-dep-eslint-3.12.2"
8+
}
9+
}

‎__tests__/fixtures/install/install-bin-links-conflicting-dev/sample-dep-eslint-3.10.1/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.10.1"
5+
}
6+
}

‎__tests__/fixtures/install/install-bin-links-conflicting-dev/sample-dep-eslint-3.12.2/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.12.2"
5+
}
6+
}

‎__tests__/fixtures/install/install-bin-links-conflicting/package.json

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"sample-dep-eslint-3.12.2": "file:sample-dep-eslint-3.12.2",
5+
"sample-dep-eslint-3.10.1": "file:sample-dep-eslint-3.10.1"
6+
}
7+
}

‎__tests__/fixtures/install/install-bin-links-conflicting/sample-dep-eslint-3.10.1/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.10.1"
5+
}
6+
}

‎__tests__/fixtures/install/install-bin-links-conflicting/sample-dep-eslint-3.12.2/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.12.2"
5+
}
6+
}

‎__tests__/fixtures/install/install-bin-links-dev/package.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"sample-dep-eslint-3.10.1": "file:sample-dep-eslint-3.10.1"
5+
},
6+
"devDependencies": {
7+
"eslint": "3.12.2"
8+
}
9+
}

‎__tests__/fixtures/install/install-bin-links-dev/sample-dep-eslint-3.10.1/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.10.1"
5+
}
6+
}

‎__tests__/fixtures/install/install-bin-links-newer/package.json

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.12.2",
5+
"sample-dep-eslint-3.10.1": "file:sample-dep-eslint-3.10.1"
6+
}
7+
}

‎__tests__/fixtures/install/install-bin-links-newer/sample-dep-eslint-3.10.1/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.10.1"
5+
}
6+
}

‎__tests__/fixtures/install/install-bin-links-older/package.json

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.10.1",
5+
"sample-dep-eslint-3.12.2": "file:sample-dep-eslint-3.12.2"
6+
}
7+
}

‎__tests__/fixtures/install/install-bin-links-older/sample-dep-eslint-3.12.2/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"version": "0.0.1",
3+
"dependencies": {
4+
"eslint": "3.12.2"
5+
}
6+
}

‎__tests__/fixtures/install/install-duplicate-bin/package.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"dependencies": {
3+
"eslint": "3.7.0",
4+
"standard": "8.4.0"
5+
}
6+
}

0 commit comments

Comments
 (0)