[WIP] Fix 3023

__tests__/commands/install/integration.js

@@ -822,14 +822,3 @@ test.concurrent('bailout should work with --production flag too', (): Promise<vo
     expect(await fs.exists(path.join(config.cwd, 'node_modules', 'left-pad', 'index.js'))).toBe(false);
   });
 });
-
-test.concurrent('package version resolve should be deterministic', (): Promise<void> => {
-  // Scenario:
-  // graceful-fs will install two versions, from @4.1.10 and @^4.1.11. The pattern @^4.1.2 would sometimes resolve
-  // to 4.1.10, if @^4.1.11 hadn't been processed before. Otherwise it would resolve to the result of @^4.1.11.
-  // Run an independent install and check, and see they have different results for @^4.1.2 - won't always see
-  // the bug, but its the best we can do without creating mock registry with controlled timing of responses.
-  return runInstall({}, 'install-deterministic-versions', async (config, reporter) => {
-    await check(config, reporter, {integrity: true}, []);
-  });
-});

__tests__/commands/install/lockfiles.js

@@ -295,3 +295,29 @@ test.concurrent("install should fix if lockfile patterns don't match resolved ve
     expect(lockContent).toContain('left-pad-1.1.2.tgz');
   });
 });
+
+// issue https://github.com/yarnpkg/yarn/issues/3023
+test.concurrent("install should downgrade a subdependency version if it is available", (): Promise<void> => {
+  // uglify-js@^2.6.2:
+  // version "2.8.18"
+  // resolved "http://r.cnpmjs.org/uglify-js/download/uglify-js-2.8.18.tgz#925d14bae48ab62d1883b41afe6e2261662adb8e"
+  //
+  // uglify-js@~2.7.3:
+  // version "2.7.5"
+  // resolved "http://r.cnpmjs.org/uglify-js/download/uglify-js-2.7.5.tgz#4612c0c7baaee2ba7c487de4904ae122079f2ca8"
+
+  // could be optimized to
+
+  // uglify-js@~2.7.3, uglify-js@^2.6.2::
+  // version "2.7.5"
+  // resolved "http://r.cnpmjs.org/uglify-js/download/uglify-js-2.7.5.tgz#4612c0c7baaee2ba7c487de4904ae122079f2ca8"
+
+  const fixture = 'lockfile-optimization';
+
+  return runInstall({}, fixture, async (config, reporter) => {
+    // no unhoisted unglify-js
+    expect(await fs.exists(path.join(config.cwd, 'node_modules', 'uglify-js'))).toBe(true);
+    expect(await fs.exists(path.join(config.cwd, 'node_modules', 'uglify-js'))).toBe(false);
+    expect(await fs.exists(path.join(config.cwd, 'node_modules', 'uglify-js'))).toBe(false);
+  });
+});

__tests__/fixtures/install/lockfile-optimization/package.json

@@ -0,0 +1,15 @@
+{
+  "name": "yarn0test",
+  "version": "1.0.0",
+  "description": "yarn test",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "dependencies": {
+    "webpack": "1.14.0",
+    "webpack-parallel-uglify-plugin": "0.2.0"
+  },
+  "author": "h",
+  "license": "MIT"
+}

src/cli/commands/import.js

