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

CLI: Instruct the correct auto-migration command #26515

Merged
merged 6 commits into from
Mar 20, 2024
Merged

CLI: Instruct the correct auto-migration command #26515

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d708857
fix https://github.com/storybookjs/storybook/issues/26478
ndelangen Mar 15, 2024
ac79433
update npx storybook command references
yannbf Mar 20, 2024
7116c05
fix
ndelangen Mar 20, 2024
1c1bb37
Merge branch 'next' into norbert/fix-mention-next-in-cli
ndelangen Mar 20, 2024
6884b4c
Merge branch 'next' into norbert/fix-mention-next-in-cli
ndelangen Mar 20, 2024
a2582ba
Remove unused import in doctor/index.ts
ndelangen Mar 20, 2024
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
9 changes: 7 additions & 2 deletions code/lib/cli/src/automigrate/helpers/getMigrationSummary.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import chalk from 'chalk';
import boxen from 'boxen';
import dedent from 'ts-dedent';
import type { InstallationMetadata } from '@storybook/core-common';
import { versions, type InstallationMetadata } from '@storybook/core-common';
import type { FixSummary } from '../types';
import { FixStatus } from '../types';
import { isPrerelease } from '../../doctor/utils';

export const messageDivider = '\n\n';
const segmentDivider = '\n\n─────────────────────────────────────────────────\n\n';
Expand Down Expand Up @@ -59,11 +60,15 @@ export function getMigrationSummary({
installationMetadata?: InstallationMetadata | null;
logFile: string;
}) {
const automigrateCommand = isPrerelease(versions.storybook)
? 'npx storybook@next automigrate'
: 'npx storybook@latest automigrate';
Copy link
Contributor

@valentinpalkovic valentinpalkovic Mar 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If someone runs npx storybook@next upgrade and after the upgrade someone wants to rerun the automigrations, shouldn't we just tell the users to call npx storybook automigrate to run the local version? This makes the most sense to me. Because people might run a specific version of an upgrade like npx [email protected] upgrade and then the above commands again do not fit, because the user wants to run the automigrate command from 8.1.0 and not from latest or next (If 8.1.0 doesn't match latest or next).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that that's a great idea, we should optimize for the common cases.
Before this PR, the suggested command to run was always with @next, this seems important to fix ASAP?

Because people might run a specific version of an upgrade like npx [email protected] upgrade and then the above commands again do not fit

True, but it's not super common to happen?
There's possibly even more edge cases?

The code is also an exact copy of how we already this the exact thing somewhere else already:

storybook/code/lib/cli/src/doctor/index.ts

Lines 144 to 146 in d52adbb

const doctorCommand = isPrerelease(storybookVersion)
? 'npx storybook@next doctor'
: 'npx storybook@latest doctor';

Copy link
Member

@yannbf yannbf Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If a user runs a command to upgrade Storybook and it fails to upgrade, then if we tell them to run npx storybook <command> that will use the non-upgraded version, a version we might not want, right?

Copy link
Contributor

@valentinpalkovic valentinpalkovic Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@yannbf I might be mistaken, but the migration summary isn't shown when the upgrade fails. It should only tell the user that the automigrate CLI can be re-executed when automigrations are skipped, but the upgrade itself was successful. Otherwise, only running the automigrate command when the upgrade fails doesn't make much sense. You don't know whether the correct dependencies are already installed or in which phase the upgrade failed.

@ndelangen, Can you elaborate on your concerns about why it isn't a good idea? I am not convinced by the arguments yet. The argument that things traditionally were already done in a specific fashion doesn't tell anything about the quality of an idea or why it is good or bad.

We explicitly worked on the idea to enable people to upgrade to specific versions of Storybook. So, we have already identified this use case as pretty important, and we want to acknowledge it and not treat it as an edge-case.

If next is 8.4 and someone upgrades to 8.3 via npx [email protected] upgrade it is a terrible idea to run automigrations from 8.4.0. I understand @yannbf argument, that 8.4 might have automigration improvement/fixes, which 8.3.0 doesn't have, on the other hand, the automigration scripts are versioned, and therefore they are coupled with the rest of Storybook's ecosystem in a particular point in time, opening the door for terrible bugs. Second, if Storybook 9 is published, and people upgrade from 7 to 8, should they run npx storybook@next automigrate or npx storybook@latest automigrate, which both point to a Storybook 9 version? I don't think that's what we want.


const messages = [];
messages.push(getGlossaryMessages(fixSummary, fixResults, logFile).join(messageDivider));

messages.push(dedent`If you'd like to run the migrations again, you can do so by running '${chalk.cyan(
'npx storybook@next automigrate'
automigrateCommand
)}'

The automigrations try to migrate common patterns in your project, but might not contain everything needed to migrate to the latest version of Storybook.
Expand Down
Loading