diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js index 1aaf482295..e2d32956a0 100644 --- a/__tests__/commands/install/integration.js +++ b/__tests__/commands/install/integration.js @@ -757,3 +757,14 @@ test.concurrent('bailout should work with --production flag too', (): Promise => { + // 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}, []); + }); +}); diff --git a/__tests__/fixtures/install/install-deterministic-versions/package.json b/__tests__/fixtures/install/install-deterministic-versions/package.json new file mode 100644 index 0000000000..9f08749c73 --- /dev/null +++ b/__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" + } +} diff --git a/src/cli/commands/import.js b/src/cli/commands/import.js index 32cee76481..02cef293d6 100644 --- a/src/cli/commands/import.js +++ b/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); diff --git a/src/package-request.js b/src/package-request.js index 8a3fec64b3..70105b9c6a 100644 --- a/src/package-request.js +++ b/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 { 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; } diff --git a/src/package-resolver.js b/src/package-resolver.js index 040bcf8362..11dbf70424 100644 --- a/src/package-resolver.js +++ b/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 => 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); + } + } }