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

BYK · 2017-09-15T17:58:52Z

Summary

Fixes #4446, fixes #4433, fixes #2688, fixes #2387. Follow up to #3803. The fix in #3893 was
too aggressive, allowing only top-level dependencies to be used in
peer dependency resolution which was incorrect. This patch allows
resolving peer dependencies from the same or higher levels in the
dependency tree.

Test plan

Additional unit and integration tests.

**Summary** Fixes #4446, fixes #4433. Follow up to #3893. The fix in #3893 was too aggressive, allowing only top-level dependencies to be used in peer dependency resolution which was incorrect. This patch allows resolving peer dependencies from the same or higher levels in the dependency tree. **Test plan** Additional unit and integration tests.

BYK · 2017-09-15T17:59:39Z

I've tested this with the mentioned issues and it seems to fix them both. I'll also test tihs with other similar issues and create-react-app but it should be fine. I'll also add tests. This is to get initial feedback to confirm my approach is sound.

BYK · 2017-09-15T18:00:16Z

package.json

@@ -40,7 +40,7 @@
    "request": "^2.81.0",
    "request-capture-har": "^1.2.2",
    "rimraf": "^2.5.0",
-    "semver": "^5.1.0",
+    "semver": "^5.4.1",


This update is not required so I can revert if you want.

buildsize · 2017-09-15T18:13:54Z

This change will increase the build size from 9.69 MB to 9.69 MB, an increase of 1.55 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	834.53 KB	834.59 KB	67 bytes (0%)
yarn-[version].js	3.69 MB	3.69 MB	656 bytes (0%)
yarn-legacy-[version].js	3.74 MB	3.74 MB	656 bytes (0%)
yarn-v[version].tar.gz	838.58 KB	838.7 KB	124 bytes (0%)
yarn_[version]all.deb	634.79 KB	634.88 KB	88 bytes (0%)

kaylie-alexa · 2017-09-15T18:25:58Z

src/package-linker.js

-        const {root, version} = pkg._reference || {};
-        return root && this._satisfiesPeerDependency(range, version);
+        const {level, version} = pkg._reference || {};
+        return level <= ref.level && this._satisfiesPeerDependency(range, version);


I think we should be prioritizing some links over others, i.e. top level dependency should satisfy its dependencies from root (same level). I'm not sure whether a nested dependency should satisfy it from root first or sibling first. But maybe instead of find, we need to add it to a collection and then pick out either the lowest level or closest level match. WDYT?

Good thinking! I think it should resolve to the closest. I'll update the code.

BYK · 2017-09-15T19:53:42Z

@kaylieEB updated per your suggestion. Also tested with yarn create react-app and all the peer dependency warnings are gone now 🎉

kaylie-alexa · 2017-09-15T20:04:03Z

src/package-linker.js

+        if (resolvedPeerPkgPattern) {
+          ref.addDependencies(resolvedPeerPkgPattern);
+        } else {
+          const depError = incorrectPeer ? 'incorrectPeer' : 'unmetPeer';


you can define let depError = 'unmetPeer' instead of let incorrectPeer = false and set it to 'incorrectPeer' above right?

Hahaha, right! Stupid me. Will do that too. Rest looks fine? I'll add tests soon.

yup the rest looks good!

BYK · 2017-09-15T22:31:09Z

@kaylieEB one more review, for tests please! :)

kaylie-alexa

minor comment, looks good!

kaylie-alexa · 2017-09-15T22:59:33Z

src/reporters/lang/en.js

@@ -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.',


saw this earlier, nice touch!

kaylie-alexa · 2017-09-15T23:04:30Z

__tests__/integration.js

@@ -51,6 +59,7 @@ addTest('https://git@github.com/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


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

Ahh, another good insight. Update coming.

paul-court · 2017-09-18T11:05:11Z

If this is now fixed, which version of yarn should we see it in 1.0.3?

BYK · 2017-09-18T12:40:04Z

@Gargoyle 1.1.0, which is around the corner. It is a minor version upgrade instead of a patch release since it will have some features like performance improvements.

BYK · 2017-09-18T12:40:28Z

it will have some features like performance improvements.

To be specific, not because of this PR but others :)

**Summary** This is a follow up to #4484 and #4478 which improves the code around those areas a bit and removes a now-unnecessary `while` loop. **Test plan** Existing tests should pass.

With yarn@1.1.0 `aio/` and some guide examples fail to build. The failures seem to be caused by something (possibly `@ngtools/webpack`, which is a dependency of `@angular/cli`) which peer-depends on TypeScript (TS@^2.0.2), getting linked to a higher version (TS@2.4.1) with yarn@1.1.0, which is stricter than the 2.3.2 version we use on `aio/` (and which that something gets linked to with yarn@1.0.2). [This change][1] is what is causing this difference in behavior. Downgrading yarn to v1.0.2 (same we use on master) for now. [1]: yarnpkg/yarn#4478

