Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Resolve peerDependencies from all higher levels, not just root #4478

Merged
merged 11 commits into from
Sep 16, 2017
29 changes: 21 additions & 8 deletions __tests__/commands/add.js
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ test.concurrent('warns when peer dependency is incorrect during add', (): Promis
);
});

test.concurrent('should only refer to root to satisfy peer dependency', (): Promise<void> => {
test.concurrent('should only refer to higher levels to satisfy peer dependency', (): Promise<void> => {
return buildRun(
reporters.BufferReporter,
fixturesLoc,
Expand All @@ -834,20 +834,33 @@ test.concurrent('should only refer to root to satisfy peer dependency', (): Prom
await add.init();

const output = reporter.getBuffer();
const warnings = output.filter(entry => entry.type === 'warning');

expect(
warnings.some(warning => {
return warning.data.toString().toLowerCase().indexOf('incorrect peer') > -1;
}),
).toEqual(true);
const warnings = output.filter(entry => entry.type === 'warning').map(entry => entry.data);
expect(warnings).toEqual(expect.arrayContaining([expect.stringContaining('incorrect peer')]));
},
['file:c'],
{},
'add-with-multiple-versions-of-peer-dependency',
);
});

test.concurrent('should refer to deeper dependencies to satisfy peer dependency', (): Promise<void> => {
return buildRun(
reporters.BufferReporter,
fixturesLoc,
async (args, flags, config, reporter, lockfile): Promise<void> => {
const add = new Add(args, flags, config, reporter, lockfile);
await add.init();

const output = reporter.getBuffer();
const warnings = output.filter(entry => entry.type === 'warning').map(entry => entry.data);
expect(warnings).not.toEqual(expect.arrayContaining([expect.stringContaining('peer')]));
},
[],
{},
'add-with-deep-peer-dependencies',
);
});

test.concurrent('should retain build artifacts after add when missing integrity file', (): Promise<void> => {
return buildRun(
reporters.BufferReporter,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "a",
"version": "0.0.0",
"dependencies": {
"b": "file:../b",
"c": "file:../c"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "b",
"version": "1.0.0",
"peerDependencies": {
"c": "1.0.0"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "c",
"version": "1.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "d",
"version": "3.0.0",
"dependencies": {
"a": "file:a"
}
}
21 changes: 15 additions & 6 deletions __tests__/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;

const path = require('path');

function addTest(pattern) {
// concurrently network requests tend to stall
test(`yarn add ${pattern}`, async () => {
function addTest(pattern, {strict} = {strict: false}) {
test.concurrent(`yarn add ${pattern}`, async () => {
const cwd = await makeTemp();
const cacheFolder = path.join(cwd, 'cache');

Expand All @@ -22,9 +21,18 @@ function addTest(pattern) {

const options = {cwd};

await fs.writeFile(path.join(cwd, 'package.json'), JSON.stringify({name: 'test'}));
await fs.writeFile(
path.join(cwd, 'package.json'),
JSON.stringify({
name: 'test',
license: 'MIT',
}),
);

await execa(command, ['add', pattern].concat(args), options);
const result = await execa(command, ['add', pattern].concat(args), options);
if (strict) {
expect(result.stderr).not.toMatch(/^warning /gm);
}

await fs.unlink(cwd);
});
Expand All @@ -41,7 +49,7 @@ function addTest(pattern) {
// path.join(folder, constants.METADATA_FILENAME),
// '{"remote": {"hash": "cafebabecafebabecafebabecafebabecafebabe"}}',
// );
// await fs.writeFile(path.join(folder, 'package.json'), '{"name": "@foo/bar", "version": "1.2.3"}');
// await fs.writeFile(path.join(foldresolve gitlab:leanlabsio/kanbaner, 'package.json'), '{"name": "@foo/bar", "version": "1.2.3"}');
// },
// true,
// ); // offline npm scoped package
Expand All @@ -51,6 +59,7 @@ addTest('https://[email protected]/stevemao/left-pad.git'); // git url, with userna
addTest('https://github.com/yarnpkg/yarn/releases/download/v0.18.1/yarn-v0.18.1.tar.gz'); // tarball
addTest('https://github.com/bestander/chrome-app-livereload.git'); // no package.json
addTest('bestander/chrome-app-livereload'); // no package.json, github, tarball
addTest('react-scripts', {strict: true}); // many peer dependencies, there shouldn't be any peerDep warnings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should fix this version in case the react-scripts package does get updated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, another good insight. Update coming.


const MIN_PORT_NUM = 56000;
const MAX_PORT_NUM = 65535;
Expand Down
3 changes: 1 addition & 2 deletions __tests__/package-resolver.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ const path = require('path');
const cachePathRe = /-\d+\.\d+\.\d+-[\dabcdef]{40}$/;

function addTest(pattern, registry = 'npm', init: ?(cacheFolder: string) => Promise<any>, offline = false) {
// concurrently network requests tend to stall
test(`${offline ? 'offline ' : ''}resolve ${pattern}`, async () => {
test.concurrent(`${offline ? 'offline ' : ''}resolve ${pattern}`, async () => {
const lockfile = new Lockfile();
const reporter = new reporters.NoopReporter({});

Expand Down
69 changes: 42 additions & 27 deletions src/package-linker.js
Original file line number Diff line number Diff line change
Expand Up @@ -432,34 +432,49 @@ export default class PackageLinker {

resolvePeerModules() {
for (const pkg of this.resolver.getManifests()) {
this._resolvePeerModules(pkg);
}
}

_resolvePeerModules(pkg: Manifest) {
const peerDeps = pkg.peerDependencies;
if (!peerDeps) {
return;
}

const ref = pkg._reference;
invariant(ref, 'Package reference is missing');
const peerDeps = pkg.peerDependencies;
if (!peerDeps) {
continue;
}
const ref = pkg._reference;
invariant(ref, 'Package reference is missing');

for (const peerDepName in peerDeps) {
const range = peerDeps[peerDepName];
const peerPkgs = this.resolver.getAllInfoForPackageName(peerDepName);

let peerError = 'unmetPeer';
let resolvedLevelDistance = Infinity;
let resolvedPeerPkgPattern;
for (const peerPkg of peerPkgs) {
const peerPkgRef = peerPkg._reference;
if (!(peerPkgRef && peerPkgRef.patterns)) {
continue;
}
const levelDistance = ref.level - peerPkgRef.level;
if (levelDistance >= 0 && levelDistance < resolvedLevelDistance) {
if (this._satisfiesPeerDependency(range, peerPkgRef.version)) {
resolvedLevelDistance = levelDistance;
resolvedPeerPkgPattern = peerPkgRef.patterns;
this.reporter.verbose(
this.reporter.lang(
'selectedPeer',
`${pkg.name}@${pkg.version}`,
`${peerDepName}@${range}`,
peerPkgRef.level,
),
);
} else {
peerError = 'incorrectPeer';
}
}
}

for (const name in peerDeps) {
const range = peerDeps[name];
const pkgs = this.resolver.getAllInfoForPackageName(name);
const found = pkgs.find(pkg => {
const {root, version} = pkg._reference || {};
return root && this._satisfiesPeerDependency(range, version);
});
const foundPattern = found && found._reference && found._reference.patterns;

if (foundPattern) {
ref.addDependencies(foundPattern);
} else {
const depError = pkgs.length > 0 ? 'incorrectPeer' : 'unmetPeer';
const [pkgHuman, depHuman] = [`${pkg.name}@${pkg.version}`, `${name}@${range}`];
this.reporter.warn(this.reporter.lang(depError, pkgHuman, depHuman));
if (resolvedPeerPkgPattern) {
ref.addDependencies(resolvedPeerPkgPattern);
} else {
this.reporter.warn(this.reporter.lang(peerError, `${pkg.name}@${pkg.version}`, `${peerDepName}@${range}`));
}
}
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/package-reference.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export default class PackageReference {
this.permissions = {};
this.patterns = [];
this.optional = null;
this.root = false;
this.level = Infinity;
this.ignore = false;
this.incompatible = false;
this.fresh = false;
Expand All @@ -39,7 +39,7 @@ export default class PackageReference {
lockfile: Lockfile;
config: Config;

root: boolean;
level: number;
name: string;
version: string;
uid: string;
Expand All @@ -66,9 +66,13 @@ export default class PackageReference {
addRequest(request: PackageRequest) {
this.requests.push(request);

if (!request.parentRequest) {
this.root = true;
let requestLevel = -1;
let currentRequest = request;
while (currentRequest && requestLevel < this.level) {
requestLevel += 1;
currentRequest = currentRequest.parentRequest;
}
this.level = requestLevel;
}

prune() {
Expand Down
1 change: 1 addition & 0 deletions src/reporters/lang/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ const messages = {

unmetPeer: '$0 has unmet peer dependency $1.',
incorrectPeer: '$0 has incorrect peer dependency $1.',
selectedPeer: 'Selecting $1 at level $2 as the peer dependency of $0.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw this earlier, nice touch!

missingBundledDependency: '$0 is missing a bundled dependency $1. This should be reported to the package maintainer.',

savedNewDependency: 'Saved 1 new dependency.',
Expand Down