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 skip-install for stable latest releases #29133

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Sep 16, 2024

Closes #29112

What I did

The --skip-install flag didn't add Storybook dependencies to the package.json if the used Storybook CLI was the latest stable version. In this case, the version information for mono repo packages was dropped, and the CLI tried to add packages to the package.json with undefined version information.

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 changes cannot be tested on a canary release, but only on the latest stable release.

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-29133-sha-2f4658bd. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-29133-sha-2f4658bd
Triggered by @valentinpalkovic
Repository storybookjs/storybook
Branch valentin/fix-skip-install-installation
Commit 2f4658bd
Datetime Mon Sep 16 10:03:21 UTC 2024 (1726481001)
Workflow run 10881606660

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=29133

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 77.4 MB 77.4 MB 0 B 2.22 0%
initSize 163 MB 163 MB -14 B 1.93 0%
diffSize 85.4 MB 85.4 MB -14 B 1.9 0%
buildSize 7.57 MB 7.57 MB 0 B 0.8 0%
buildSbAddonsSize 1.66 MB 1.66 MB 0 B 0.89 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.34 MB 2.34 MB 0 B 0.82 0%
buildSbPreviewSize 352 kB 352 kB 0 B 0.65 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.55 MB 4.55 MB 0 B 0.89 0%
buildPreviewSize 3.02 MB 3.02 MB 0 B 0.11 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 25.4s 7s -18s -420ms -0.83 -262.6%
generateTime 19.7s 20.4s 687ms -0.26 3.4%
initTime 16.1s 18.1s 1.9s 0.73 11%
buildTime 10.4s 12.7s 2.2s 1.04 17.9%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 7.9s 7s -909ms 0.1 -12.8%
devManagerResponsive 5.1s 4.3s -802ms -0.21 -18.3%
devManagerHeaderVisible 874ms 813ms -61ms -0.06 -7.5%
devManagerIndexVisible 915ms 850ms -65ms -0.07 -7.6%
devStoryVisibleUncached 1.3s 1.2s -51ms -0.32 -4%
devStoryVisible 914ms 850ms -64ms -0.08 -7.5%
devAutodocsVisible 776ms 705ms -71ms -0.22 -10.1%
devMDXVisible 778ms 704ms -74ms -0.2 -10.5%
buildManagerHeaderVisible 904ms 710ms -194ms -0.67 -27.3%
buildManagerIndexVisible 944ms 718ms -226ms -0.77 -31.5%
buildStoryVisible 945ms 752ms -193ms -0.8 -25.7%
buildAutodocsVisible 898ms 778ms -120ms 0.53 -15.4%
buildMDXVisible 726ms 672ms -54ms -0.15 -8%

Greptile Summary

This PR addresses an issue with the --skip-install flag for stable latest releases of Storybook. The main changes are:

  • Updated getVersionedPackages method in JsPackageManager class to fix version handling
  • Added unit tests for getVersionedPackages to cover different scenarios
  • Ensures correct package versions are added to package.json when using latest stable Storybook CLI
  • Fixes the recurring issue where Storybook dependencies were not added to package.json with --skip-install flag
  • Improves consistency in version handling for both latest stable releases and other versions

@valentinpalkovic valentinpalkovic self-assigned this Sep 16, 2024
@valentinpalkovic valentinpalkovic added bug cli patch:yes Bugfix & documentation PR that need to be picked to main branch ci:normal labels Sep 16, 2024
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings

// @ts-expect-error Ignore abstract class error
jsPackageManager = new JsPackageManager();
jsPackageManager.latestVersion = mockLatestVersion;
(global as any).storybookPackagesVersions = mockStorybookPackagesVersions;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Avoid using (global as any) for type safety. Consider defining a custom global interface

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-skip-install-installation branch from 2f4658b to 8c58016 Compare September 16, 2024 10:06
Copy link

nx-cloud bot commented Sep 16, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 20c0b10. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

I still think we should at least build the Storybook in CI, even if it's just two lines extra to the config and not the full sandbox flow.

Extra context for anyone:
It wouldn't have caught this issue, because this issue specifically only happen with stable releases of Storybook, and our CI only runs against our prereleases.

@JReinhold
Copy link
Contributor

Correction @valentinpalkovic. It would actually have caught this issue, because we run CI on latest releases on main daily and on every release. It would have caught it after the release, but still valuable though.

@valentinpalkovic valentinpalkovic added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:normal labels Sep 17, 2024
@valentinpalkovic valentinpalkovic merged commit 57cdf15 into next Sep 17, 2024
107 checks passed
@valentinpalkovic valentinpalkovic deleted the valentin/fix-skip-install-installation branch September 17, 2024 11:57
storybook-bot pushed a commit that referenced this pull request Sep 17, 2024
…installation

CLI: Fix skip-install for stable latest releases
(cherry picked from commit 57cdf15)
storybook-bot pushed a commit that referenced this pull request Sep 19, 2024
…installation

CLI: Fix skip-install for stable latest releases
(cherry picked from commit 57cdf15)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job. 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]: storybook init --skip-install still broken
2 participants