* chore(resolver): Minor improvements in resolver code and tests **Summary** This is a follow up to #4484 and #4478 which improves the code around those areas a bit and removes a now-unnecessary `while` loop. **Test plan** Existing tests should pass. * Fix logic

…arnpkg#4478) **Summary** Fixes yarnpkg#4446, fixes yarnpkg#4433, fixes yarnpkg#2688, fixes yarnpkg#2387. Follow up to yarnpkg#3803. The fix in yarnpkg#3893 was too aggressive, allowing only top-level dependencies to be used in peer dependency resolution which was incorrect. This patch allows resolving peer dependencies from the same or higher levels in the dependency tree. **Test plan** Additional unit and integration tests.

…kg#4644) * chore(resolver): Minor improvements in resolver code and tests **Summary** This is a follow up to yarnpkg#4484 and yarnpkg#4478 which improves the code around those areas a bit and removes a now-unnecessary `while` loop. **Test plan** Existing tests should pass. * Fix logic

BYK requested review from bestander and kaylie-alexa September 15, 2017 17:58

BYK commented Sep 15, 2017

View reviewed changes

kaylie-alexa reviewed Sep 15, 2017

View reviewed changes

BYK added 2 commits September 15, 2017 20:06

Revert unnecessary semver upgrade

e79d0f4

Always select the closest peer dependency

e04d5d0

kaylie-alexa approved these changes Sep 15, 2017

View reviewed changes

BYK added 4 commits September 15, 2017 21:56

Merge branch 'master' into fix-peer-warnings

bd77957

even simpler code

86a3d26

Add integration test

dd3a380

Add simpler unit test

c384f4b

Fix lint & moar concurrency

73ab3fd

kaylie-alexa reviewed Sep 15, 2017

View reviewed changes

Fix react-scripts version in tests

a443166

kaylie-alexa approved these changes Sep 15, 2017

View reviewed changes

This was referenced Sep 15, 2017

"Unmet peer dependency" with peer dependency already installed as transitive dep #2688

Closed

Peer Dependency installed incorrectly #1661

Closed

Disable react-scripts test on Node 4

fc87e2b

BYK mentioned this pull request Sep 16, 2017

Cascading peer dependencies does not work and probably should? #4432

Closed

Increase timeout interval for integration tests

08b1a4f

BYK merged commit 96c215c into master Sep 16, 2017

BYK deleted the fix-peer-warnings branch September 16, 2017 10:16

tech4him1 mentioned this pull request Sep 20, 2017

Update Dependencies decaporg/decap-cms#524

Closed

ocombe mentioned this pull request Sep 25, 2017

build: replace npm with yarn - part 2 angular/angular#19366

Closed

This was referenced Sep 25, 2017

Yarn ignores bundledDependencies and copies all node_modules for file: packages #4532

Closed

Theia builds are weirdly broken with npm 5.3.0 + yarn 1.1.0 #4539

Closed

swim mentioned this pull request Sep 29, 2017

Gatsby breaks with yarn 1.x gatsbyjs/gatsby#2244

Closed

BYK mentioned this pull request Oct 5, 2017

chore(resolver): Minor improvements in resolver code and tests #4644

Merged

gkalpak mentioned this pull request Oct 5, 2017

build(aio): remove unnecessary -- from yarn commands angular/angular#19565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BYK commented Sep 15, 2017 •

edited

Loading

BYK commented Sep 15, 2017

BYK Sep 15, 2017

buildsize bot commented Sep 15, 2017 •

edited

Loading

kaylie-alexa Sep 15, 2017

BYK Sep 15, 2017

BYK commented Sep 15, 2017

kaylie-alexa Sep 15, 2017

BYK Sep 15, 2017

kaylie-alexa Sep 15, 2017

BYK commented Sep 15, 2017

kaylie-alexa left a comment

kaylie-alexa Sep 15, 2017

kaylie-alexa Sep 15, 2017

BYK Sep 15, 2017

paul-court commented Sep 18, 2017

BYK commented Sep 18, 2017

BYK commented Sep 18, 2017

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

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

Conversation

BYK commented Sep 15, 2017 • edited Loading

BYK commented Sep 15, 2017

Choose a reason for hiding this comment

buildsize bot commented Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Sep 15, 2017

kaylie-alexa left a 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

paul-court commented Sep 18, 2017

BYK commented Sep 18, 2017

BYK commented Sep 18, 2017

BYK commented Sep 15, 2017 •

edited

Loading

buildsize bot commented Sep 15, 2017 •

edited

Loading