Skip to content

Refactor: Update getVersionedPackages method to handle non-Storybook packages correctly - #35769

Merged
valentinpalkovic merged 1 commit into
nextfrom
valentin/handle-non-storybook-packages-correctly
Aug 5, 2026
Merged

valentinpalkovic merged 1 commit into
nextfrom
valentin/handle-non-storybook-packages-correctly

Conversation

@valentinpalkovic

@valentinpalkovic valentinpalkovic commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Closes #

What I did

This PR adds third-party packages during Storybook init with the latest tag to the user's package.json to make sure that an installation constrained by npmMinimalAgeGate is actually respected and the init passes.

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

Caution

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-storybook/src/sandbox-templates.ts

  • Declare whether manual QA will be needed for this PR during the next release, through qa:needed or qa:skip

  • 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 publish.yml --field pr=<PR_NUMBER>

@valentinpalkovic
valentinpalkovic requested a review from a team August 5, 2026 12:20
@github-actions

github-actions Bot commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor
Fails
🚫 The "#### Manual testing" section must be filled in. Please describe how to test the changes you've made, step by step, so that reviewers can confirm your PR works as intended.

Generated by 🚫 dangerJS against 8bea716

@ndelangen ndelangen self-assigned this Aug 5, 2026
@valentinpalkovic valentinpalkovic added the build Internal-facing build tooling & test updates label Aug 5, 2026
@valentinpalkovic valentinpalkovic added bug and removed build Internal-facing build tooling & test updates labels Aug 5, 2026
@ndelangen ndelangen moved this to In Progress in Core Team Projects Aug 5, 2026
@valentinpalkovic
valentinpalkovic merged commit 3f67ae4 into next Aug 5, 2026
12 of 17 checks passed
@valentinpalkovic
valentinpalkovic deleted the valentin/handle-non-storybook-packages-correctly branch August 5, 2026 12:22
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Core Team Projects Aug 5, 2026
@ndelangen ndelangen added patch:yes Bugfix & documentation PR that need to be picked to main branch ci:normal Run our default set of CI jobs (choose this for most PRs). qa:skip Pull Requests that do not need any QA. (e.g. documentation) labels Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

getVersionedPackages now leaves all non-Storybook package identifiers unchanged. Documentation describes package-manager resolution during installation. A test verifies the behavior.

Changes

Package versioning

Layer / File(s) Summary
Preserve non-Storybook packages
code/core/src/common/js-package-manager/JsPackageManager.ts, code/core/src/common/js-package-manager/JsPackageManager.test.ts
getVersionedPackages bypasses version resolution for non-Storybook packages. Documentation and test coverage reflect the unchanged package identifiers.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
code/core/src/common/js-package-manager/JsPackageManager.test.ts (1)

50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for versioned external package strings.

This test covers only an unversioned package. Add a case such as some-other-package@1.2.3 and assert that the exact input string is returned unchanged.

As per coding guidelines, tests must cover real behavior and public contracts.

Suggested test update
-    const result = await jsPackageManager.getVersionedPackages(['some-other-package']);
+    const packages = ['some-other-package', 'some-other-package@1.2.3'];
+    const result = await jsPackageManager.getVersionedPackages(packages);

-    expect(result).toEqual(['some-other-package']);
+    expect(result).toEqual(packages);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/core/src/common/js-package-manager/JsPackageManager.test.ts` around
lines 50 - 54, The current getVersionedPackages test only covers an unversioned
external package, so add a case for an already versioned input like
some-other-package@1.2.3 and assert it is returned unchanged. Update
JsPackageManager.test.ts in the getVersionedPackages coverage, reusing the
existing jsPackageManager setup and the same public-contract expectation for
non-Storybook packages. Keep the existing unversioned-package assertion, and add
the new case so the method’s passthrough behavior for versioned external package
strings is verified.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@code/core/src/common/js-package-manager/JsPackageManager.ts`:
- Around line 443-453: Update the return documentation for the method shown
above to describe results as package install specifiers rather than claiming
every result includes a package version. State that only Storybook monorepo
packages are rewritten, while non-Storybook inputs are returned unchanged.
- Line 463: Replace the inherited-property check using `in` in the package
lookup logic with an own-property check on `storybookPackagesVersions`,
preventing names such as constructor, toString, and __proto__ from matching.
Apply the same change in getVersion while preserving the existing behavior for
explicitly defined package entries.

---

