Delay resolving patterns to existing versions until all final version…

…s are known. Fixes #3466 (#3477)

__tests__/commands/install/integration.js

@@ -758,3 +758,14 @@ 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__/fixtures/install/install-deterministic-versions/package.json

@@ -0,0 +1,7 @@
+{
+  "dependencies": {
+    "graceful-fs": "4.1.10",
+    "write-file-atomic": "1.3.1",
+    "path-type": "1.1.0"
+  }
+}

src/cli/commands/import.js

@@ -235,6 +235,9 @@ 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,6 +32,7 @@ export default class PackageRequest {
     this.optional = req.optional;
     this.pattern = req.pattern;
     this.config = resolver.config;
+    this.foundInfo = null;
 
     resolver.usedRegistries.add(req.registry);
   }
@@ -54,6 +55,7 @@ export default class PackageRequest {
   config: Config;
   registry: ResolverRegistryNames;
   optional: boolean;
+  foundInfo: ?Manifest;
 
   getParentNames(): Array<string> {
     const chain = [];
@@ -226,6 +228,24 @@ 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
    */
@@ -246,11 +266,7 @@ export default class PackageRequest {
     const {range, name} = PackageRequest.normalizePattern(this.pattern);
     const resolved: ?Manifest = this.resolver.getHighestRangeVersionMatch(name, range);
     if (resolved) {
-      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);
+      this.resolver.reportPackageWithExistingVersion(this, info);
       return;
     }
 

src/package-resolver.js

@@ -27,6 +27,7 @@ export default class PackageResolver {
     this.reporter = config.reporter;
     this.lockfile = lockfile;
     this.config = config;
+    this.delayedResolveQueue = [];
   }
 
   // whether the dependency graph will be flattened
@@ -78,6 +79,10 @@ 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
    */
@@ -474,7 +479,32 @@ export default class PackageResolver {
     //
     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();
+
     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);
+    }
+  }
 }