Skip to content

Commit 22ff4f2

Browse files
authored
Fix: invalid linking in workspaces for unused packages (#4129)
**Summary** Fixes #4106. When a local package is listed under `workspaces` but that specific version is never used by anything, `yarn` still tries to install its bin links because it is listed as a dependency of the virtual workspace aggregator (but never get installed). This patch makes sure we have a location inside `node_modules` when trying to create bin links. It also fixes where we don't pass the proper location for workspaces root which may result in unexpected behavior under certain circumstances. **Test plan** Added a new test case which fails on master and passes with the fix. Manual: ``` git clone [email protected]:babel/babel.git yarn ``` Fails before the patch with latest nightly and passes after the patch.
1 parent f072e96 commit 22ff4f2

File tree

13 files changed

+129
-13
lines changed

13 files changed

+129
-13
lines changed

__tests__/commands/install/workspaces-install.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,16 @@ test.concurrent(
132132
},
133133
);
134134

135+
test.concurrent('install should not link a workspace if the version is not compatible', (): Promise<void> => {
136+
return runInstall({binLinks: true}, 'workspaces-install-link-invalid', async (config): Promise<void> => {
137+
// node_modules/left-pad - from npm
138+
const packageFile = await fs.readFile(path.join(config.cwd, 'node_modules', 'left-pad', 'package.json'));
139+
const readme = await fs.readFile(path.join(config.cwd, 'node_modules', 'left-pad', 'README.md'));
140+
expect(JSON.parse(packageFile).version).not.toBe('1.1.2');
141+
expect(readme.split('\n')[0]).not.toEqual('WORKSPACES ROCK!');
142+
});
143+
});
144+
135145
test.concurrent('install should prioritize non workspace dependency at root over the workspace symlink', (): Promise<
136146
void,
137147
> => {
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
workspaces-experimental true
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
{
2+
"name": "my-project",
3+
"private": true,
4+
"devDependencies": {
5+
"left-pad": "0.0.9"
6+
},
7+
"workspaces": [
8+
"packages/*"
9+
]
10+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
WORKSPACES ROCK!
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#!/usr/bin/env node
2+
3+
var leftPad = require('./');
4+
5+
console.log(leftPad(...process.argv.slice(0, 2)));
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
'use strict';
2+
module.exports = leftPad;
3+
4+
var cache = ['', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '];
5+
6+
function leftPad(str, len, ch) {
7+
// convert `str` to `string`
8+
str = str + '';
9+
// `len` is the `pad`'s length now
10+
len = len - str.length;
11+
// doesn't need to pad
12+
if (len <= 0) return str;
13+
// `ch` defaults to `' '`
14+
if (!ch && ch !== 0) ch = ' ';
15+
// convert `ch` to `string`
16+
ch = ch + '';
17+
// cache common use cases
18+
if (ch === ' ' && len < 10) return cache[len] + str;
19+
// `pad` starts with an empty string
20+
var pad = '';
21+
// loop
22+
while (true) {
23+
// add `ch` to `pad` if `len` is odd
24+
if (len & 1) pad += ch;
25+
// devide `len` by 2, ditch the fraction
26+
len >>= 1;
27+
// "double" the `ch` so this operation count grows logarithmically on `len`
28+
// each time `ch` is "doubled", the `len` would need to be "doubled" too
29+
// similar to finding a value in binary search tree, hence O(log(n))
30+
if (len) ch += ch;
31+
else
32+
// `len` is 0, exit the loop
33+
break;
34+
}
35+
// pad `str`!
36+
return pad + str;
37+
}
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
{
2+
"name": "left-pad",
3+
"version": "1.1.2",
4+
"description": "String left pad",
5+
"main": "index.js",
6+
"bin": "bin.js",
7+
"scripts": {
8+
"test": "node test",
9+
"bench": "node perf/perf.js"
10+
},
11+
"keywords": [
12+
"leftpad",
13+
"left",
14+
"pad",
15+
"padding",
16+
"string",
17+
"repeat"
18+
],
19+
"repository": {
20+
"url": "[email protected]:stevemao/left-pad.git",
21+
"type": "git"
22+
},
23+
"author": "azer",
24+
"maintainers": [
25+
{
26+
"name": "Cameron Westland",
27+
"email": "[email protected]"
28+
}
29+
],
30+
"license": "WTFPL"
31+
}
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
var leftPad = require('./');
2+
var test = require('tape');
3+
4+
test('left pad', function(assert) {
5+
assert.plan(7);
6+
assert.strictEqual(leftPad('foo', 5), ' foo');
7+
assert.strictEqual(leftPad('foobar', 6), 'foobar');
8+
assert.strictEqual(leftPad(1, 2, 0), '01');
9+
assert.strictEqual(leftPad(1, 2, '-'), '-1');
10+
assert.strictEqual(leftPad('foo', 2, ' '), 'foo');
11+
assert.strictEqual(leftPad('foo', -1, ' '), 'foo');
12+
assert.strictEqual(leftPad('foo', 7, 1), '1111foo');
13+
});
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#!/usr/bin/env node
2+
3+
var leftPad = require('./');
4+
5+
console.log(leftPad(...process.argv.slice(0, 2)));

src/cli/commands/install.js

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -275,15 +275,16 @@ export class Install {
275275
pushDeps('optionalDependencies', projectManifestJson, {hint: 'optional', optional: true}, true);
276276

277277
if (this.config.workspacesEnabled) {
278-
const workspaces = await this.config.resolveWorkspaces(path.dirname(loc), projectManifestJson);
278+
const workspacesRoot = path.dirname(loc);
279+
const workspaces = await this.config.resolveWorkspaces(workspacesRoot, projectManifestJson);
279280
workspaceLayout = new WorkspaceLayout(workspaces, this.config);
280281
// add virtual manifest that depends on all workspaces, this way package hoisters and resolvers will work fine
281282
const virtualDependencyManifest: Manifest = {
282283
_uid: '',
283284
name: `workspace-aggregator-${uuid.v4()}`,
284285
version: '1.0.0',
285286
_registry: 'npm',
286-
_loc: '.',
287+
_loc: workspacesRoot,
287288
dependencies: {},
288289
};
289290
workspaceLayout.virtualManifestName = virtualDependencyManifest.name;
@@ -293,7 +294,7 @@ export class Install {
293294
}
294295
const virtualDep = {};
295296
virtualDep[virtualDependencyManifest.name] = virtualDependencyManifest.version;
296-
workspaces[virtualDependencyManifest.name] = {loc: '', manifest: virtualDependencyManifest};
297+
workspaces[virtualDependencyManifest.name] = {loc: workspacesRoot, manifest: virtualDependencyManifest};
297298

298299
pushDeps('workspaces', {workspaces: virtualDep}, {hint: 'workspaces', optional: false}, true);
299300
}

0 commit comments

Comments
 (0)