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.

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

[Upgrade Tool] Chores / Quality of Life Improvements #19053

Merged
merged 10 commits into from Dec 15, 2023
29 changes: 17 additions & 12 deletions packages/utils/upgrade/src/cli/commands/upgrade.ts
Expand Up @@ -8,12 +8,28 @@ import type { Command } from '../types';

export const upgrade: Command = async (options) => {
try {
const logger = loggerFactory({ silent: options.silent, debug: options.debug });
const { silent, debug, yes } = options;
const logger = loggerFactory({ silent, debug });

logger.warn(
"Please make sure you've created a backup of your codebase and files before upgrading"
);

const confirm = async (message: string) => {
if (yes) {
return true;
}

const { confirm } = await prompts({
name: 'confirm',
type: 'confirm',
message,
});

// If confirm is undefined (Ctrl + C), default to false
return confirm ?? false;
};

await tasks.upgrade({
logger,
confirm,
Expand All @@ -25,14 +41,3 @@ export const upgrade: Command = async (options) => {
handleError(err);
}
};

const confirm = async (message: string) => {
const { confirm } = await prompts({
name: 'confirm',
type: 'confirm',
message,
});

// If confirm is undefined (Ctrl + C), default to false
return confirm ?? false;
};
5 changes: 5 additions & 0 deletions packages/utils/upgrade/src/cli/index.ts
Expand Up @@ -15,6 +15,11 @@ const addReleaseUpgradeCommand = (releaseType: Version.ReleaseType, description:
.option('-n, --dry', 'Simulate the upgrade without updating any files', false)
.option('-d, --debug', 'Get more logs in debug mode', false)
.option('-s, --silent', "Don't log anything", false)
.option(
'-y, --yes',
Copy link
Contributor

Choose a reason for hiding this comment

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

We use "force" everywhere else, like data-transfer. The only reason I would switch to "yes" is if we consider force="yes to all", yes="yes to things that are non-destructive" (and thus have both available here)

Copy link
Member Author

Choose a reason for hiding this comment

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

For me, it doesn't mean the same thing. Force means "go for it even if we have weird stuff on the way" whereas "yes" means... answering "yes" to all prompts where you can answer "yes". I don't know if that makes sense though

Copy link
Member Author

Choose a reason for hiding this comment

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

Although for now, I agree it is similar, since the only things we're answering to are optional requirements

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it would technically be 'yes' in this case and then we would introduce 'force' if we encountered something like "run this codemod even if it's unpredictable based on your code" but my worry is that we already use force for data-transfer and now we'd have two different wordings that don't really make that distinction clear.

@marcoautiero your thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

but my worry is that we already use force for data-transfer and now we'd have two different wordings that don't really make that distinction clear.

I agree, it was my worry too. But I believe that in the long run having well-documented atomic options/flags, is more helpful than trying to accommodate everything under a big one-do-it-all

Also, yes is quite a well-known flag since it's already used quite a lot by package managers & other CLI. So I don't think people would be lost

Copy link
Member

Choose a reason for hiding this comment

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

We use force to ignore prompts with experimental commands in the strapi CLI by the way. I originally wrote yes but Ben highlighted that we use force in DTS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we do use force in the DTS, but again, I feel like it's not aiming at doing the same thing 😕
I don't really see why both couldn't live together

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a problem with yes, but imo if you are ignoring prompts with yes i think you need to change the CLI too because that is doing the same thing as here, otherwise it's just an inconsistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2023-12-14 at 16 28 16
Do we have potentially destructive effects here?

Copy link
Member Author

@Convly Convly Dec 14, 2023

Choose a reason for hiding this comment

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

Not in the current prompts

'Automatically answer "yes" to any prompts that the CLI might print on the command line.',
false
)
.action(async (options: CLIOptions) => {
const { upgrade } = await import('./commands/upgrade.js');

Expand Down
1 change: 1 addition & 0 deletions packages/utils/upgrade/src/cli/types.ts
Expand Up @@ -5,6 +5,7 @@ export interface CLIOptions {
dry: boolean;
debug: boolean;
silent: boolean;
yes: boolean;

projectPath?: string;
}
Expand Down
11 changes: 7 additions & 4 deletions packages/utils/upgrade/src/modules/format/formats.ts
Expand Up @@ -5,19 +5,22 @@ import { constants as timerConstants } from '../timer';

import type { Version } from '../version';
import type { Report } from '../report';
import { isSemVer } from '../version';

export const path = (path: string) => chalk.blue(path);

export const version = (version: Version.LiteralVersion | Version.SemVer) => {
return chalk.italic.yellow(isSemVer(version) ? version.raw : version);
return chalk.italic.yellow(`v${version}`);
};

export const versionRange = (range: string) => chalk.bold.green(range);
export const versionRange = (range: Version.Range) => chalk.italic.yellow(range);

export const transform = (transformFilePath: string) => chalk.cyan(transformFilePath);

export const highlight = (text: string) => chalk.bold.underline(text);
export const highlight = (arg: unknown) => chalk.bold.underline(arg);

export const upgradeStep = (text: string, step: [current: number, total: number]) => {
return chalk.bold(`(${step[0]}/${step[1]}) ${text}...`);
};

export const reports = (reports: Report.CodemodReport[]) => {
const rows = reports.map(({ codemod, report }, i) => {
Expand Down
66 changes: 55 additions & 11 deletions packages/utils/upgrade/src/modules/upgrader/upgrader.ts
@@ -1,4 +1,5 @@
import assert from 'node:assert';
import chalk from 'chalk';
import { packageManager } from '@strapi/utils';

import {
Expand All @@ -21,9 +22,9 @@ import type { Project } from '../project';
type DependenciesEntries = Array<[name: string, version: Version.LiteralSemVer]>;

export class Upgrader implements UpgraderInterface {
private project: Project;
private readonly project: Project;

private npmPackage: NPM.Package;
private readonly npmPackage: NPM.Package;

private target: Version.SemVer;

Expand Down Expand Up @@ -79,19 +80,37 @@ export class Upgrader implements UpgraderInterface {
}

async upgrade(): Promise<UpgradeReport> {
if (this.isDry) {
this.logger?.warn(
'Running the upgrade in dry mode. No files will be modified during the process.'
);
}
this.logger?.debug(
`Upgrading from ${f.version(this.project.strapiVersion)} to ${f.version(this.target)}`
);

const range = rangeFromVersions(this.project.strapiVersion, this.target);
const npmVersionsMatches = this.npmPackage?.findVersionsInRange(range) ?? [];

this.logger?.debug(
`Found ${f.highlight(npmVersionsMatches.length)} versions satisfying ${f.versionRange(range)}`
);

try {
this.logger?.info(f.upgradeStep('Checking requirement', [1, 4]));
await this.checkRequirements(this.requirements, {
npmVersionsMatches,
project: this.project,
target: this.target,
});

this.logger?.info(f.upgradeStep('Upgrading Strapi dependencies', [2, 4]));
await this.updateDependencies();

this.logger?.info(f.upgradeStep('Installing dependencies', [3, 4]));
await this.installDependencies();

this.logger?.info(f.upgradeStep('Applying the latest code modifications', [4, 4]));
await this.runCodemods(range);
} catch (e) {
return erroredReport(unknownToError(e));
Expand Down Expand Up @@ -130,22 +149,25 @@ export class Upgrader implements UpgraderInterface {
requirement: Requirement.Requirement,
originalError: Error
): Promise<void> {
const errorMessage = `Upgrade requirement "${requirement.name}" failed: ${originalError.message}`;
const confirmationMessage = `Optional requirement "${requirement.name}" failed with "${originalError.message}", do you want to proceed anyway?`;
const errorMessage = `Requirement failed: ${originalError.message} (${f.highlight(
requirement.name
)})`;
const warningMessage = originalError.message;
const confirmationMessage = `Ignore optional requirement "${f.highlight(requirement.name)}" ?`;

const error = new Error(errorMessage);

if (requirement.isRequired) {
throw error;
}

this.logger?.warn(warningMessage);

const response = await this.confirmationCallback?.(confirmationMessage);

if (!response) {
throw error;
}

this.logger?.warn(errorMessage);
}

private async updateDependencies(): Promise<void> {
Expand All @@ -156,6 +178,11 @@ export class Upgrader implements UpgraderInterface {
const dependencies = json.get<Record<string, string>>('dependencies', {});
const strapiDependencies = this.getScopedStrapiDependencies(dependencies);

this.logger?.debug(`Found ${f.highlight(strapiDependencies.length)} dependency(ies) to update`);
strapiDependencies.forEach((dependency) =>
this.logger?.debug(`- ${dependency[0]} (${dependency[1]} -> ${this.target})`)
);

if (strapiDependencies.length === 0) {
return;
}
Expand All @@ -164,9 +191,12 @@ export class Upgrader implements UpgraderInterface {

const updatedPackageJSON = json.root();

if (!this.isDry) {
await saveJSON(packageJSONPath, updatedPackageJSON);
if (this.isDry) {
this.logger?.debug(`Skipping dependencies update (${chalk.italic('dry mode')}`);
return;
}

await saveJSON(packageJSONPath, updatedPackageJSON);
}

private getScopedStrapiDependencies(dependencies: Record<string, string>): DependenciesEntries {
Expand All @@ -192,6 +222,13 @@ export class Upgrader implements UpgraderInterface {

const packageManagerName = await packageManager.getPreferred(projectPath);

this.logger?.debug(`Using ${f.highlight(packageManagerName)} as package manager`);

if (this.isDry) {
this.logger?.debug(`Skipping dependencies installation (${chalk.italic('dry mode')}`);
return;
}

await packageManager.installDependencies(projectPath, packageManagerName, {
stdout: this.logger?.stdout,
stderr: this.logger?.stderr,
Expand All @@ -210,10 +247,15 @@ export class Upgrader implements UpgraderInterface {

const hasCodemodsToRun = versionedCodemods.length > 0;
if (!hasCodemodsToRun) {
this.logger?.debug(`Found no codemods to run for ${this.target}`);
this.logger?.debug(`Found no codemods to run for ${f.version(this.target)}`);
return;
}

this.logger?.debug(`Found codemods for ${f.highlight(versionedCodemods.length)} version(s)`);
versionedCodemods.forEach(({ version, codemods }) =>
this.logger?.debug(`- ${f.version(version)} (${codemods.length})`)
);

// Flatten the collection to a single list of codemods, the original list should already be sorted
const codemods = versionedCodemods.map(({ codemods }) => codemods).flat();

Expand All @@ -234,12 +276,14 @@ export const upgraderFactory = (
// The targeted version is the latest one that matches the given range
const targetedNPMVersion = npmVersionsMatches.at(-1);

assert(targetedNPMVersion, `No available version found for ${range}`);
assert(targetedNPMVersion, `Could not find any version in the range ${f.versionRange(range)}`);

// Make sure the latest version matched in the range is the same as the targeted one (only if target is a semver)
if (isSemVer(target) && target.raw !== targetedNPMVersion.version) {
throw new Error(
`${target} doesn't exist on the registry. Closest one found is ${targetedNPMVersion.version}`
`${f.version(target)} doesn't exist on the registry. Closest version found is ${
targetedNPMVersion.version
}`
);
}

Expand Down
Expand Up @@ -11,7 +11,7 @@ export const REQUIRE_GIT_CLEAN_REPOSITORY = requirementFactory(

if (!status.isClean()) {
throw new Error(
'Repository is not clean. Please commit or stash any changes before upgrading'
'Repository is not clean. Please consider committing or stashing any changes before upgrading'
Convly marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions packages/utils/upgrade/src/tasks/upgrade/upgrade.ts
Expand Up @@ -28,8 +28,8 @@ export const upgrade = async (options: UpgradeOptions) => {

if (options.target === Version.ReleaseType.Major) {
upgrader
.addRequirement(requirements.major.REQUIRE_AVAILABLE_NEXT_MAJOR.asOptional())
.addRequirement(requirements.major.REQUIRE_LATEST_FOR_CURRENT_MAJOR.asOptional());
.addRequirement(requirements.major.REQUIRE_AVAILABLE_NEXT_MAJOR)
.addRequirement(requirements.major.REQUIRE_LATEST_FOR_CURRENT_MAJOR);
}

upgrader.addRequirement(requirements.common.REQUIRE_GIT.asOptional());
Expand Down