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

Core: Do not add packageManager field to package.json during storybook dev #29152

Merged

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Sep 19, 2024

Closes #29146

What I did

We run pnpm --version in a try/catch block to determine whether the user has pnpm installed to detect the right package manager. I guess that since pnpm 8, pnpm will add a packageManager to the package.json if corepack is enabled and if you run the pnpm CLI, even if it is about to check the pnpm version via pnpm --version.

The fix is to set COREPACK_ENABLE_STRICT to 0 when running pnpm --version. This leads to the packageManager field not automatically being added to the user's package.json. The yarn CLI suffers under the same symptoms of adding packageManager fields to the package.json file. Therefore, it is necessary to apply the same fix there as well.

The bug was very likely introduced by #26219.

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

  1. corepack enable
  2. Run npx storybook@0.0.0-pr-29152-sha-0d238e2d init in an empty directory
  3. npm run storybook
  4. Ensure the package.json does not contain a packageManager field.

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

More information
Published version 0.0.0-pr-29152-sha-0d238e2d
Triggered by @valentinpalkovic
Repository storybookjs/storybook
Branch valentin/fix-package-manager-addition-to-package-json
Commit 0d238e2d
Datetime Thu Sep 19 09:02:18 UTC 2024 (1726736538)
Workflow run 10937820499

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

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 77.4 MB 77.4 MB 18.6 kB 4.76 0%
initSize 162 MB 162 MB 19 kB -0.24 0%
diffSize 85 MB 85 MB 372 B -0.38 0%
buildSize 7.57 MB 7.57 MB 0 B 0.49 0%
buildSbAddonsSize 1.66 MB 1.66 MB 0 B 0.57 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.34 MB 2.34 MB 0 B 0.5 0%
buildSbPreviewSize 352 kB 352 kB 0 B 0.33 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.55 MB 4.55 MB 0 B 0.57 0%
buildPreviewSize 3.02 MB 3.02 MB 0 B -0.3 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 24.3s 22.2s -2s -63ms 1.26 🔰-9.3%
generateTime 18.9s 18.6s -361ms -1.35 -1.9%
initTime 15.8s 14.6s -1s -156ms -1.82 🔰-7.9%
buildTime 10.4s 10.2s -248ms -1.26 -2.4%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 8s 5.9s -2s -98ms -1.45 🔰-35.5%
devManagerResponsive 5.1s 4s -1s -108ms -1.01 -27.7%
devManagerHeaderVisible 854ms 686ms -168ms -1.14 -24.5%
devManagerIndexVisible 897ms 716ms -181ms -1.19 -25.3%
devStoryVisibleUncached 1.3s 1.1s -219ms -0.73 -19.2%
devStoryVisible 896ms 715ms -181ms -1.2 -25.3%
devAutodocsVisible 763ms 658ms -105ms -0.78 -16%
devMDXVisible 753ms 589ms -164ms -1.56 🔰-27.8%
buildManagerHeaderVisible 982ms 652ms -330ms -1.28 🔰-50.6%
buildManagerIndexVisible 985ms 654ms -331ms -1.36 🔰-50.6%
buildStoryVisible 1s 725ms -314ms -1.15 -43.3%
buildAutodocsVisible 939ms 634ms -305ms -0.86 -48.1%
buildMDXVisible 821ms 662ms -159ms -0.38 -24%

Greptile Summary

This pull request addresses an issue where running storybook dev inadvertently added a packageManager field to the user's package.json file.

  • Modified code/core/src/common/js-package-manager/JsPackageManagerFactory.ts to set COREPACK_ENABLE_STRICT=0 when checking npm and pnpm versions
  • Prevents automatic addition of packageManager field, likely introduced by pnpm 8 with corepack enabled
  • Fixes bug reported in issue [Bug]: Storybook injecting packageManager pnpm 8.1.0 when you run npm run storybook #29146 where Storybook was injecting pnpm version information into package.json
  • Resolves potential conflicts with Chromatic GitHub Action and unwanted package manager enforcement

@valentinpalkovic valentinpalkovic added bug patch:yes Bugfix & documentation PR that need to be picked to main branch core ci:normal labels Sep 19, 2024
@valentinpalkovic valentinpalkovic self-assigned this Sep 19, 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

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

Copy link

nx-cloud bot commented Sep 19, 2024

☁️ Nx Cloud Report

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

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-package-manager-addition-to-package-json branch from a49cfdc to 0d238e2 Compare September 19, 2024 08:59
@valentinpalkovic valentinpalkovic merged commit 57ee6e1 into next Sep 19, 2024
58 of 59 checks passed
@valentinpalkovic valentinpalkovic deleted the valentin/fix-package-manager-addition-to-package-json branch September 19, 2024 09:50
storybook-bot pushed a commit that referenced this pull request Sep 19, 2024
…er-addition-to-package-json

Core: Do not add packageManager field to package.json during `storybook dev`
(cherry picked from commit 57ee6e1)
@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:normal 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]: Storybook injecting packageManager pnpm 8.1.0 when you run npm run storybook
2 participants