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

Knarwani/fix workspace resolution issue #2550

Closed
wants to merge 3 commits into from
Closed

Knarwani/fix workspace resolution issue #2550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d9d3ff
Add feature for workspace resolution in Monorepo
krishna8770 Jul 21, 2023
60ce377
Resolve test issues due to added feature
krishna8770 Jul 21, 2023
e3be60c
Update the lock file
krishna8770 Jul 21, 2023
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: 3 additions & 0 deletions packages/align-deps/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@rnx-kit/tools-node": "*",
"@rnx-kit/tools-workspaces": "*",
"@types/jest": "^29.2.1",
"@types/parse-package-name": "0.1.0",
"@types/prompts": "^2.0.0",
"@types/semver": "^7.0.0",
"@types/yargs": "^16.0.0",
Expand All @@ -44,10 +45,12 @@
"jest": "^29.2.1",
"markdown-table": "^3.0.0",
"package-json": "^8.1.1",
"parse-package-name": "^0.1.0",
"prettier": "^3.0.0",
"prompts": "^2.4.0",
"semver": "^7.0.0",
"typescript": "^5.0.0",
"workspace-tools": "^0.26.0",
"yargs": "^16.0.0"
},
"engines": {
Expand Down
13 changes: 8 additions & 5 deletions packages/align-deps/src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { defaultConfig } from "./config";
import { printError, printInfo } from "./errors";
import { isString } from "./helpers";
import type { Args, Command } from "./types";
import type { PackageInfos } from "workspace-tools";
import { getPackageInfos } from "workspace-tools";

export const cliOptions = {
"exclude-packages": {
Expand Down Expand Up @@ -142,7 +144,7 @@ function reportConflicts(conflicts: [string, string][], args: Args): boolean {
}, false);
}

async function makeCommand(args: Args): Promise<Command | undefined> {
async function makeCommand(args: Args, allPackages: PackageInfos): Promise<Command | undefined> {
const conflicts: [string, string][] = [
["init", "set-version"],
["init", args.write ? "write" : "no-write"],
Expand Down Expand Up @@ -175,21 +177,22 @@ async function makeCommand(args: Args): Promise<Command | undefined> {
};

if (typeof init !== "undefined") {
return makeInitializeCommand(init, options);
return makeInitializeCommand(init, options, allPackages);
}

// When `--set-version` is without a value, `setVersion` is an empty string if
// invoked directly. When invoked via `@react-native-community/cli`,
// `setVersion` is `true` instead.
if (setVersion || isString(setVersion)) {
return makeSetVersionCommand(setVersion, options);
return makeSetVersionCommand(setVersion, options, allPackages);
}

return makeCheckCommand(options);
return makeCheckCommand(options, allPackages);
}

export async function cli({ packages, ...args }: Args): Promise<void> {
const command = await makeCommand(args);
const allPackages = getPackageInfos(process.cwd());
Copy link
Member

Choose a reason for hiding this comment

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

Hi, thanks for working on this. I think it's a good start, but I have several issues with this:

  1. We're scanning the whole monorepo and reading all packages before the command has been created. This work may be thrown away if we exit early here.
  2. By introducing this call, we're now doing two scans instead of one. See getManifests().
  3. getPackageInfos eagerly reads all packages to return PackageInfos. We may not even need them. This should be done lazily.
  4. We don't want to introduce new dependencies, i.e. we cannot add parse-package-name and workspace-tools. If we're missing anything, we should consider adding them to tools-node or tools-workspaces instead.

Keep in mind that we have users running align-deps as a Git hook. This command cannot be reading the whole monorepo before the checks even start. We need to postpone work as much as possible until we actually need them.

We should be passing the list of manifests to the command itself and then to loadPreset. loadPreset will try finding the package from the monorepo if it's there. This result should be cached so that we only perform this once. If we are passing a "context" object with all the information, we can easily add caching to it. This will require us to modify the signature of commands, but we should be able to make it backwards compatible:

 export function checkPackageManifest(
-  manifestPath: string,
+  context: string | {
+    manifestPath: string;
+    manifests: string[];
+    "$cache": Record<string, string>
+  },
   options: Options,
   inputConfig = loadConfig(manifestPath, options),
   logError = error
 ): ErrorCode {

const command = await makeCommand(args, allPackages);
if (!command) {
process.exitCode = 1;
return;
Expand Down
15 changes: 10 additions & 5 deletions packages/align-deps/src/commands/check.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { updatePackageManifest } from "../manifest";
import { resolve } from "../preset";
import type { Command, ErrorCode, Options } from "../types";
import { checkPackageManifestUnconfigured } from "./vigilant";
import type { PackageInfos } from "workspace-tools";

/**
* Checks the specified package manifest for misaligned dependencies.
Expand Down Expand Up @@ -39,6 +40,7 @@ import { checkPackageManifestUnconfigured } from "./vigilant";
export function checkPackageManifest(
manifestPath: string,
options: Options,
allPackages: PackageInfos,
inputConfig = loadConfig(manifestPath, options),
logError = error
): ErrorCode {
Expand All @@ -50,7 +52,8 @@ export function checkPackageManifest(
const { devPreset, prodPreset, capabilities } = resolve(
config,
path.dirname(manifestPath),
options
options,
allPackages
);
const { kitType, manifest } = config;

Expand Down Expand Up @@ -121,10 +124,10 @@ export function checkPackageManifest(
* @param options Command line options
* @returns The check command
*/
export function makeCheckCommand(options: Options): Command {
export function makeCheckCommand(options: Options, allPackages: PackageInfos): Command {
const { presets, requirements } = options;
if (!requirements) {
return (manifest: string) => checkPackageManifest(manifest, options);
return (manifest: string) => checkPackageManifest(manifest, options, allPackages);
}

return (manifest: string) => {
Expand All @@ -139,11 +142,12 @@ export function makeCheckCommand(options: Options): Command {
const logError = (message: string) => {
output.push(message);
};
const res1 = checkPackageManifest(manifest, options, config, logError);
const res1 = checkPackageManifest(manifest, options, allPackages, config, logError);
const res2 = checkPackageManifestUnconfigured(
manifest,
options,
config,
allPackages,
logError
);
for (const message of output) {
Expand All @@ -164,7 +168,8 @@ export function makeCheckCommand(options: Options): Command {
capabilities: [],
},
manifest: readPackage(manifest),
});
},
allPackages);
}

return config;
Expand Down
12 changes: 8 additions & 4 deletions packages/align-deps/src/commands/initialize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { defaultConfig } from "../config";
import { dropPatchFromVersion, modifyManifest } from "../helpers";
import { filterPreset, mergePresets } from "../preset";
import type { Command, Options } from "../types";
import type { PackageInfos } from "workspace-tools";

function isKitType(type: string): type is KitType {
return type === "app" || type === "library";
Expand Down Expand Up @@ -42,7 +43,8 @@ export function initializeConfig(
manifest: PackageManifest,
projectRoot: string,
kitType: KitType,
{ presets }: Options
{ presets }: Options,
allPackages: PackageInfos
): PackageManifest | null {
const kitConfig = manifest["rnx-kit"];
if (kitConfig?.alignDeps) {
Expand All @@ -61,7 +63,7 @@ export function initializeConfig(
const requirements = [
`react-native@${dropPatchFromVersion(targetReactNativeVersion)}`,
];
const preset = filterPreset(mergePresets(presets, projectRoot), requirements);
const preset = filterPreset(mergePresets(presets, projectRoot, allPackages), requirements);

return {
...manifest,
Expand Down Expand Up @@ -91,7 +93,8 @@ export function initializeConfig(

export function makeInitializeCommand(
kitType: string,
options: Options
options: Options,
allPackages: PackageInfos
): Command | undefined {
if (!isKitType(kitType)) {
error(`Invalid kit type: '${kitType}'`);
Expand All @@ -108,7 +111,8 @@ export function makeInitializeCommand(
manifest,
path.dirname(manifestPath),
kitType,
options
options,
allPackages
);
if (!updatedManifest) {
return "missing-react-native";
Expand Down
8 changes: 5 additions & 3 deletions packages/align-deps/src/commands/setVersion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
Options,
} from "../types";
import { checkPackageManifest } from "./check";
import type { PackageInfos } from "workspace-tools";

function parseVersions(versions: string): string[] {
return versions.split(",").map((v) => {
Expand Down Expand Up @@ -157,7 +158,8 @@ function setVersion(
*/
export async function makeSetVersionCommand(
versions: string | number,
options: Options
options: Options,
allPackages: PackageInfos,
): Promise<Command | undefined> {
const input = await parseInput(versions);
if (!input) {
Expand All @@ -174,13 +176,13 @@ export async function makeSetVersionCommand(
return config;
}

const checkResult = checkPackageManifest(manifestPath, checkOnly, config);
const checkResult = checkPackageManifest(manifestPath, checkOnly, allPackages, config);
if (checkResult !== "success") {
return checkResult;
}

const result = setVersion(config, targetVersion, supportedVersions);
modifyManifest(manifestPath, result);
return checkPackageManifest(manifestPath, write);
return checkPackageManifest(manifestPath, write, allPackages);
};
}
14 changes: 9 additions & 5 deletions packages/align-deps/src/commands/vigilant.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {
Package,
Preset,
} from "../types";
import type { PackageInfos } from "workspace-tools";

type Report = {
changes: Changes;
Expand Down Expand Up @@ -83,11 +84,13 @@ function resolveUnmanagedCapabilities(
*/
export function buildManifestProfile(
manifestPath: string,
{ kitType, alignDeps }: AlignDepsConfig
{ kitType, alignDeps }: AlignDepsConfig,
allPackages: PackageInfos,
): ManifestProfile {
const mergedPresets = mergePresets(
alignDeps.presets,
path.dirname(manifestPath)
path.dirname(manifestPath),
allPackages
);

const [targetPreset, supportPreset] = (() => {
Expand Down Expand Up @@ -170,11 +173,11 @@ export function buildManifestProfile(
*/
const buildManifestProfileCached = ((): typeof buildManifestProfile => {
const profileCache: Record<string, ManifestProfile> = {};
return (manifestPath, config) => {
return (manifestPath, config, allPackages) => {
const { kitType, alignDeps } = config;
const key = `${kitType}:${JSON.stringify(alignDeps)}`;
if (!profileCache[key]) {
const result = buildManifestProfile(manifestPath, config);
const result = buildManifestProfile(manifestPath, config, allPackages);
profileCache[key] = result;
}

Expand Down Expand Up @@ -268,13 +271,14 @@ export function checkPackageManifestUnconfigured(
manifestPath: string,
{ excludePackages, write }: Options,
config: AlignDepsConfig,
allPackages: PackageInfos,
logError = error
): ErrorCode {
if (excludePackages?.includes(config.manifest.name)) {
return "success";
}

const manifestProfile = buildManifestProfileCached(manifestPath, config);
const manifestProfile = buildManifestProfileCached(manifestPath, config, allPackages);
const { manifest } = config;
const { changes, changesCount, unmanagedDependencies } = inspect(
manifest,
Expand Down
43 changes: 36 additions & 7 deletions packages/align-deps/src/preset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import semverValidRange from "semver/ranges/valid";
import { gatherRequirements } from "./dependencies";
import { preset as reactNativePreset } from "./presets/microsoft/react-native";
import type { AlignDepsConfig, Options, Preset } from "./types";
import path from "path";
import parsePackageName from "parse-package-name";
import type { PackageInfos } from "workspace-tools";

type Resolution = {
devPreset: Preset;
Expand All @@ -15,13 +18,35 @@ type Resolution = {
function loadPreset(
preset: string,
projectRoot: string,
resolve = require.resolve
resolve = require.resolve,
allPackages: PackageInfos
): Preset {
switch (preset) {
case "microsoft/react-native":
return reactNativePreset;
default:
return require(resolve(preset, { paths: [projectRoot] }));
default: {
const savePresetVal = preset;
let check = 1;
if(preset[0] === '/' || preset[0] === '\\') {
check = 0;
preset = preset.slice(1)
}
const parsedInfo = parsePackageName(preset);
if(check === 0) {
preset = savePresetVal;
}
if (parsedInfo.name === ".") {
console.error("Incorrect package name in the preset field");
}
const info = allPackages[parsedInfo.name];
if (info !== undefined) {
const returnResolvedPath = path.join(path.dirname(info.packageJsonPath), parsedInfo.path);
return require(returnResolvedPath);
}
else {
return require(resolve(preset, { paths: [projectRoot] }));
}
}
}
}

Expand Down Expand Up @@ -106,11 +131,12 @@ export function filterPreset(preset: Preset, requirements: string[]): Preset {
export function mergePresets(
presets: string[],
projectRoot: string,
allPackages: PackageInfos,
resolve = require.resolve
): Preset {
const mergedPreset: Preset = {};
for (const presetName of presets) {
const preset = loadPreset(presetName, projectRoot, resolve);
const preset = loadPreset(presetName, projectRoot, resolve, allPackages);
for (const [profileName, profile] of Object.entries(preset)) {
mergedPreset[profileName] = {
...mergedPreset[profileName],
Expand All @@ -122,6 +148,8 @@ export function mergePresets(
return mergedPreset;
}



/**
* Loads specified presets and filters them according to the requirements. The
* list of capabilities are also gathered from transitive dependencies if
Expand All @@ -134,14 +162,15 @@ export function mergePresets(
export function resolve(
{ kitType, alignDeps, manifest }: AlignDepsConfig,
projectRoot: string,
options: Options
options: Options,
allPackages: PackageInfos
): Resolution {
const { capabilities, presets, requirements } = alignDeps;

const prodRequirements = Array.isArray(requirements)
? requirements
: requirements.production;
const mergedPreset = mergePresets(presets, projectRoot);
const mergedPreset = mergePresets(presets, projectRoot, allPackages);
const initialProdPreset = filterPreset(mergedPreset, prodRequirements);
ensurePreset(initialProdPreset, prodRequirements);

Expand Down
3 changes: 3 additions & 0 deletions packages/align-deps/test/capabilities.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ describe("resolveCapabilities()", () => {
mergePresets(
["microsoft/react-native", "mock-custom-profiles-module"],
process.cwd(),
{},
makeMockResolver("mock-custom-profiles-module")
),
["[email protected] || 0.63 || 0.64"]
Expand Down Expand Up @@ -232,6 +233,7 @@ describe("resolveCapabilities()", () => {
mergePresets(
["microsoft/react-native", "mock-meta-package"],
process.cwd(),
{},
makeMockResolver("mock-meta-package")
),
["[email protected]"]
Expand Down Expand Up @@ -277,6 +279,7 @@ describe("resolveCapabilities()", () => {
mergePresets(
["microsoft/react-native", "mock-meta-package-loop"],
process.cwd(),
{},
makeMockResolver("mock-meta-package-loop")
),
["[email protected]"]
Expand Down
Loading
Loading