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

Prioritize direct dependency if available #192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Prioritize direct dependency if available #192

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,35 @@ It will deduplicate `library@*` and `library@>=1.1.0` to `1.2.0`.
Note that this may cause some packages to be **downgraded**. Be sure to check the changelogs between
all versions and understand the consequences of that downgrade. If unsure, don't use this strategy.

`direct` will prioritize dependencies specified in `package.json` but will use the highest version
otherwise. For example, with the following `yarn.lock`:

```text
library@*:
version "2.0.0"

library@^1.2.0:
version "1.2.0"

other@*:
version "2.0.0"

other@^1.3.0:
version "1.3.0"
```

and `package.json`:
```
{
"dependencies": {
"library": "^1.2.0",
...
},
}
```

It will deduplicate `library@*` to `1.2.0` but keep `other@^1.3.0` as is.

It is not recommended to use different strategies for different packages. There is no guarantee that
the strategy will be honored in subsequent runs of `yarn-deduplicate` unless the same set of flags
is specified again.
Expand Down
16 changes: 10 additions & 6 deletions src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ program
.usage('[options] [yarn.lock path (default: yarn.lock)]')
.option(
'-s, --strategy <strategy>',
'deduplication strategy. Valid values: fewer, highest. Default is "highest"',
'deduplication strategy. Valid values: fewer, highest, direct.',
Copy link
Author

Choose a reason for hiding this comment

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

commander already shows the default value so I removed Default is "highest" as it was redundant.

'highest'
)
.option('-l, --list', 'do not change yarn.lock, just output the diagnosis')
Expand All @@ -32,7 +32,8 @@ program
.option(
'--includePrerelease',
'Include prereleases in version comparisons, e.g. ^1.0.0 will be satisfied by 1.0.1-alpha'
);
)
.option('--package-json <path>', 'path to package.json, used with direct strategy', 'package.json');

program.parse(process.argv);

Expand All @@ -47,6 +48,7 @@ const {
includePrerelease,
print,
noStats,
packageJson: packageJsonPath,
} = program.opts();

const file = program.args.length ? program.args[0] : 'yarn.lock';
Expand All @@ -56,18 +58,19 @@ if (scopes && packages) {
program.help();
}

if (strategy !== 'highest' && strategy !== 'fewer') {
if (strategy !== 'highest' && strategy !== 'fewer' && strategy !== 'direct') {
console.error(`Invalid strategy ${strategy}`);
program.help();
}

try {
const yarnLock = fs.readFileSync(file, 'utf8');
const useMostCommon = strategy === 'fewer';
const packageJson = strategy === 'direct' ? fs.readFileSync(packageJsonPath, 'utf8') : null;

if (list) {
const duplicates = listDuplicates(yarnLock, {
useMostCommon,
packageJson,
strategy,
includeScopes: scopes,
includePackages: packages,
excludePackages: exclude,
Expand All @@ -81,7 +84,8 @@ try {
}
} else {
let dedupedYarnLock = fixDuplicates(yarnLock, {
useMostCommon,
packageJson,
strategy,
includeScopes: scopes,
includePackages: packages,
excludePackages: exclude,
Expand Down
75 changes: 64 additions & 11 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import * as lockfile from '@yarnpkg/lockfile';
import semver from 'semver';

type PackageJson = {
dependencies?: Record<string, string>,
devDependencies?: Record<string, string>,
optionalDependencies?: Record<string, string>
}

type YarnEntry = {
resolved: string
version: string
Expand All @@ -23,20 +29,43 @@ type Package = {

type Version = {
pkg: YarnEntry,
isDirectDependency: boolean,
satisfies: Set<Package>
}

type Versions = Map<string, Version>;

export type Strategy = 'highest' | 'fewer' | 'direct';

type Options = {
packageJson?: string | null;
includeScopes?: string[];
includePackages?: string[];
excludePackages?: string[];
excludeScopes?: string[];
useMostCommon?: boolean;
strategy?: Strategy;
includePrerelease?: boolean;
}

const getDirectDependencies = (file: string | null): Set<string> => {
const result = new Set<string>();
if (file === null) {
return result;
}

const packageJson = JSON.parse(file) as PackageJson;
for (const [packageName, requestedVersion] of Object.entries(packageJson.dependencies ?? {})) {
result.add(`${packageName}@${requestedVersion}`);
}
for (const [packageName, requestedVersion] of Object.entries(packageJson.devDependencies ?? {})) {
result.add(`${packageName}@${requestedVersion}`);
}
for (const [packageName, requestedVersion] of Object.entries(packageJson.optionalDependencies ?? {})) {
result.add(`${packageName}@${requestedVersion}`);
}
return result;
}

const parseYarnLock = (file:string) => lockfile.parse(file).object as YarnEntries;

const extractPackages = (
Expand Down Expand Up @@ -100,18 +129,32 @@ const extractPackages = (
return packages;
};

const computePackageInstances = (packages: Packages, name: string, useMostCommon: boolean, includePrerelease = false): Package[] => {
const computePackageInstances = (
packages: Packages,
name: string,
strategy: Strategy,
directDependencies: Set<string>,
includePrerelease = false,
): Package[] => {
// Instances of this package in the tree
const packageInstances = packages[name];

// Extract the list of unique versions for this package
const versions:Versions = new Map();
for (const packageInstance of packageInstances) {
if (versions.has(packageInstance.installedVersion)) continue;
versions.set(packageInstance.installedVersion, {
pkg: packageInstance.pkg,
satisfies: new Set(),
})
// Mark candidates which have at least one requested version matching a
// direct dependency as direct
const isDirectDependency = directDependencies.has(`${name}@${packageInstance.requestedVersion}`);
if (versions.has(packageInstance.installedVersion)) {
const existingPackage = versions.get(packageInstance.installedVersion)!;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't get why we need to add isDirectDependency to every dep in the tree.

Wouldn't checking directDependencies.has(entryName) when sorting the versions (line 177) suffice? Especially when we encapsulate this logic into its own strategy.

Copy link
Author

Choose a reason for hiding this comment

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

I've moved this down here so only Version needs the isDirectDependency flag. There's no entryName to check in the version sorting step so it's not that easy to do it there. If we absolutely want to avoid having this, I suppose it's possible to loop over satisfies in the sorting function and check if any requested version is a direct dependency but that'd just be worse for performance.

existingPackage.isDirectDependency ||= isDirectDependency;
} else {
versions.set(packageInstance.installedVersion, {
pkg: packageInstance.pkg,
satisfies: new Set(),
isDirectDependency,
});
}
}

// Link each package instance with all the versions it could satisfy.
Expand Down Expand Up @@ -139,7 +182,15 @@ const computePackageInstances = (packages: Packages, name: string, useMostCommon
// Compute the versions that actually satisfy this instance
packageInstance.candidateVersions = Array.from(packageInstance.satisfiedBy);
packageInstance.candidateVersions.sort((versionA:string, versionB:string) => {
if (useMostCommon) {
if (strategy === 'direct') {
// Sort versions that are specified in package.json first. In
// case of a tie, use the highest version.
const isDirectA = versions.get(versionA)!.isDirectDependency;
const isDirectB = versions.get(versionB)!.isDirectDependency;
if (isDirectA && !isDirectB) return -1;
if (!isDirectB && isDirectA) return 1;
}
if (strategy === 'fewer') {
// Sort verions based on how many packages it satisfies. In case of a tie, put the
// highest version first.
const satisfiesA = (versions.get(versionA) as Version).satisfies;
Expand All @@ -160,11 +211,12 @@ const computePackageInstances = (packages: Packages, name: string, useMostCommon
export const getDuplicates = (
yarnEntries: YarnEntries,
{
packageJson = null,
includeScopes = [],
includePackages = [],
excludePackages = [],
excludeScopes = [],
useMostCommon = false,
strategy = 'highest',
includePrerelease = false,
}: Options = {}
): Package[] => {
Expand All @@ -176,11 +228,13 @@ export const getDuplicates = (
excludeScopes
);

const directDependencies = getDirectDependencies(packageJson);

return Object.keys(packages)
.reduce(
(acc:Package[], name) =>
acc.concat(
computePackageInstances(packages, name, useMostCommon, includePrerelease)
computePackageInstances(packages, name, strategy, directDependencies, includePrerelease)
),
[]
)
Expand All @@ -206,4 +260,3 @@ export const fixDuplicates = ( yarnLock: string, options: Options = {} ) => {

return lockfile.stringify(json);
};

46 changes: 44 additions & 2 deletions tests/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ test('dedupes lockfile to most common compatible version', () => {
resolved "https://example.net/library@^2.1.0"
`;
const deduped = fixDuplicates(yarn_lock, {
useMostCommon: true,
strategy: 'fewer',
});
const json = lockfile.parse(deduped).object;

Expand All @@ -53,7 +53,7 @@ test('dedupes lockfile to most common compatible version', () => {
expect(json['library@^2.0.0']['version']).toEqual('2.1.0');

const list = listDuplicates(yarn_lock, {
useMostCommon: true,
strategy: 'fewer',
});

expect(list).toContain('Package "library" wants >=1.0.0 and could get 2.1.0, but got 3.0.0');
Expand Down Expand Up @@ -272,3 +272,45 @@ test('should support the integrity field if present', () => {
// We should not have made any change to the order of outputted lines (@yarnpkg/lockfile 1.0.0 had this bug)
expect(yarn_lock).toBe(deduped);
});

test('prioritizes direct requirements if present', () => {
const yarn_lock = outdent`
a-package@*:
Copy link
Owner

Choose a reason for hiding this comment

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

Ideally (and I acknowledge this is slightly out of the scope of the PR), we should use a similar yarn.lock to test all strategies (i.e. the first two tests in this file). That way it's more clear what's the different behaviour of each strategy.

version "2.0.0"
resolved "http://example.com/a-package/2.0.0"

a-package@^1.0.0, a-package@^1.0.1, a-package@^1.0.2:
version "1.0.2"
resolved "http://example.com/a-package/1.0.2"

a-package@^0.1.0:
version "0.1.0"
resolved "http://example.com/a-package/0.1.0"

other-package@>=1.0.0:
Copy link
Owner

Choose a reason for hiding this comment

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

We should remove any mention to other-package in this test, as it is not related to the test intention.

Copy link
Author

Choose a reason for hiding this comment

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

This is actually important as it verifies that packages that are not in package.json fall back to the highest strategy.

version "2.0.0"
resolved "http://example.com/other-package/2.0.0"

other-package@^1.0.0:
version "1.0.12"
resolved "http://example.com/other-package/1.0.12"
`;
const package_json = outdent`
{
"dependencies": {
"a-package": "^1.0.1"
}
}
`;

const deduped = fixDuplicates(yarn_lock, {
strategy: 'direct',
packageJson: package_json,
});
const json = lockfile.parse(deduped).object;
expect(json['a-package@*']['version']).toEqual('1.0.2');
expect(json['a-package@^1.0.0']['version']).toEqual('1.0.2');
expect(json['a-package@^0.1.0']['version']).toEqual('0.1.0');
expect(json['other-package@>=1.0.0']['version']).toEqual('2.0.0');
expect(json['other-package@^1.0.0']['version']).toEqual('1.0.12');
});