fix(resolver): Fix incorrect peer dependency res. from different trees #4687

BYK · 2017-10-11T16:38:27Z

Summary

Fixes #4539. Yarn was resolving peer dependencies from the closest level where the peer dependency was requested
but it was not checking if the peer dependency was in the same subtree. This was causing incorrect
peer dependency resolutions and package duplication when an unrelated subtree has a depedency
satisfying the required peer dependency at the same tree level.

Test plan

Added new install integration test that fails without the fix.

…t trees **Summary** Fixes #4539. Yarn was resolving peer dependencies from the closest level where the peer dependency was requested but it was not checking if the peer dependency was in the same subtree. This was causing incorrect peer dependency resolutions and package duplication when an unrelated subtree has a depedency satisfying the required peer dependency at the same tree level. **Test plan** Manually verified. Autmated tests coming.

arcanis · 2017-10-11T16:43:23Z

src/package-linker.js

@@ -446,6 +446,24 @@ export default class PackageLinker {
      }
      const ref = pkg._reference;
      invariant(ref, 'Package reference is missing');
+      // TODO: We are taking the "shortest" ref tree but there may be multiple ref trees with the same length
+      const refTree = ref.requests.map(req => req.parentNames).sort((arr1, arr2) => arr1.length - arr2.length)[0];
+      const refTreeLen = refTree.length;


I'd just use refTree.length, it looks like a premature optim (same for pkgParentsLen)

It was mostly for ease of reading for myself but sure.

Oh well, these were easier on the eyes in a previous version of the code. Now they are useless as you pointed out.

arcanis · 2017-10-11T16:46:26Z

src/package-linker.js

@@ -446,6 +446,24 @@ export default class PackageLinker {
      }
      const ref = pkg._reference;
      invariant(ref, 'Package reference is missing');
+      // TODO: We are taking the "shortest" ref tree but there may be multiple ref trees with the same length


Is it a problem if we choose one of those at random?

I honestly don't know, hence the TODO.

buildsize · 2017-10-11T16:48:13Z

This change will decrease the build size from 9.93 MB to 9.93 MB, a decrease of 6.68 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	859.11 KB	858.49 KB	-631 bytes (0%)
yarn-[version].js	3.78 MB	3.78 MB	-2.53 KB (0%)
yarn-legacy-[version].js	3.83 MB	3.83 MB	-2.63 KB (0%)
yarn-v[version].tar.gz	864.78 KB	864.25 KB	-551 bytes (0%)
yarn_[version]all.deb	653.78 KB	653.41 KB	-374 bytes (0%)

kaylie-alexa · 2017-10-11T17:54:08Z

src/package-linker.js

-          const levelDistance = ref.level - peerPkgRef.level;
-          if (levelDistance >= 0 && levelDistance < resolvedLevelDistance) {
+          const levelDistance = getLevelDistance(peerPkgRef);
+          if (isFinite(levelDistance) && levelDistance < resolvedLevelDistance) {


Is it possible to not set minDistance to Infinity and just check for existence now?

I wanted that too but then I'd need to add special casing into the getLevelDistance code above due to the distance >= 0 && distance < minDistance checks. Is it too confusing right now? :(

Ah I see, hmm can't think of an alternative atm. Thanks for explaining. :)

kaylie-alexa · 2017-10-11T17:56:05Z

src/package-linker.js

+          const pkgParentsLen = pkgParents.length;
+          const distance = refTreeLen - pkgParentsLen;
+
+          if (distance >= 0 && distance < minDistance && pkgParents.every((name, idx) => name === refTree[idx])) {


Might be more less readable, but more efficient to use !pakgParents.some instead?

kaylie-alexa · 2017-10-11T18:00:07Z

__tests__/commands/install/integration.js

+test.concurrent('install should resolve peer dependencies from same subtrees', async () => {
+  await runInstall({}, 'peer-dep-same-subtree', async (config): Promise<void> => {
+    expect(JSON.parse(await fs.readFile(`${config.cwd}/node_modules/d/node_modules/a/package.json`)).version).toEqual(
+      '1.1.0',


shouldn't this equal 1.0.0 since d points to file:../a-1.0.0 as a dependency? and the top level version of a equal 1.1.0?

Yup, that's why the test was failing :)

kaylie-alexa · 2017-10-11T18:01:57Z

src/package-linker.js

+        for (const req of pkgRef.requests) {
+          const distance = refTree.length - req.parentNames.length;
+
+          if (distance >= 0 && distance < minDistance && req.parentNames.every((name, idx) => name === refTree[idx])) {


It might be slightly less readable but more efficient to use !some to exit early if they don't match

I think both every and some do early exits so don't think it would change perf.

ugh, I keep forgetting that sorry!

yarnpkg#4687) **Summary** Fixes yarnpkg#4539. Yarn was resolving peer dependencies from the closest level where the peer dependency was requested but it was not checking if the peer dependency was in the same subtree. This was causing incorrect peer dependency resolutions and package duplication when an unrelated subtree has a depedency satisfying the required peer dependency at the same tree level. **Test plan** Added new install integration test that fails without the fix.

BYK assigned arcanis Oct 11, 2017

BYK requested review from arcanis and kaylie-alexa October 11, 2017 16:38

arcanis reviewed Oct 11, 2017

View reviewed changes

BYK added 2 commits October 11, 2017 17:58

Add test

a2d11ea

Better code

bba38c4

arcanis approved these changes Oct 11, 2017

View reviewed changes

kaylie-alexa reviewed Oct 11, 2017

View reviewed changes

fix test

9cf5287

BYK merged commit f60269f into master Oct 11, 2017

BYK deleted the peer-dep-tree-hug branch October 11, 2017 22:48

hexa00 mentioned this pull request Oct 12, 2017

Since 1.2.1: error Could not open cafile: EISDIR: illegal operation on a directory, read #4695

Closed

FDiskas mentioned this pull request Oct 12, 2017

PearDependency issue #4698

Open

BYK mentioned this pull request Oct 19, 2017

Workspaces complain about missing hoisted peer dependencies #4743

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resolver): Fix incorrect peer dependency res. from different trees #4687

fix(resolver): Fix incorrect peer dependency res. from different trees #4687

BYK commented Oct 11, 2017 •

edited

Loading

arcanis Oct 11, 2017

BYK Oct 11, 2017

BYK Oct 11, 2017

arcanis Oct 11, 2017

BYK Oct 11, 2017

buildsize bot commented Oct 11, 2017 •

edited

Loading

kaylie-alexa Oct 11, 2017

BYK Oct 11, 2017

kaylie-alexa Oct 11, 2017

kaylie-alexa Oct 11, 2017

kaylie-alexa Oct 11, 2017

BYK Oct 11, 2017

kaylie-alexa Oct 11, 2017

BYK Oct 11, 2017

kaylie-alexa Oct 11, 2017

fix(resolver): Fix incorrect peer dependency res. from different trees #4687

fix(resolver): Fix incorrect peer dependency res. from different trees #4687

Conversation

BYK commented Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buildsize bot commented Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Oct 11, 2017 •

edited

Loading

buildsize bot commented Oct 11, 2017 •

edited

Loading