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

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Dec 15, 2022

Issue: N/A

What I did

Manual migrations are now using promptOnly, and provide a different title, which shows whether migrations are "auto" or "manual":

image

Additionally, the summary provides now "skipped migrations" and "manual migrations". Manual migrations that are "accepted" will be shown in blue, and when the user selects "quit", they will be shown in red.

image

This PR also makes the automigration be cancellable via ctrl + C, which was not the case before (it still provides a summary at the end).

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Comment on lines 14 to +18
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>;
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

Comment on lines +191 to 209
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(', ')}
`
: '';
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!

@shilman shilman changed the title CLI: improve automigration to show prompt-only migrations CLI: Improve automigration to show prompt-only migrations Dec 16, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Great! 💪

@shilman shilman merged commit cdcc385 into next Dec 16, 2022
@shilman shilman deleted the feat/automigration-manual-migrations branch December 16, 2022 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants