Fix: 'yarn --flat' unbound transitive deps shouldn't conflict with top level deps #4488

mxmul · 2017-09-17T06:21:54Z

Summary

Fixes #3780, and makes the failing test from #3779 passing.

As a final step of package resolution, for each dependency we check whether any version satisfies all resolved version ranges.

Test plan

Fixes an existing (failing) test: "unbound transitive dependencies should not conflict with top level dependency"

BYK

Looks good but I have some suggestions.

Also, I'm wondering if we can put this logic into .find() itself, potentially avoiding some more network requests and the extra loop over dependencies at the end.

BYK · 2017-09-18T11:26:57Z

src/package-resolver.js

+    // https://github.com/yarnpkg/yarn/issues/79
+    const anyLocked = patterns.some(pattern => this.lockfile.getLocked(pattern));
+    if (anyLocked) {
+      return;


Why not exclude only the locked ones and still optimize the rest?

So I've made this change, but it causes some workspace related tests to fail ("install should link workspaces that refer each other", "install should not link workspaces that refer not compatible version of another workspace"). I don't quite understand why these are failing, but I've just disabled the optimization wherever remote.type === "workspace". Open to other suggestions.

BYK · 2017-09-18T11:28:02Z

src/package-resolver.js

+      return;
+    }
+
+    const manifestByVersion: Map<string, Manifest> = new Map();


Looks like you just need this.getAllInfoForPackageName(name) above instead.

BYK · 2017-09-18T11:30:00Z

src/package-resolver.js

+    });
+
+    // reverse sort, so we'll find the maximum satisfying version
+    const availableVersions = [...manifestByVersion.keys()];


Why not Array.from(manifestByVersion.keys()). That said spread looks is faster in latest V8 versions so NVM: https://jsperf.com/set-iterator-vs-foreach/4

BYK · 2017-09-18T11:43:45Z

src/package-resolver.js

+    const availableVersions = [...manifestByVersion.keys()];
+    availableVersions.sort(semver.rcompare);
+
+    const requiredRanges = patterns.map(p => normalizePattern(p).range);


I think you can leverage some semver secrets as follows:

const fullRange = patterns.map(pattern => normalizePattern(p).range).join(' '); const oneRingToRuleThemAll = semver.maxSatisfying([...manifestByVersion.keys()], fullRange); if (oneRingToRuleThemAll) { this.collapseAllVersionsOfPackage(name, oneRingToRuleThemAll); }

This should be more efficient than what the code below does. Also, the code below continuously resolves to new versions as it goes on since return in a forEach loop doesn't terminate the loop at all.

Yep, that's a nice optimization.

buildsize · 2017-09-18T23:48:42Z

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

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	835.38 KB	834.94 KB	-445 bytes (0%)
yarn-[version].js	3.7 MB	3.7 MB	-137 bytes (0%)
yarn-legacy-[version].js	3.75 MB	3.74 MB	-551 bytes (0%)
yarn-v[version].tar.gz	839.47 KB	839.06 KB	-423 bytes (0%)
yarn_[version]all.deb	635.3 KB	635.03 KB	-272 bytes (0%)

buildsize · 2017-09-18T23:48:42Z

This change will increase the build size from 9.69 MB to 9.7 MB, an increase of 870 bytes (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	835.17 KB	835 KB	-179 bytes (0%)
yarn-[version].js	3.69 MB	3.7 MB	693 bytes (0%)
yarn-legacy-[version].js	3.74 MB	3.75 MB	554 bytes (0%)
yarn-v[version].tar.gz	839.21 KB	839.11 KB	-102 bytes (0%)
yarn_[version]all.deb	635.2 KB	635.1 KB	-96 bytes (0%)

BYK

I'm a bit worried about impacting performance with this but overall looks good.

Would love to get @arcanis or @kaylieEB's feedback before approving though.

BYK · 2017-09-19T12:58:12Z

src/package-resolver.js

+      const remote = this.patterns[pattern]._remote;
+      return !this.lockfile.getLocked(pattern) && (!remote || remote.type !== 'workspace');
+    });
+    if (collapsablePatterns.length === 0) {


Actually, maybe this should be < 2 since if you have only one pattern, you still don't need to collapse it, right?

BYK · 2017-09-19T12:59:07Z

src/package-resolver.js

@@ -530,10 +545,37 @@ export default class PackageResolver {
      this.resolveToResolution(req);
    }

+    deps.forEach(dep => {


For loops are more efficient so I strongly recommend writing this as a for loop. .forEach becomes as fast only after Node 8.5.

Waiting other's input. The final version LGTM.

…el dependency

arcanis

Makes sense to me. I think the ideal fix would rather be in the hoister (we broke part of it somewhere in 0.26, which is why we noticed some packages stopped being merged together since this release), but it seems like a valid iteration for the time being.

… with top level dependency (#4488)" **Summary** Fixes #4547, unfixes #3780 by reverting #4488 (commit 4020ccd). **Test plan** Existing unit tests.

**Summary** Fixes #4550. The optimization introduced in #4488 should only apply to flat installations since even if a single pattern can satisfy all resolved versions, it is not guaranteed that it is strict enough for resolving correctly for all patterns under all circumstances. **Test plan** Manual verification.

…el dependency (yarnpkg#4488) **Summary** Fixes yarnpkg#3780, and makes the failing test from yarnpkg#3779 passing. As a final step of package resolution, for each dependency we check whether any version satisfies all resolved version ranges. **Test plan** Fixes an existing (failing) test: "unbound transitive dependencies should not conflict with top level dependency"

**Summary** Fixes yarnpkg#4550. The optimization introduced in yarnpkg#4488 should only apply to flat installations since even if a single pattern can satisfy all resolved versions, it is not guaranteed that it is strict enough for resolving correctly for all patterns under all circumstances. **Test plan** Manual verification.

BYK mentioned this pull request Sep 17, 2017

[feature] improve resolution deduping #3778

Open

BYK previously requested changes Sep 18, 2017

View reviewed changes

BYK reviewed Sep 19, 2017

View reviewed changes

BYK requested review from arcanis and kaylie-alexa September 19, 2017 13:06

Fix: unbound transitive dependencies should not conflict with top lev…

fc51000

…el dependency

arcanis reviewed Sep 22, 2017

View reviewed changes

BYK approved these changes Sep 22, 2017

View reviewed changes

BYK merged commit 4020ccd into yarnpkg:master Sep 22, 2017

BYK mentioned this pull request Sep 22, 2017

--frozen-lockfile fails #4454

Closed

BYK mentioned this pull request Sep 27, 2017

Fix: Revert "Fix: unbound transitive dependency optimization" #4559

Closed

BYK mentioned this pull request Oct 3, 2017

fix(resolver): Only optimize patterns in flat mode #4613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: 'yarn --flat' unbound transitive deps shouldn't conflict with top level deps #4488

Fix: 'yarn --flat' unbound transitive deps shouldn't conflict with top level deps #4488

mxmul commented Sep 17, 2017

BYK left a comment

BYK Sep 18, 2017

mxmul Sep 18, 2017

BYK Sep 18, 2017

BYK Sep 18, 2017

BYK Sep 18, 2017

mxmul Sep 18, 2017

buildsize bot commented Sep 18, 2017 •

edited

Loading

buildsize bot commented Sep 18, 2017

BYK left a comment

BYK Sep 19, 2017

BYK Sep 19, 2017

arcanis left a comment

Fix: 'yarn --flat' unbound transitive deps shouldn't conflict with top level deps #4488

Fix: 'yarn --flat' unbound transitive deps shouldn't conflict with top level deps #4488

Conversation

mxmul commented Sep 17, 2017

BYK 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buildsize bot commented Sep 18, 2017 • edited Loading

buildsize bot commented Sep 18, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis left a comment

Choose a reason for hiding this comment

buildsize bot commented Sep 18, 2017 •

edited

Loading