Selective version resolution feature #4105

kaylie-alexa · 2017-08-06T20:50:57Z

Summary

Based on a glob pattern specified in the resolutions field of package.json, yarn will override specific versions of packages. This now includes ranges such as async@>0.9 <2.0 and exotic versions such as "async@https://github.com/caolan/async.git
Warnings are generated when

The glob pattern doesn't end with a valid package name (currently it only checks if it's empty after the last slash, since it's difficult to know what sub-dependencies exist at the time of install)
When the given resolution version is incompatible with the version specified in the request package.json (see screenshot)
When the given resolution version is an invalid semver (currently doesn't support exotic versions)

Edge cases that won't be supported yet

Yarn's resolution method getResolvedPattern has no idea about the parent requests, so this means if one nested dependency happens to have the same semver (say, package a has left-pad~1.0.0 and package b also has left-pad~1.0.0), then they can end up being resolved to the same version defined in resolutions. However if they are different by any one character, such as semver range or version number, then they can individually be resolved. Imho I don't think it's too negative given that under normal circumstances, yarn will resolve both requests to the same package anyways.
I have not yet implemented '**' case where all nested dependencies would resolve to particular version. I can't find any good use case for this.

Test plan
I've modified / added scenarios to @victornoel's existing tests, and it didn't break existing functionalities. More tests coming soon!
//TODO: Add more test cases for warnings and ranges / exotic versions

(Directly nested dependencies pattern)

(All nested dependencies pattern)

arcanis · 2017-08-07T09:41:11Z

src/cli/commands/install.js

-
-    this.resolver = new PackageResolver(config, lockfile);
+    this.resolutions = map();
+    this._resolutions = new Resolutions(config);


Those names are a bit confusing

hm so this.resolutions was what was being previously used for flat mode - I didn't necessarily want to mess with it but the rfc said the plan was to eventually have flat mode replaced by resolutions so I kind of left both as is. How would you rename it?

ResolutionMap / ResolutionRegistry?

arcanis · 2017-08-07T10:16:44Z

src/cli/index.js

@@ -170,6 +170,7 @@ export function main({
  const run = (): Promise<void> => {
    invariant(command, 'missing command');
    return command.run(config, reporter, commander, commander.args).then(exitCode => {
+      reporter.close();


Do you really need to call close on the reporter? It will break the footer call below

the reporter stuff is from the revert of the commit 280b6eb :( it was necessary to have the feature working after rebasing, but let me re-revert it once a fix lands.

arcanis · 2017-08-07T10:18:31Z

src/package-resolver.js

    this.patternsByPackage = map();
    this.fetchingPatterns = map();
    this.fetchingQueue = new BlockingQueue('resolver fetching');
    this.patterns = map();
+    this.resolutions = resolutions || new Resolutions(config);


You can use a default parameter instead of || new Resolutions(config)

you're right! thanks :)

Note that they have different semantics. The default parameter will only work if the argument is omitted. If the argument is falsy, it will be null in the end:

const fn = (arg = 42) => arg; fn(null); // null const fn = arg => arg || 42; fn(null); // 42

It depends on what you are looking for.

arcanis · 2017-08-07T10:25:29Z

src/resolutions.js

+      return resolvedPattern;
+    }
+
+    const {version} = resolutions.find(({pattern}) => minimatch(path.join(...parentNames, name), pattern)) || {};


We should probably use an explicit .join('/'), since path.join is system-specific.

arcanis · 2017-08-07T10:27:41Z

src/resolutions.js

+
+      if (!semver.valid(version)) {
+        this.reporter.warn(this.reporter.lang('invalidResolutionVersion', resolvedPattern));
+      }


This warning will cause issues when people will use the exotic resolvers such as file: or git, which I assume will account for a fair percent of the feature usage (will allow people to fix a bug in a fork).

Ok, my thought was that most people will put in versions and not exotic resolvers. But I can look into supporting the latter for v1 if you think it'll be most of the use case.

I think it would be best if we could support it for 1.0, yep. But we can merge this PR before that I you prefer, and iterate on it.

arcanis · 2017-08-07T10:33:53Z

src/resolutions.js

+
+      if (!semver.satisfies(version, range)) {
+        this.reporter.warn(this.reporter.lang('incompatibleResolutionVersion', resolvedPattern, reqPattern));
+      }


The resolution value is a range, not a pinned reference. We should delay printing a warning until the range has been resolved (we can do the check directly in the resolvers).

arcanis · 2017-08-07T10:35:51Z

Nice PR! Left a few notes here and there, let me know what you think :)

kaylie-alexa · 2017-08-09T15:58:35Z

I've resolved the TODO item above and been working on supporting exotic ranges -- will try to post the revision and merge this weekend :)

arcanis · 2017-08-14T14:17:26Z

I've run the tests locally and they failed to pass, I'll take some time to investigate a bit and see if my environment is responsible 👍

arcanis

Was my environment! Looks good to go :)

bestander · 2017-08-14T17:58:02Z

Best feature ever!!!

cpojer · 2017-08-14T18:26:32Z

Party time! @kaylieEB this work was impressive and we are all extremely grateful that you did all this work :)

victornoel · 2017-08-24T07:23:37Z

Thanks a lot for taking care of that :)

EmielM · 2022-11-13T22:11:15Z

Sorry for hijacking the topic, but was it ever a consideration to also allow filtering (nested) dependencies with this, eg by allowing "none", "skip" or "filter" in the resolution map? Seems like simple way to implement #4611.

arcanis reviewed Aug 7, 2017

View reviewed changes

BYK mentioned this pull request Aug 7, 2017

Selective versions resolutions #3868

Closed

3 tasks

BYK self-assigned this Aug 9, 2017

victornoel and others added 3 commits August 12, 2017 10:07

refactor some test helpers

7010ee1

first failing tests

f40f7c1

WIP

5c46448

kaylie-alexa force-pushed the selective-resolution branch from 8eee43d to 5c46448 Compare August 12, 2017 17:08

kaylie-alexa changed the title ~~[WIP] Selective version resolution feature~~ Selective version resolution feature Aug 12, 2017

BYK assigned arcanis and unassigned BYK Aug 14, 2017

Uses "ResolutionMap" instead of the duplicate name "Resolutions"

d18c93e

arcanis approved these changes Aug 14, 2017

View reviewed changes

arcanis merged commit 634c239 into yarnpkg:master Aug 14, 2017

vjpr mentioned this pull request Aug 15, 2017

Lock a sub-dependency version pnpm/pnpm#651

Closed

magnushiie mentioned this pull request Aug 15, 2017

yarn list with resolutions gives "expected manifest" error #4178

Closed

This was referenced Aug 22, 2017

Docs: document selective version resolutions yarnpkg/website#605

Closed

yarn upgrade react@next breaks webpack bundling, peer dependencies #4043

Closed

s-panferov mentioned this pull request Aug 23, 2017

Selective module resolution doesn't work with scoped packages #4242

Closed

kaylie-alexa mentioned this pull request Aug 26, 2017

Installing phantomjs-prebuilt fails due to no npm_config_tmp #1538

Closed

glentakahashi mentioned this pull request Sep 6, 2017

difference between npm and yarn behavior with nested dependencies #3951

Closed

BYK mentioned this pull request Sep 15, 2017

Handling dependencies of dependencies and their resolution #2763

Closed

Jontem mentioned this pull request Sep 18, 2017

Installing Typescript definitions for react and react-addons-test-utils is inconsistent with npm #4226

Open

alanhogan mentioned this pull request Oct 6, 2017

Start warning when multiple versions of a library appear in one build webpack/webpack#385

Open

HaleTom mentioned this pull request Oct 6, 2017

Enable global npm installation sc-forks/solidity-coverage#129

Closed

IgorMinar mentioned this pull request Oct 21, 2017

the "resolutions" block of package.json is not documented #4754

Closed

Noahsok mentioned this pull request Mar 6, 2018

Emoji with skin color modifier is shown as two glyphs (macOS) signalapp/Signal-Desktop#2065

Open

1 task

pward123 mentioned this pull request Nov 1, 2018

Engine bump is a minor revision change? hapijs/topo#32

Closed

coreyward mentioned this pull request May 13, 2019

Bug: "resolutions" trumps --ignore-optional #6040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selective version resolution feature #4105

Selective version resolution feature #4105

kaylie-alexa commented Aug 6, 2017 •

edited

Loading

arcanis Aug 7, 2017

kaylie-alexa Aug 7, 2017

arcanis Aug 9, 2017

arcanis Aug 7, 2017

kaylie-alexa Aug 7, 2017

arcanis Aug 7, 2017

kaylie-alexa Aug 7, 2017

cpojer Aug 8, 2017

arcanis Aug 7, 2017

arcanis Aug 7, 2017

kaylie-alexa Aug 7, 2017

arcanis Aug 9, 2017

arcanis Aug 7, 2017

kaylie-alexa Aug 7, 2017

arcanis commented Aug 7, 2017

kaylie-alexa commented Aug 9, 2017

arcanis commented Aug 14, 2017 •

edited

Loading

arcanis left a comment

bestander commented Aug 14, 2017

cpojer commented Aug 14, 2017

victornoel commented Aug 24, 2017

EmielM commented Nov 13, 2022

Selective version resolution feature #4105

Selective version resolution feature #4105

Conversation

kaylie-alexa commented Aug 6, 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

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

arcanis commented Aug 7, 2017

kaylie-alexa commented Aug 9, 2017

arcanis commented Aug 14, 2017 • edited Loading

arcanis left a comment

Choose a reason for hiding this comment

bestander commented Aug 14, 2017

cpojer commented Aug 14, 2017

victornoel commented Aug 24, 2017

EmielM commented Nov 13, 2022

kaylie-alexa commented Aug 6, 2017 •

edited

Loading

arcanis commented Aug 14, 2017 •

edited

Loading