@@ -235,9 +235,6 @@ class ImportPackageResolver extends PackageResolver {
     deps = this.next;
     this.next = [];
     if (!deps.length) {
-      // all required package versions have been discovered, so now packages that
-      // resolved to existing versions can be resolved to their best available version
-      this.resolvePackagesWithExistingVersions();
       return;
     }
     await this.findAll(deps);

src/package-request.js

@@ -32,7 +32,6 @@ export default class PackageRequest {
     this.optional = req.optional;
     this.pattern = req.pattern;
     this.config = resolver.config;
-    this.foundInfo = null;
 
     resolver.usedRegistries.add(req.registry);
   }
@@ -55,7 +54,6 @@ export default class PackageRequest {
   config: Config;
   registry: ResolverRegistryNames;
   optional: boolean;
-  foundInfo: ?Manifest;
 
   getParentNames(): Array<string> {
     const chain = [];
@@ -228,24 +226,6 @@ export default class PackageRequest {
 
   reportResolvedRangeMatch(info: Manifest, resolved: Manifest) {}
 
-  /**
-   * Do the final resolve of a package that had a match with an existing version.
-   * After all unique versions have been discovered, so the best available version
-   * is found.
-   */
-  resolveToExistingVersion(info: Manifest) {
-    // get final resolved version
-    const {range, name} = PackageRequest.normalizePattern(this.pattern);
-    const resolved: ?Manifest = this.resolver.getHighestRangeVersionMatch(name, range);
-    invariant(resolved, 'should have a resolved reference');
-
-    this.reportResolvedRangeMatch(info, resolved);
-    const ref = resolved._reference;
-    invariant(ref, 'Resolved package info has no package reference');
-    ref.addRequest(this);
-    ref.addPattern(this.pattern, resolved);
-  }
-
   /**
    * TODO description
    */
@@ -267,7 +247,11 @@ export default class PackageRequest {
     const {range, name} = PackageRequest.normalizePattern(this.pattern);
     const resolved: ?Manifest = this.resolver.getHighestRangeVersionMatch(name, range);
     if (resolved) {
-      this.resolver.reportPackageWithExistingVersion(this, info);
+      this.reportResolvedRangeMatch(info, resolved);
+      const ref = resolved._reference;
+      invariant(ref, 'Resolved package info has no package reference');
+      ref.addRequest(this);
+      ref.addPattern(this.pattern, resolved);
       return;
     }
 

src/package-resolver.js

@@ -26,7 +26,6 @@ export default class PackageResolver {
     this.reporter = config.reporter;
     this.lockfile = lockfile;
     this.config = config;
-    this.delayedResolveQueue = [];
   }
 
   // whether the dependency graph will be flattened
@@ -71,10 +70,6 @@ export default class PackageResolver {
   // environment specific config methods and options
   config: Config;
 
-  // list of packages need to be resolved later (they found a matching version in the
-  // resolver, but better matches can still arrive later in the resolve process)
-  delayedResolveQueue: Array<{req: PackageRequest, info: Manifest}>;
-
   /**
    * TODO description
    */
@@ -316,6 +311,9 @@ export default class PackageResolver {
 
   addPattern(pattern: string, info: Manifest) {
     this.patterns[pattern] = info;
+    if(info.name === 'uglify-js') {
+      console.log("ADD PATTERN", pattern, info.version)
+    }
 
     const byName = (this.patternsByPackage[info.name] = this.patternsByPackage[info.name] || []);
     byName.push(pattern);
@@ -395,6 +393,9 @@ export default class PackageResolver {
 
       return info;
     });
+    if(name === 'uglify-js') {
+      console.log(versionNumbers)
+    }
 
     const maxValidRange = semver.maxSatisfying(versionNumbers, range);
     if (!maxValidRange) {
@@ -448,6 +449,21 @@ export default class PackageResolver {
     await request.find(fresh);
   }
 
+  // for a given package name see if less manifests can satisfy most resolutions
+  optimizeResolutions(name: string, patterns: Array<string>) {
+    const manifests: Set<Manifest> = new Set();
+    // TODO map version to pattern
+    patterns.forEach(p => {
+      manifests.add(this.patterns[p]);
+    });
+    const availableVersions: Array<string> = [...manifests].map(m => m.version);
+    // TODO only non exotic ones
+    const requiredRanges = patterns.map(p => PackageRequest.normalizePattern(p).range);
+
+    // TODO match availableVersions to ranges so that we get  
+
+  }
+
   /**
    * TODO description
    */
@@ -457,32 +473,12 @@ export default class PackageResolver {
     const activity = (this.activity = this.reporter.activity());
     await Promise.all(deps.map((req): Promise<void> => this.find(req)));
 
-    // all required package versions have been discovered, so now packages that
-    // resolved to existing versions can be resolved to their best available version
-    this.resolvePackagesWithExistingVersions();
+    Object.keys(this.patternsByPackage).forEach((name) => {
+      this.optimizeResolutions(name, this.patternsByPackage[name]);
+    });
+    // TODO clean orphan patterns - the ones that are not needed after optimization
 
     activity.end();
     this.activity = null;
   }
-
-  /**
-    * Called by the package requester for packages that this resolver already had
-    * a matching version for. Delay the resolve, because better matches can still be
-    * discovered.
-    */
-
-  reportPackageWithExistingVersion(req: PackageRequest, info: Manifest) {
-    this.delayedResolveQueue.push({req, info});
-  }
-
-  /**
-    * Executes the resolve to existing versions for packages after the find process,
-    * when all versions that are going to be used have been discovered.
-    */
-
-  resolvePackagesWithExistingVersions() {
-    for (const {req, info} of this.delayedResolveQueue) {
-      req.resolveToExistingVersion(info);
-    }
-  }
 }