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: Fix upgrade detecting the wrong version of existing Storybooks #25752

Merged
merged 8 commits into from Jan 29, 2024

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Jan 25, 2024

Closes #25734

What I did

Changed the upgrade command to use package manager's CLI for detecting the existing version of both storybook and @storybook/cli instead of using a combination of require.resolve and reading package.json, as that would run in the global context thus not detecting the correct version as it is in the project. The packageManager.findInstallations function being used was originally develop for the doctor command but it works fine in this context too.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This can be (and has been) tested with the canary release and using the different package manager executors. Eg. in a 7.6.10 sandbox/project, running this should result in the message Upgrading Storybook from version 7.6.10 to version 0.0.0-pr-25752-sha-424630b7..

  • npx storybook@0.0.0-pr-25752-sha-424630b7 upgrade
  • yarn dlx storybook@0.0.0-pr-25752-sha-424630b7 upgrade
  • pnpm dlx storybook@0.0.0-pr-25752-sha-424630b7 upgrade
  • pnpx storybook@0.0.0-pr-25752-sha-424630b7 upgrade

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-25752-sha-424630b7. Try it out in a new sandbox by running npx storybook@0.0.0-pr-25752-sha-424630b7 sandbox or in an existing project with npx storybook@0.0.0-pr-25752-sha-424630b7 upgrade.

More information
Published version 0.0.0-pr-25752-sha-424630b7
Triggered by @JReinhold
Repository storybookjs/storybook
Branch jeppe/fix-upgrade-version-detection
Commit 424630b7
Datetime Fri Jan 26 09:33:43 UTC 2024 (1706261623)
Workflow run 7666315388

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=25752

@JReinhold JReinhold marked this pull request as ready for review January 26, 2024 10:40
Comment on lines -32 to -35
export const getStorybookCoreVersion = async () => {
const { version } = await getActualPackageVersion('@storybook/cli');
return version;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shilman this is a function in telemetry but it was only used by upgrade which doesn't use it anymore, so I felt it was okay to remove it. Let me know if not.

if (!installations) {
return;
}
const cliVersion = installations.dependencies['@storybook/cli']?.[0].version;
Copy link
Contributor

Choose a reason for hiding this comment

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

installations.dependencies['@storybook/cli']?.[0] -> Can this be undefined? If so installations.dependencies['@storybook/cli']?.[0].version will through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't, TS would complain, and the current code producing it doesn't do that either.

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

I'm assuming you tested the scenarios thoroughly via the canary, in that case LGTM!

@JReinhold JReinhold merged commit 7c21c34 into next Jan 29, 2024
57 of 58 checks passed
@JReinhold JReinhold deleted the jeppe/fix-upgrade-version-detection branch January 29, 2024 14:51
@github-actions github-actions bot mentioned this pull request Jan 29, 2024
11 tasks
@JReinhold JReinhold added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jan 30, 2024
This was referenced Jan 30, 2024
JReinhold added a commit that referenced this pull request Jan 31, 2024
…detection

CLI: Fix `upgrade` detecting the wrong version of existing Storybooks
(cherry picked from commit 7c21c34)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:normal cli patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: upgrade command always throws UpgradeStorybookToSameVersionError when using pnpx
4 participants