Skip to content

fix(align-deps): handle capabilities resolving to the same package #1950

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

Merged
tido64 merged 2 commits into from
Oct 23, 2022
Merged

fix(align-deps): handle capabilities resolving to the same package #1950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
277f23b
fix(align-deps): handle capabilities resolving to the same package
tido64 Oct 13, 2022
33fb0d9
docs(changeset):
tido64 Oct 21, 2022
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
2 changes: 2 additions & 0 deletions .changeset/six-lamps-warn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
---
79 changes: 42 additions & 37 deletions packages/align-deps/src/preset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,16 @@
import type { Capability } from "@rnx-kit/config";
import semverCoerce from "semver/functions/coerce";
import semverSatisfies from "semver/functions/satisfies";
import semverValidRange from "semver/ranges/valid";
import { gatherRequirements } from "./dependencies";
import type {
AlignDepsConfig,
MetaPackage,
Options,
Package,
Preset,
} from "./types";
import type { AlignDepsConfig, Options, Preset } from "./types";

type Resolution = {
devPreset: Preset;
prodPreset: Preset;
capabilities: Capability[];
};

function compileRequirements(
requirements: string[]
): ((pkg: MetaPackage | Package) => boolean)[] {
const includePrerelease = { includePrerelease: true };
return requirements.map((req) => {
const [requiredPackage, requiredVersionRange] = req.split("@");
return (pkg: MetaPackage | Package) => {
if (pkg.name !== requiredPackage || !("version" in pkg)) {
return false;
}

const coercedVersion = semverCoerce(pkg.version);
if (!coercedVersion) {
throw new Error(`Invalid version number: ${pkg.name}@${pkg.version}`);
}

return semverSatisfies(
coercedVersion,
requiredVersionRange,
includePrerelease
);
};
});
}

