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 init --skip-install #28853

Merged
merged 9 commits into from
Aug 9, 2024
Merged

CLI: Fix init --skip-install #28853

merged 9 commits into from
Aug 9, 2024

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Aug 9, 2024

Closes #28819

What I did

  • I applied a hack for the modules that were being confused about ESM/CJS at bundle time.
  • Add a test, to ensure this doesn't happen in the future

We should really expediate the migration to ESM only.

Checklist for Contributors

Testing

The test can be seen here:
https://app.circleci.com/pipelines/github/storybookjs/storybook/82184/workflows/d80f14f1-291e-4558-8932-68607bbba30f/jobs/693976

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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

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

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 76.3 MB 76.3 MB 0 B -0.98 0%
initSize 167 MB 167 MB 112 B -0.72 0%
diffSize 91.1 MB 91.1 MB 112 B -0.72 0%
buildSize 7.42 MB 7.42 MB 0 B 1.13 0%
buildSbAddonsSize 1.61 MB 1.61 MB 0 B -3 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.29 MB 2.29 MB 0 B 3 0%
buildSbPreviewSize 351 kB 351 kB 0 B -3 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.45 MB 4.45 MB 0 B 3 0%
buildPreviewSize 2.97 MB 2.97 MB 0 B -0.2 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 19.3s 11.7s -7s -602ms -1.41 🔰-64.6%
generateTime 19.8s 27.5s 7.6s 1.38 🔺27.7%
initTime 16.7s 19s 2.3s -0.09 12.4%
buildTime 11.1s 12.7s 1.6s 0.12 13%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 8.7s 8.4s -270ms -0.13 -3.2%
devManagerResponsive 5.1s 5.1s -35ms -0.2 -0.7%
devManagerHeaderVisible 859ms 802ms -57ms -0.34 -7.1%
devManagerIndexVisible 896ms 836ms -60ms -0.3 -7.2%
devStoryVisibleUncached 1.2s 1.2s 14ms 0.04 1.1%
devStoryVisible 909ms 851ms -58ms -0.36 -6.8%
devAutodocsVisible 802ms 713ms -89ms -0.48 -12.5%
devMDXVisible 916ms 697ms -219ms -0.55 -31.4%
buildManagerHeaderVisible 952ms 812ms -140ms 0.14 -17.2%
buildManagerIndexVisible 958ms 814ms -144ms 0.09 -17.7%
buildStoryVisible 995ms 855ms -140ms 0.08 -16.4%
buildAutodocsVisible 870ms 724ms -146ms -0.13 -20.2%
buildMDXVisible 728ms 688ms -40ms -0.14 -5.8%

Greptile Summary

The PR addresses the --skip-install flag issue in the Storybook CLI for new SvelteKit projects, focusing on ESM bundling and resolving module export issues.

  • .circleci/config.yml: Added a job to test the --skip-install flag, ensuring it works across different scenarios.
  • code/core/scripts/prep.ts: Modified esbuild configuration to prefer ESM bundling, updated mainFields to prioritize 'module'.
  • code/core/src/cli/dirs.ts: Implemented a fallback mechanism for resolving the default export of get-npm-tarball-url, fixing ESM/CJS confusion.

Thorough testing is recommended to ensure stability and compatibility.

@ndelangen ndelangen changed the title add test Bug: CLI failure when --no-install passed fixed Aug 9, 2024
@ndelangen ndelangen changed the title Bug: CLI failure when --no-install passed fixed Bug: CLI init failure when --no-install passed fixed Aug 9, 2024
@ndelangen ndelangen self-assigned this Aug 9, 2024
@ndelangen ndelangen added bug cli core ci:daily Run the CI jobs that normally run in the daily job. labels Aug 9, 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.

LGTM

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

Copy link

nx-cloud bot commented Aug 9, 2024

☁️ Nx Cloud Report

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

@JReinhold
Copy link
Contributor

Nice!
The test only checks that init doesn't fail, does it make sense to run a smoke test or try to build the Storybook afterwards to ensure it actually produces something valid?

@benmccann I've triggered a canary release of this PR if you want to try it out.

@ndelangen ndelangen added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Aug 9, 2024
@ndelangen
Copy link
Member Author

ndelangen commented Aug 9, 2024

@JReinhold well the init step was failing before, and the same sandbox is already tested deeper elsewhere.

It seems like a total waste of time & money to test the same sandbox in the exact same way twice.

@benmccann
Copy link
Contributor

It looks like it's using get-npm-tarball-url version 2.0.3, which is 3 years old and missing an exports map in the package.json. If you upgrade to the latest 2.1.0 it might avoid you having to do the .default trick?

@ndelangen
Copy link
Member Author

We're already using version: 2.1.0 @benmccann.

I think it's because the module is type=commonjs, but I'm not sure, what exactly the problem with the package really is.

@ndelangen ndelangen merged commit e5960ff into next Aug 9, 2024
104 checks passed
@ndelangen ndelangen deleted the norbert/fix-no-install-bug branch August 9, 2024 19:46
@benmccann
Copy link
Contributor

thanks!!

@shilman shilman changed the title Bug: CLI init failure when --no-install passed fixed CLI: Fix init failure when --no-install passed fixed Aug 13, 2024
@shilman shilman changed the title CLI: Fix init failure when --no-install passed fixed CLI: Fix init failure when --no-install passed Aug 13, 2024
@shilman shilman changed the title CLI: Fix init failure when --no-install passed CLI: Fix init --no-install Aug 13, 2024
@shilman shilman changed the title CLI: Fix init --no-install CLI: Fix init --skip-install Aug 13, 2024
shilman pushed a commit that referenced this pull request Aug 13, 2024
Bug: CLI `init` failure when `--no-install` passed fixed
(cherry picked from commit e5960ff)
shilman added a commit that referenced this pull request Aug 13, 2024
Bug: CLI `init` failure when `--no-install` passed fixed
(cherry picked from commit e5960ff)
@github-actions github-actions bot added patch:done Patch/release PRs already cherry-picked to main/release branch labels Aug 13, 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 core 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]: npx sb@next init --skip-install is broken on new SvelteKit project
3 participants