Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion __tests__/commands/install/integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,8 @@ test.concurrent('transitive file: dependencies should work', (): Promise<void> =
});
});

test('unbound transitive dependencies should not conflict with top level dependency', async () => {
// Unskip once https://github.com/yarnpkg/yarn/issues/3778 is resolved
test.skip('unbound transitive dependencies should not conflict with top level dependency', async () => {
await runInstall({flat: true}, 'install-conflicts', async config => {
expect((await fs.readJson(path.join(config.cwd, 'node_modules', 'left-pad', 'package.json'))).version).toEqual(
'1.0.0',
Expand Down
42 changes: 0 additions & 42 deletions src/package-resolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,14 +228,6 @@ export default class PackageResolver {

getAllInfoForPackageName(name: string): Array<Manifest> {
const patterns = this.patternsByPackage[name] || [];
return this.getAllInfoForPatterns(patterns);
}

/**
* Retrieve all the package info stored for a list of patterns.
*/

getAllInfoForPatterns(patterns: string[]): Array<Manifest> {
const infos = [];
const seen = new Set();

Expand Down Expand Up @@ -292,13 +284,6 @@ export default class PackageResolver {

collapseAllVersionsOfPackage(name: string, version: string): string {
const patterns = this.dedupePatterns(this.patternsByPackage[name]);
return this.collapsePackageVersions(name, version, patterns);
}

/**
* Make all given patterns resolve to version.
*/
collapsePackageVersions(name: string, version: string, patterns: string[]): string {
const human = `${name}@${version}`;

// get manifest that matches the version we're collapsing too
Expand Down Expand Up @@ -545,37 +530,10 @@ export default class PackageResolver {
this.resolveToResolution(req);
}

for (const dep of deps) {
const name = normalizePattern(dep.pattern).name;
this.optimizeResolutions(name);
}

activity.end();
this.activity = null;
}

// for a given package, see if a single manifest can satisfy all ranges
optimizeResolutions(name: string) {
const patterns: Array<string> = this.dedupePatterns(this.patternsByPackage[name] || []);

// don't optimize things that already have a lockfile entry:
// https://github.com/yarnpkg/yarn/issues/79
const collapsablePatterns = patterns.filter(pattern => {
const remote = this.patterns[pattern]._remote;
return !this.lockfile.getLocked(pattern) && (!remote || remote.type !== 'workspace');
});
if (collapsablePatterns.length < 2) {
return;
}

const availableVersions = this.getAllInfoForPatterns(collapsablePatterns).map(manifest => manifest.version);
const combinedRange = collapsablePatterns.map(pattern => normalizePattern(pattern).range).join(' ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could #4547 also be fixed by removing this semver range munging (patterns.join(' ')), and testing each version against every pattern in turn? This seems to be basically what semver.maxSatisfying is doing, but would do the correct thing with ranges containing "||".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. My initial approach was to do a proper intersection and I have the code locally. The problem is range can be exotic too like file:./a.

const singleVersion = semver.maxSatisfying(availableVersions, combinedRange);
if (singleVersion) {
this.collapsePackageVersions(name, singleVersion, collapsablePatterns);
}
}

/**
* 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
Expand Down