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

Addon Test: Improve messages and post install script handling #29036

Merged
merged 18 commits into from
Sep 5, 2024

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Sep 3, 2024

Closes #

What I did

This PR does revamps the postinstall script from addon test, improving its messages as well as error handling.

Before:
image

After:

1 - Successful scenario:
image

2 - Failure scenario with early bail (slightly outdated):
image

3 - Partial failure scenario (slightly outdated):
image

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 section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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-29036-sha-b6b6c248. Try it out in a new sandbox by running npx storybook@0.0.0-pr-29036-sha-b6b6c248 sandbox or in an existing project with npx storybook@0.0.0-pr-29036-sha-b6b6c248 upgrade.

More information
Published version 0.0.0-pr-29036-sha-b6b6c248
Triggered by @yannbf
Repository storybookjs/storybook
Branch cli/improve-sb-add-messages
Commit b6b6c248
Datetime Wed Sep 4 09:39:22 UTC 2024 (1725442762)
Workflow run 10699124681

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

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 76.5 MB 76.5 MB 0 B 1.31 0%
initSize 161 MB 161 MB 0 B -1.52 0%
diffSize 84.6 MB 84.6 MB 0 B -1.63 0%
buildSize 7.48 MB 7.48 MB 0 B -0.94 0%
buildSbAddonsSize 1.62 MB 1.62 MB 0 B 1.54 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.31 MB 2.31 MB 0 B -1.53 0%
buildSbPreviewSize 352 kB 352 kB 0 B - 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.47 MB 4.47 MB 0 B 1.5 0%
buildPreviewSize 3 MB 3 MB 0 B -0.99 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 23.9s 8.5s -15s -393ms -0.83 -180.2%
generateTime 18.9s 26.1s 7.2s 1.67 🔺27.7%
initTime 15.1s 21.5s 6.3s 1.56 🔺29.4%
buildTime 12.1s 12.9s 779ms 0.49 6%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 7.2s 7s -207ms 0.06 -2.9%
devManagerResponsive 4.8s 4.6s -200ms 0.22 -4.3%
devManagerHeaderVisible 849ms 830ms -19ms 0.36 -2.3%
devManagerIndexVisible 890ms 876ms -14ms 0.4 -1.6%
devStoryVisibleUncached 1.4s 1.3s -114ms -0.13 -8.6%
devStoryVisible 889ms 874ms -15ms 0.39 -1.7%
devAutodocsVisible 831ms 686ms -145ms -0.25 -21.1%
devMDXVisible 800ms 759ms -41ms 0.71 -5.4%
buildManagerHeaderVisible 816ms 757ms -59ms 0.07 -7.8%
buildManagerIndexVisible 817ms 760ms -57ms -0.09 -7.5%
buildStoryVisible 905ms 796ms -109ms -0.18 -13.7%
buildAutodocsVisible 797ms 678ms -119ms -0.23 -17.6%
buildMDXVisible 753ms 726ms -27ms 0.83 -3.7%

Greptile Summary

This PR enhances the postinstall script for the Storybook Vitest addon, improving user experience with better messaging and error handling.

  • Added code/addons/test/src/postinstall-logger.ts for improved console output with boxed messages and color-coding
  • Updated code/addons/test/src/postinstall.ts with more thorough compatibility checks and informative error messages
  • Modified code/lib/cli-storybook/src/add.ts to provide clearer instructions and better handling of already installed addons
  • Added new devDependencies (@types/semver, boxen, semver) in code/addons/test/package.json to support enhanced functionality

Copy link

nx-cloud bot commented Sep 3, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8170e25. 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

@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.

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

Comment on lines +5 to +6
const fancy =
process.platform !== 'win32' || process.env.CI || process.env.TERM === 'xterm-256color';
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a more descriptive variable name than 'fancy', such as 'useFancyCharacters' or 'useUnicodeSymbols'

Comment on lines +24 to +34
export const printInfo = (title: string, message: string, options?: Options) =>
print(message, { borderColor: 'blue', title, ...options });

export const printWarning = (title: string, message: string, options?: Options) =>
print(message, { borderColor: 'yellow', title, ...options });

export const printError = (title: string, message: string, options?: Options) =>
print(message, { borderColor: 'red', title, ...options });

export const printSuccess = (title: string, message: string, options?: Options) =>
print(message, { borderColor: 'green', title, ...options });
Copy link
Contributor

Choose a reason for hiding this comment

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

style: These functions are very similar. Consider using a single function with an enum parameter for the message type to reduce code duplication

Comment on lines 24 to 27
const addonName = '@storybook/experimental-addon-vitest';
const dependencies = ['vitest', '@vitest/browser', 'playwright'];
const optionalDependencies = ['@vitest/coverage-istanbul', '@vitest/coverage-v8'];
const extensions = ['.js', '.jsx', '.ts', '.tsx', '.cts', '.mts', '.cjs', '.mjs'];
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a constant for the addon name and dependencies to improve maintainability

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.

Love the focus on UX here!

Added some suggestions to make it easier for users to follow along, feel free to ignore any of them.

code/addons/test/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/test/src/postinstall.ts Show resolved Hide resolved
code/addons/test/src/postinstall.ts Show resolved Hide resolved
code/addons/test/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/test/src/postinstall.ts Outdated Show resolved Hide resolved
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 improvements.

One meta comment is that there are inconsistent references to the addon, as either the "the Test Addon", "the Vitest Addon", or "the Storybook Addon @storybook/experimental-addon-test".

I propose we refer to the product as "Storybook Test" which reads better. We also do this for "Storybook Docs", although that one was unfortunate because it can also refer to the project documentation.

code/addons/test/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/test/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/test/src/postinstall.ts Outdated Show resolved Hide resolved
code/addons/test/src/postinstall.ts Outdated Show resolved Hide resolved
code/lib/cli-storybook/src/add.ts Outdated Show resolved Hide resolved
@yannbf yannbf merged commit c8e47c5 into next Sep 5, 2024
4 checks passed
@yannbf yannbf deleted the cli/improve-sb-add-messages branch September 5, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants