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

CLI: Improve automigration to show prompt-only migrations #20292

Merged
merged 3 commits into from
Dec 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions code/lib/cli/src/automigrate/fixes/remove-global-client-apis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ interface GlobalClientAPIOptions {

export const removedGlobalClientAPIs: Fix<GlobalClientAPIOptions> = {
id: 'removedglobalclientapis',
promptOnly: true,

async check({ packageManager }) {
const packageJson = packageManager.retrievePackageJson();
Expand Down Expand Up @@ -48,19 +49,18 @@ export const removedGlobalClientAPIs: Fix<GlobalClientAPIOptions> = {
},
prompt({ usedAPIs, previewPath }) {
return dedent`
${chalk.bold(
chalk.red('Attention')
)}: We could not automatically make this change. You'll need to do it manually.

The following APIs (used in "${chalk.yellow(previewPath)}") have been removed from Storybook:

${usedAPIs.map((api) => `- ${chalk.cyan(api)}`).join('\n')}

You'll need to update "${chalk.yellow(previewPath)}" manually.

Please see the migration guide for more information:
${chalk.yellow(
'https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#removed-global-client-apis'
)}
`;
},
async run() {
console.log('Skipping automatic fix for removed global client APIs');
},
};
157 changes: 113 additions & 44 deletions code/lib/cli/src/automigrate/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,77 +23,129 @@ interface FixOptions {
enum FixStatus {
CHECK_FAILED = 'check_failed',
UNNECESSARY = 'unnecessary',
MANUAL_SUCCEEDED = 'manual_succeeded',
MANUAL_SKIPPED = 'manual_skipped',
SKIPPED = 'skipped',
SUCCEEDED = 'succeeded',
FAILED = 'failed',
}

type FixSummary = {
skipped: FixId[];
manual: FixId[];
succeeded: FixId[];
failed: Record<FixId, string>;
};

export const automigrate = async ({ fixId, dryRun, yes, useNpm, force }: FixOptions = {}) => {
const packageManager = JsPackageManagerFactory.getPackageManager({ useNpm, force });
const filtered = fixId ? fixes.filter((f) => f.id === fixId) : fixes;

logger.info('🔎 checking possible migrations..');
const fixResults = {} as Record<FixId, FixStatus>;
const fixSummary = { succeeded: [], failed: {} } as {
succeeded: FixId[];
failed: Record<FixId, string>;
};
const fixSummary: FixSummary = { succeeded: [], failed: {}, manual: [], skipped: [] };

for (let i = 0; i < filtered.length; i += 1) {
const f = fixes[i] as Fix;
let result;
let fixStatus = FixStatus.UNNECESSARY;

try {
result = await f.check({ packageManager });
} catch (error) {
logger.info(`⚠️ failed to check fix ${chalk.bold(f.id)}`);
fixStatus = FixStatus.CHECK_FAILED;
fixSummary.failed[f.id] = error.message;
fixResults[f.id] = FixStatus.CHECK_FAILED;
}

if (result) {
logger.info(`\n🔎 found a '${chalk.cyan(f.id)}' migration:`);
const message = f.prompt(result);

logger.info(boxen(message, { borderStyle: 'round', padding: 1, borderColor: '#F1618C' }));
logger.info(
boxen(message, {
borderStyle: 'round',
padding: 1,
borderColor: '#F1618C',
title: f.promptOnly ? 'Manual migration detected' : 'Automigration detected',
})
);

let runAnswer: { fix: boolean };

if (dryRun) {
runAnswer = { fix: false };
} else if (yes) {
runAnswer = { fix: true };
} else {
runAnswer = await prompts({
type: 'confirm',
name: 'fix',
message: `Do you want to run the '${chalk.cyan(f.id)}' migration on your project?`,
initial: true,
});
}
try {
if (dryRun) {
runAnswer = { fix: false };
} else if (yes) {
runAnswer = { fix: true };
} else if (f.promptOnly) {
fixResults[f.id] = FixStatus.MANUAL_SUCCEEDED;
fixSummary.manual.push(f.id);

if (runAnswer.fix) {
try {
await f.run({ result, packageManager, dryRun });
logger.info(`✅ ran ${chalk.cyan(f.id)} migration`);
fixStatus = FixStatus.SUCCEEDED;
fixSummary.succeeded.push(f.id);
} catch (error) {
fixStatus = FixStatus.FAILED;
fixSummary.failed[f.id] = error.message;
logger.info(`❌ error when running ${chalk.cyan(f.id)} migration:`);
logger.info(error);
logger.info();
const { shouldContinue } = await prompts(
{
type: 'toggle',
name: 'shouldContinue',
message:
'Select continue once you have made the required changes, or quit to exit the migration process',
initial: true,
active: 'continue',
inactive: 'quit',
},
{
onCancel: () => {
throw new Error();
},
}
);

if (!shouldContinue) {
fixResults[f.id] = FixStatus.MANUAL_SKIPPED;
break;
}
} else {
runAnswer = await prompts(
{
type: 'confirm',
name: 'fix',
message: `Do you want to run the '${chalk.cyan(f.id)}' migration on your project?`,
initial: true,
},
{
onCancel: () => {
throw new Error();
},
}
);
}
} else {
fixStatus = FixStatus.SKIPPED;
logger.info(`Skipping the ${chalk.cyan(f.id)} migration.`);
logger.info();
} catch (err) {
break;
}
}

fixResults[f.id] = fixStatus;
if (!f.promptOnly) {
if (runAnswer.fix) {
try {
await f.run({ result, packageManager, dryRun });
logger.info(`✅ ran ${chalk.cyan(f.id)} migration`);

fixResults[f.id] = FixStatus.SUCCEEDED;
fixSummary.succeeded.push(f.id);
} catch (error) {
fixResults[f.id] = FixStatus.FAILED;
fixSummary.failed[f.id] = error.message;

logger.info(`❌ error when running ${chalk.cyan(f.id)} migration`);
logger.info(error);
logger.info();
}
} else {
fixResults[f.id] = FixStatus.SKIPPED;
fixSummary.skipped.push(f.id);
}
}
} else {
fixResults[f.id] ||= FixStatus.UNNECESSARY;
}
}

logger.info();
Expand All @@ -103,10 +155,7 @@ export const automigrate = async ({ fixId, dryRun, yes, useNpm, force }: FixOpti
return fixResults;
};

function getMigrationSummary(
fixResults: Record<string, FixStatus>,
fixSummary: { succeeded: FixId[]; failed: Record<FixId, string> }
) {
function getMigrationSummary(fixResults: Record<string, FixStatus>, fixSummary: FixSummary) {
const hasNoFixes = Object.values(fixResults).every((r) => r === FixStatus.UNNECESSARY);
const hasFailures = Object.values(fixResults).some(
(r) => r === FixStatus.FAILED || r === FixStatus.CHECK_FAILED
Expand All @@ -121,7 +170,7 @@ function getMigrationSummary(
const successfulFixesMessage =
fixSummary.succeeded.length > 0
? `
${chalk.bold('Migrations that succeeded:')}\n\n ${fixSummary.succeeded
${chalk.bold('Successful migrations:')}\n\n ${fixSummary.succeeded
yannbf marked this conversation as resolved.
Show resolved Hide resolved
.map((m) => chalk.green(m))
.join(', ')}
`
Expand All @@ -130,19 +179,39 @@ function getMigrationSummary(
const failedFixesMessage =
Object.keys(fixSummary.failed).length > 0
? `
${chalk.bold('Migrations that failed:')}\n ${Object.entries(fixSummary.failed).reduce(
${chalk.bold('Failed migrations:')}\n ${Object.entries(fixSummary.failed).reduce(
(acc, [id, error]) => {
return `${acc}\n${chalk.redBright(id)}:\n${error}\n`;
},
''
)}
\n`
`
: '';

const manualFixesMessage =
fixSummary.manual.length > 0
? `
${chalk.bold('Manual migrations:')}\n\n ${fixSummary.manual
.map((m) =>
fixResults[m] === FixStatus.MANUAL_SUCCEEDED ? chalk.green(m) : chalk.blue(m)
)
.join(', ')}
`
: '';

const skippedFixesMessage =
fixSummary.skipped.length > 0
? `
${chalk.bold('Skipped migrations:')}\n\n ${fixSummary.skipped
.map((m) => chalk.cyan(m))
.join(', ')}
`
: '';
Comment on lines +191 to 209
Copy link
Member

Choose a reason for hiding this comment

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

the formatting on this is a bit of a nightmare, and likely only works by trial an error.

I do agree with @tmeasday that adding stories for these over time (that accurately demonstrate what is shown in the CLI ofc.) is a must-have for the future-us. Because I suspect changing something in here, will possibly change the whole formatting..

Copy link
Member Author

@yannbf yannbf Dec 15, 2022

Choose a reason for hiding this comment

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

I did experiment with making stories, but Storybook got all weird because boxen didn't work correctly in the browser:
2022-12-15 10 48 37

I would love to do this properly in the future!


const divider = hasNoFixes ? '' : '\n─────────────────────────────────────────────────\n\n';

const summaryMessage = dedent`
${successfulFixesMessage}${failedFixesMessage}${divider}If you'd like to run the migrations again, you can do so by running '${chalk.cyan(
${successfulFixesMessage}${manualFixesMessage}${failedFixesMessage}${skippedFixesMessage}${divider}If you'd like to run the migrations again, you can do so by running '${chalk.cyan(
'npx storybook@next automigrate'
)}'

Expand Down
3 changes: 2 additions & 1 deletion code/lib/cli/src/automigrate/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ export interface RunOptions<ResultType> {

export interface Fix<ResultType = any> {
id: string;
promptOnly?: boolean;
check: (options: CheckOptions) => Promise<ResultType | void>;
prompt: (result: ResultType) => string;
run: (options: RunOptions<ResultType>) => Promise<void>;
run?: (options: RunOptions<ResultType>) => Promise<void>;
Comment on lines 14 to +18
Copy link
Member

Choose a reason for hiding this comment

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

for future improvement:

This might be better as a union of 2 types, where 1 has promptOnly: true, and therefore has no run method, and another with it set to false and thus MUST have a run method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed! Let's do it later

}