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: Reduce installation noise and improve error handling #22554

Merged
merged 7 commits into from May 19, 2023

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented May 15, 2023

Closes #22343

What I did

Before, errors thrown by addDeps from our package manager proxies would always look something like this, without proper explanation:

Command failed with exit code 1: npm install -D storybook@^7.1.0-alpha.11 @storybook/react@^7.1.0-alpha.11 @storybook/react@^7.1.0-alpha.11 @storybook/react-vite@^7.1.0-alpha.11 @storybook/addon-links@^7.1.0-alpha.11 @storybook/addon-essentials@^7.1.0-alpha.11 @storybook/blocks@^7.1.0-alpha.11 @storybook/addon-interactions@^7.1.0-alpha.11 @storybook/testing-library@^0.0.14-next.2 prop-types@^15.8.1

In this PR, I do a few things:

  • Add a log stream implementation to generate log files
  • Use that implementation as part of all package manager proxies when installing dependencies. The log stream will reduce the installation noise by 100%, but I also added a "Installing dependencies" log for users to know what's going on.
  • Upon failure, each package manager will try and proxy the errors and come up with better error messages
  • For each package manager (but yarn1) there's a list of errors which will be matched, else it will show e.g. "Unknown PNPM error"
  • Add ora package to display spinner in the CLI (now that users don't see progress they might end up quitting the CLI if we don't display a spinner)

Before:
image

After (example for each package manager):
image

And an example of how the log file looks like:
image

Spinners in action:
2023-05-16 09 09 49

How to test

  1. Build the Storybook CLI
  2. Choose a project that does not have Storybook (use sandboxes repo) e.g. react-webpack
  3. run STORYBOOK_DISABLE_TELEMETRY=true storybook init command (using the built binary) in those
  4. Watch the experience

when testing the init command, you can also pass a package manager via the --package-manager flag

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@yannbf yannbf added maintenance User-facing maintenance tasks cli ci:merged Run the CI jobs that normally run when merged. labels May 15, 2023
@yannbf yannbf force-pushed the fix/reduce-package-install-noise branch from a670366 to 5709e45 Compare May 15, 2023 06:47
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.

This is awesome @yannbf -- was going to ask for this as a next step to the error handling project but you beat me to it. 😅

code/lib/cli/src/js-package-manager/NPMProxy.ts Outdated Show resolved Hide resolved
@shilman
Copy link
Member

shilman commented May 15, 2023

RE: UX can we show a CLI spinner while the installation is occurring now that we're swallowing all the installer output? Also we probably need to handle ctl+C behavior.

code/lib/cli/src/utils.ts Outdated Show resolved Hide resolved
code/lib/cli/src/utils.ts Show resolved Hide resolved
code/lib/cli/src/utils.ts Show resolved Hide resolved
@yannbf yannbf force-pushed the fix/reduce-package-install-noise branch from 04ac08c to b201226 Compare May 16, 2023 07:18
@yannbf
Copy link
Member Author

yannbf commented May 16, 2023

TODO: Move process.on('SIGINT'...) to withTelemetry and send a canceled event, something like:

export async function withTelemetry<T>(
  eventType: EventType,
  options: TelemetryOptions,
  run: () => Promise<T>
): Promise<T> {
  process.on('SIGINT', async () => {
    if (!options.cliOptions.disableTelemetry) {
      await telemetry('canceled', { eventType }, { stripMetadata: true });
    }

    // figure out how to exit properly from here
    // allowing SIGINT default handling mechanism to run
  });

  if (!options.cliOptions.disableTelemetry)
    telemetry('boot', { eventType }, { stripMetadata: true });

  try {
    return await run();
  } catch (error) {
    const { printError = logger.error } = options;
    printError(error);

    await sendTelemetryError(error, eventType, options);
    throw error;
  }
}

And make sure this yields the correct result when users do Ctrl + C in the middle of init, rather than generate an error that is unrelated!

@ndelangen ndelangen merged commit 50ad585 into next May 19, 2023
76 checks passed
@ndelangen ndelangen deleted the fix/reduce-package-install-noise branch May 19, 2023 13:52
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label May 19, 2023
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 22, 2023
shilman pushed a commit that referenced this pull request May 22, 2023
…-noise

CLI: Reduce installation noise and improve error handling
@shilman shilman mentioned this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:merged Run the CI jobs that normally run when merged. cli maintenance User-facing maintenance tasks 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.

storybook init: Issues with PackageManager proxies
4 participants