function ensurePreset(preset: Preset, requirements: string[]): void {
if (Object.keys(preset).length === 0) {
throw new Error(
Expand All @@ -61,6 +31,23 @@ function loadPreset(
}
}

export function parseRequirements(requirements: string[]): [string, string][] {
return requirements.map((req) => {
const index = req.lastIndexOf("@");
if (index <= 0) {
throw new Error(`Invalid requirement: ${req}`);
}

const name = req.substring(0, index);
const version = req.substring(index + 1);
if (!version || !semverValidRange(version)) {
throw new Error(`Invalid version range in requirement: ${req}`);
}

return [name, version];
});
}

/**
* Filters out any profiles that do not satisfy the specified requirements.
* @param preset The preset to filter
Expand All @@ -69,13 +56,31 @@ function loadPreset(
*/
export function filterPreset(preset: Preset, requirements: string[]): Preset {
const filteredPreset: Preset = {};
const reqs = compileRequirements(requirements);

const includePrerelease = { includePrerelease: true };
const reqs = parseRequirements(requirements);

for (const [profileName, profile] of Object.entries(preset)) {
// FIXME: Some capabilities can resolve to the same package (e.g. core vs core-microsoft)
const packages = Object.values(profile);
const satisfiesRequirements = reqs.every((predicate) =>
packages.some(predicate)
);
const satisfiesRequirements = reqs.every(([pkgName, pkgVersion]) => {
// User provided capabilities can resolve to the same package (e.g. core
// vs core-microsoft). We will only look at the first capability to avoid
// unexpected behaviour, e.g. due to extensions declaring an older version
// of a package that is also declared in the built-in preset.
Comment on lines +67 to +69
Copy link
Contributor

@afoxman afoxman Oct 21, 2022

Choose a reason for hiding this comment

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

Is this kind of conflict something that should be surfaced earlier, and lead to failure? e.g. When you parse requirements and detect an unsatisfiable conflict, you would fail with "The requirements specified conflict with each other, and can never be met. ...details here...".

Copy link
Contributor

@afoxman afoxman Oct 21, 2022

Choose a reason for hiding this comment

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

Otherwise, you start making choices for the user that are hard (not obvious) to debug.

Copy link
Member Author

@tido64 tido64 Oct 23, 2022

Choose a reason for hiding this comment

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

This scenario is outlined in the README: https://github.com/microsoft/rnx-kit/tree/main/packages/align-deps#requirements

I think this is the most reasonable way to handle the conflict given current constraints. But we should probably look into verifying presets and notify users.

const pkg = packages.find((pkg) => pkg.name === pkgName);
if (!pkg || !("version" in pkg)) {
return false;
}

const coercedVersion = semverCoerce(pkg.version);
if (!coercedVersion) {
throw new Error(
`Invalid version number in '${profileName}': ${pkg.name}@${pkg.version}`
);
}

return semverSatisfies(coercedVersion, pkgVersion, includePrerelease);
});
if (satisfiesRequirements) {
filteredPreset[profileName] = profile;
}
Expand Down
86 changes: 72 additions & 14 deletions packages/align-deps/test/preset.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
import { filterPreset } from "../src/preset";
import preset from "../src/presets/microsoft/react-native";
import { filterPreset, parseRequirements } from "../src/preset";
import defaultPreset from "../src/presets/microsoft/react-native";
import profile_0_68 from "../src/presets/microsoft/react-native/profile-0.68";
import profile_0_69 from "../src/presets/microsoft/react-native/profile-0.69";
import profile_0_70 from "../src/presets/microsoft/react-native/profile-0.70";

describe("filterPreset()", () => {
test("returns no profiles if requirements cannot be satisfied", () => {
const profiles = filterPreset(preset, [
const profiles = filterPreset(defaultPreset, [
"react@17.0",
"react-native@>=69.0",
]);
expect(profiles).toEqual({});
});

test("returns profiles satisfying single version range", () => {
const profiles = filterPreset(preset, ["react-native@0.70"]);
const profiles = filterPreset(defaultPreset, ["react-native@0.70"]);
expect(profiles).toEqual({ "0.70": profile_0_70 });
});

test("returns profiles satisfying multiple version ranges", () => {
const profiles = filterPreset(preset, ["react-native@0.68 || 0.70"]);
const profiles = filterPreset(defaultPreset, ["react-native@0.68 || 0.70"]);
expect(profiles).toEqual({ "0.68": profile_0_68, "0.70": profile_0_70 });
});

test("returns profiles satisfying wide version range", () => {
const profiles = filterPreset(preset, ["react-native@>=0.68"]);
const profiles = filterPreset(defaultPreset, ["react-native@>=0.68"]);
expect(profiles).toEqual({
"0.68": profile_0_68,
"0.69": profile_0_69,
Expand All @@ -33,21 +33,79 @@ describe("filterPreset()", () => {
});

test("returns profiles satisfying non-react-native requirements", () => {
const profiles = filterPreset(preset, ["react@18"]);
const profiles = filterPreset(defaultPreset, ["react@18"]);
expect(profiles).toEqual({
"0.69": profile_0_69,
"0.70": profile_0_70,
});
});

test("returns profiles satisfying multiple requirements", () => {
const profiles = filterPreset(preset, [
"react@^18.0",
"react-native@>=0.64",
const profiles = filterPreset(defaultPreset, [
"react@18",
"react-native@<0.70",
]);
expect(profiles).toEqual({
"0.69": profile_0_69,
"0.70": profile_0_70,
});
expect(profiles).toEqual({ "0.69": profile_0_69 });
});

test("ignores extra capabilities resolving to the same package", () => {
const presetWithExtraCapabilities = {
...defaultPreset,
"0.70": {
...defaultPreset["0.70"],
"should-be-ignored": profile_0_69.core,
},
};
const profiles = filterPreset(presetWithExtraCapabilities, [
"react-native@0.69",
]);
expect(profiles).toEqual({ "0.69": profile_0_69 });
});
});

describe("parseRequirements()", () => {
test("throws if requirement is invalid", () => {
expect(() => parseRequirements(["@rnx-kit/align-deps"])).toThrow(
"Invalid requirement"
);
expect(() => parseRequirements(["react-native"])).toThrow(
"Invalid requirement"
);
});

test("throws if version is invalid", () => {
expect(() => parseRequirements(["@rnx-kit/align-deps@"])).toThrow(
"Invalid version range"
);
expect(() => parseRequirements(["@rnx-kit/align-deps@latest"])).toThrow(
"Invalid version range"
);
});

test("returns package name and version", () => {
expect(parseRequirements(["@rnx-kit/align-deps@1.0"])).toEqual([
["@rnx-kit/align-deps", "1.0"],
]);
expect(parseRequirements(["react-native@0.70"])).toEqual([
["react-native", "0.70"],
]);
});
});

describe("presets should not have duplicate packages", () => {
const allowedAliases = ["core", "core-android", "core-ios"];

for (const [name, profile] of Object.entries(defaultPreset)) {
test(`microsoft/react-native/${name}`, () => {
const packages = new Set<string>();
for (const [capability, pkg] of Object.entries(profile)) {
if (pkg.name === "#meta" || allowedAliases.includes(capability)) {
continue;
}

expect(packages.has(pkg.name)).toBe(false);
packages.add(pkg.name);
}
});
}
});