Nitpick comments:
In `@code/core/src/common/js-package-manager/JsPackageManager.test.ts`:
- Around line 50-54: The current getVersionedPackages test only covers an
unversioned external package, so add a case for an already versioned input like
some-other-package@1.2.3 and assert it is returned unchanged. Update
JsPackageManager.test.ts in the getVersionedPackages coverage, reusing the
existing jsPackageManager setup and the same public-contract expectation for
non-Storybook packages. Keep the existing unversioned-package assertion, and add
the new case so the method’s passthrough behavior for versioned external package
strings is verified.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ea8968c0-a27a-4707-9858-9ccd9e0e16c4

📥 Commits

Reviewing files that changed from the base of the PR and between 5115da7 and 8bea716.

📒 Files selected for processing (2)
  • code/core/src/common/js-package-manager/JsPackageManager.test.ts
  • code/core/src/common/js-package-manager/JsPackageManager.ts

Comment on lines +443 to +453
* Return an array of strings matching following format: `<storybook_package_name>@<package_latest_version>`
*
* For packages in the storybook monorepo, when the latest version is equal to the version of the
* current CLI the version is not added to the string.
*
* When a package is in the monorepo, and the version is not equal to the CLI version, the version
* is taken from the versions.ts file and added to the string.
*
* When a package is not in the monorepo, we don't change the package version and return the package name as is.
* The package manager will resolve the latest version of the package upon installing it.
*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the return-format documentation.

The description at Line 443 says every result matches <storybook_package_name>@<package_latest_version>. The method now also returns the complete non-Storybook input unchanged. Use “package install specifier” for the return value and state that only Storybook packages are rewritten.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/core/src/common/js-package-manager/JsPackageManager.ts` around lines 443
- 453, Update the return documentation for the method shown above to describe
results as package install specifiers rather than claiming every result includes
a package version. State that only Storybook monorepo packages are rewritten,
while non-Storybook inputs are returned unchanged.

// If the packageVersion is specified and we are not dealing with a storybook package,
// just return the requested version.
if (packageVersion && !(packageName in storybookPackagesVersions)) {
if (!(packageName in storybookPackagesVersions)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 6 '\bstorybookPackagesVersions\b' code/core/src --glob '*.ts'

Repository: storybookjs/storybook

Length of output: 7662


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- code/core/src/versions.ts ---'
cat -n code/core/src/versions.ts

printf '%s\n' '--- relevant package-manager implementation ---'
sed -n '440,525p' code/core/src/common/js-package-manager/JsPackageManager.ts

Repository: storybookjs/storybook

Length of output: 251


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- version files ---'
git ls-files '*versions.ts' '*versions.tsx' | head -80

printf '%s\n' '--- relevant implementation ---'
sed -n '440,525p' code/core/src/common/js-package-manager/JsPackageManager.ts

Repository: storybookjs/storybook

Length of output: 3499


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- code/core/src/common/versions.ts ---'
cat -n code/core/src/common/versions.ts

printf '%s\n' '--- references to the exported map ---'
rg -n -C 3 'storybookPackagesVersions|export default|Object\.create|Object\.freeze' code/core/src/common/versions.ts code/core/src/common --glob '*.ts'

Repository: storybookjs/storybook

Length of output: 15646


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- getPackageDetails definition and usages ---'
rg -n -C 8 'getPackageDetails' code/core/src/common/js-package-manager/JsPackageManager.ts

printf '%s\n' '--- package-name parsing tests or callers ---'
rg -n -C 4 'getVersionedPackages|getVersion\(' code/core/src/common/js-package-manager --glob '*.{ts,tsx}'

printf '%s\n' '--- prototype-membership behavioral probe ---'
node - <<'JS'
const storybookPackagesVersions = {
  '`@storybook/react`': '10.6.0-alpha.4',
  storybook: '10.6.0-alpha.4',
};
for (const packageName of ['`@storybook/react`', 'constructor', 'toString', '__proto__', 'not-a-package']) {
  console.log(JSON.stringify({
    packageName,
    inOperator: packageName in storybookPackagesVersions,
    ownProperty: Object.hasOwn(storybookPackagesVersions, packageName),
    value: storybookPackagesVersions[packageName],
  }));
}
JS

Repository: storybookjs/storybook

Length of output: 12643


Replace in with an own-property check for storybookPackagesVersions. The map is a normal object, so constructor, toString, and __proto__ match inherited properties. Apply the same fix in getVersion at line 510.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/core/src/common/js-package-manager/JsPackageManager.ts` at line 463,
Replace the inherited-property check using `in` in the package lookup logic with
an own-property check on `storybookPackagesVersions`, preventing names such as
constructor, toString, and __proto__ from matching. Apply the same change in
getVersion while preserving the existing behavior for explicitly defined package
entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal Run our default set of CI jobs (choose this for most PRs). 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 qa:skip Pull Requests that do not need any QA. (e.g. documentation)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants