CLI: Change yarn package manager value to yarn1#33099
Conversation
|
View your CI Pipeline Execution ↗ for commit 984cb43
☁️ Nx Cloud last updated this comment at |
valentinpalkovic
force-pushed
the
valentin/fix-package-manager-yarn-value
branch
from
aeb4de8 to
3a54386
Compare
📝 WalkthroughWalkthroughUpdated the package manager identifier for Yarn v1 and replaced string-literal usages with the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Enum as PackageManagerName
participant Factory as JsPackageManagerFactory
participant Postinstall as addons/postinstall
participant Shell
Note over Enum: Yarn v1 identifier changed to "yarn1"
User->>CLI: run command --package-manager <type>
CLI->>Enum: use Object.values(PackageManagerName) for choices
CLI->>CLI: validate package-manager via Option.choices(...)
CLI->>Factory: resolve manager using enum key
CLI->>Postinstall: invoke postinstall(packageManager)
Postinstall->>Enum: map enum -> command string
Postinstall->>Shell: spawn mapped command
Shell-->>Postinstall: exit/status
Postinstall-->>CLI: return result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
22-28: Enum literal change forYARN1may need explicit back‑compat handlingChanging
PackageManagerName.YARN1from'yarn'to'yarn1'alters the serialized value for this package manager. Any existing configs, CLI flags, stored metadata, or external integrations that still use'yarn'could now fail to match or be misdetected.Consider either:
- Accepting both
'yarn'and'yarn1'in CLI/config and normalizing toPackageManagerName.YARN1, or- Introducing a separate alias/normalize layer so the enum value change doesn’t silently break older inputs.
Please double‑check all consumers (CLI options, config parsing, telemetry, docs) for references to
'yarn'vs'yarn1'to ensure this is intentionally breaking or fully covered. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
code/core/src/common/js-package-manager/JsPackageManager.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/sandbox/generate.ts (1)
113-113: UsingPackageManagerName.YARN1for the--package-managerflag is correct and future‑proofPassing
--package-manager=${PackageManagerName.YARN1}keeps this script consistent with the central mapping (now'yarn1') and avoids drift if the underlying literal ever changes; no issues spotted. If we later support other package managers for sandboxes, this spot would be a natural extension point.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)scripts/sandbox/generate.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/common/js-package-manager/JsPackageManager.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
scripts/sandbox/generate.ts
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Applied to files:
scripts/sandbox/generate.ts
🧬 Code graph analysis (1)
scripts/sandbox/generate.ts (1)
code/core/src/core-server/presets/common-preset.ts (1)
env(145-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
scripts/sandbox/generate.ts (1)
15-15: ImportingPackageManagerNamekeeps sandbox tooling aligned with core PM semanticsPulling the enum from
js-package-managerremoves a magic string source of truth here and should track future changes to package-manager naming automatically. Looks good as-is.Based on learnings
valentinpalkovic
force-pushed
the
valentin/fix-package-manager-yarn-value
branch
2 times, most recently
from
8a9d4b5 to
f95810c
Compare
valentinpalkovic
force-pushed
the
valentin/fix-package-manager-yarn-value
branch
from
f95810c to
f58366c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
code/lib/cli-storybook/src/bin/run.ts (3)
116-119: Consistent package-manager choices foraddcommand
addnow shares the same enum-backed--package-manageroption asinit, which keeps behavior and validation consistent across commands. No further issues here beyond the BC/docs considerations already mentioned forinit.
264-268:automigratepackage-manager option consistent with other commands
automigratenow shares the same validated--package-managerchoices asinit/add/upgrade, which is good for predictability and error handling. No additional issues beyond the shared BC/docs considerations.
287-291:doctorpackage-manager option matches the new enum-based pattern
doctorusing the sameOption('--package-manager <type>').choices(Object.values(PackageManagerName))pattern keeps all CLI entry points aligned on the same set of allowed package managers, reducing configuration drift. Looks good.
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/bin/run.ts (1)
92-96: Typed--package-managerwith enum choices improves safety; check BC & dedup opportunityUsing:
.addOption( new Option('--package-manager <type>', 'Force package manager for installing deps').choices( Object.values(PackageManagerName) ) )is a solid improvement: it constrains the value to the enum, keeps the CLI in lockstep with
PackageManagerName(includingyarn1), and makes the allowed values visible in--help.Two follow-ups to consider:
- This will now reject any previously used ad-hoc values like
--package-manager yarnifPackageManagerNamehas been changed toyarn1. If older docs/scripts still mentionyarn, you may want either:
- a deprecation period with an alias, or
- explicit release notes stating
yarn1is now the required value.- The same
Option(...).choices(Object.values(PackageManagerName))pattern is repeated across several commands; a small helper likecreatePackageManagerOption()returning a newOptioninstance would reduce duplication and keep description/choices perfectly in sync.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)code/lib/cli-storybook/src/bin/run.ts(8 hunks)scripts/sandbox/generate.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/sandbox/generate.ts
- code/core/src/common/js-package-manager/JsPackageManager.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
code/lib/cli-storybook/src/bin/run.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/lib/cli-storybook/src/bin/run.ts (4)
5-5: Enum-based package manager import keeps CLI in sync with coreImporting
PackageManagerNamehere is the right way to drive CLI values from the single source of truth instorybook/internal/common, so the--package-managerflag stays aligned with core (including the newyarn1literal). Looks good.
141-144:removecommand wiring aligns with factory usageThe enum-based
--package-manageroption forremoveis consistent with other commands and feeds into:const packageManager = JsPackageManagerFactory.getPackageManager({ configDir: options.configDir, force: options.packageManager, });which can now rely on
options.packageManagerbeing one of the knownPackageManagerNamevalues. Looks correct; just ensureJsPackageManagerFactoryis updated to accept the newyarn1literal.
169-172:upgradeadopts the same constrained package-manager flagUsing the same enum-backed
--package-manageroption forupgradekeeps behavior uniform and should simplify telemetry and option handling. AssumingUpgradeOptions’s type is updated (if needed) to include a compatiblepackageManagerfield, this looks fine.
15-15: CommanderOptionimport is verified as appropriateThe import
{ Option, program }fromcommanderis confirmed supported; Storybook's CLI dependency specifiescommander ^12.1.0, which provides bothOption#choicesandprogram.addOption. The file uses this pattern correctly in multiple places (lines 92, 116, 141, 169, 264, 287) with.addOption(new Option(...).choices(...)).No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/addons/themes/src/postinstall.ts (1)
13-16: Consider stricter typing for improved type safety.The
selectPackageManagerCommandfunction accepts anystringand requires a type assertion, while thepostinstallfunction'spackageManagerparameter lacks an explicit type. This allows invalid package manager values to pass through at compile time and fail silently at runtime.Consider applying this diff to strengthen type safety:
-const selectPackageManagerCommand = (packageManager: string) => - PACKAGE_MANAGER_TO_COMMAND[packageManager as keyof typeof PACKAGE_MANAGER_TO_COMMAND]; +const selectPackageManagerCommand = (packageManager: PackageManagerName) => + PACKAGE_MANAGER_TO_COMMAND[packageManager]; -export default async function postinstall({ packageManager = PackageManagerName.NPM }) { +export default async function postinstall({ + packageManager = PackageManagerName.NPM +}: { packageManager?: PackageManagerName }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/addons/themes/src/postinstall.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManagerFactory.ts(1 hunks)code/lib/cli-storybook/src/bin/run.ts(8 hunks)scripts/sandbox/generate.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/common/js-package-manager/JsPackageManager.ts
- scripts/sandbox/generate.ts
- code/lib/cli-storybook/src/bin/run.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
code/addons/themes/src/postinstall.ts
🧬 Code graph analysis (1)
code/core/src/common/js-package-manager/JsPackageManagerFactory.ts (5)
code/core/src/common/js-package-manager/NPMProxy.ts (1)
NPMProxy(70-299)code/core/src/common/js-package-manager/PNPMProxy.ts (1)
PNPMProxy(40-296)code/core/src/common/js-package-manager/Yarn1Proxy.ts (1)
Yarn1Proxy(37-242)code/core/src/common/js-package-manager/Yarn2Proxy.ts (1)
Yarn2Proxy(81-342)code/core/src/common/js-package-manager/BUNProxy.ts (1)
BUNProxy(69-323)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/common/js-package-manager/JsPackageManagerFactory.ts (1)
166-170: LGTM! Enum-based keys improve type safety and consistency.The refactor from string literals to enum-based computed property names ensures the PROXY_MAP keys align with the PackageManagerName enum values and the
typeproperties of the proxy classes. The Record type ensures all enum values are present, and the 'in' operator checks (lines 112, 147) work correctly with enum values.code/addons/themes/src/postinstall.ts (2)
1-2: LGTM!The import of PackageManagerName enables enum-based package manager handling throughout the file.
6-10: Enum keys improve consistency.The migration from string literals to enum-based keys aligns this mapping with the PackageManagerName enum values used across the codebase. Note that the key for Yarn 1 changes from 'yarn' to 'yarn1', which is a breaking change for any external callers passing 'yarn' as a string.
valentinpalkovic
force-pushed
the
valentin/fix-package-manager-yarn-value
branch
from
f58366c to
8de834e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/addons/themes/src/postinstall.ts (1)
13-14: Consider strengthening the parameter type.The function still accepts a generic
stringand relies on a type assertion. Consider updating the signature to acceptPackageManagerNamefor better compile-time type safety.Apply this diff:
-const selectPackageManagerCommand = (packageManager: string) => +const selectPackageManagerCommand = (packageManager: PackageManagerName) => PACKAGE_MANAGER_TO_COMMAND[packageManager as keyof typeof PACKAGE_MANAGER_TO_COMMAND];Or simplify the lookup:
-const selectPackageManagerCommand = (packageManager: string) => - PACKAGE_MANAGER_TO_COMMAND[packageManager as keyof typeof PACKAGE_MANAGER_TO_COMMAND]; +const selectPackageManagerCommand = (packageManager: PackageManagerName) => + PACKAGE_MANAGER_TO_COMMAND[packageManager];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/addons/themes/src/postinstall.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManagerFactory.ts(1 hunks)code/lib/cli-storybook/src/bin/run.ts(9 hunks)scripts/sandbox/generate.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/sandbox/generate.ts
- code/core/src/common/js-package-manager/JsPackageManager.ts
- code/core/src/common/js-package-manager/JsPackageManagerFactory.ts
- code/lib/cli-storybook/src/bin/run.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
code/addons/themes/src/postinstall.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/addons/themes/src/postinstall.ts (1)
1-1: LGTM! Clean refactoring to typed enum.The migration from string literals to
PackageManagerNameenum improves type safety and maintainability. The computed property names in the object literal correctly resolve to the enum values at runtime, and the default parameter is now more explicit.Also applies to: 6-10, 16-16
valentinpalkovic
force-pushed
the
valentin/fix-package-manager-yarn-value
branch
from
8de834e to
f58366c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/generate-sandboxes.yml(2 hunks)code/addons/themes/src/postinstall.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManagerFactory.ts(1 hunks)code/lib/cli-storybook/src/bin/run.ts(8 hunks)scripts/sandbox/generate.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/common/js-package-manager/JsPackageManager.ts
- code/core/src/common/js-package-manager/JsPackageManagerFactory.ts
- scripts/sandbox/generate.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
.github/workflows/generate-sandboxes.ymlcode/addons/themes/src/postinstall.tscode/lib/cli-storybook/src/bin/run.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: Generate to main
- GitHub Check: Generate to next
🔇 Additional comments (4)
code/addons/themes/src/postinstall.ts (2)
1-1: LGTM: Import of PackageManagerName enum.This import enables type-safe package manager handling throughout the file.
16-16: LGTM: Type-safe default parameter.Using
PackageManagerName.NPMinstead of the string literal'npm'provides better type safety and consistency.code/lib/cli-storybook/src/bin/run.ts (2)
5-5: LGTM: Imports for typed package manager options.Adding
PackageManagerNameand Commander'sOptionclass enables type-safe, validated CLI options.Also applies to: 15-15
92-96: LGTM: Consistent typed option implementation across commands.The migration from inline
.option()to.addOption()withOption.choices()provides excellent type safety and validation. The pattern is consistently applied across all 6 commands (init, add, remove, upgrade, automigrate, doctor), ensuring a uniform interface.This change automatically validates user input against
PackageManagerNameenum values and provides clear error messages for invalid choices.Also applies to: 116-120, 141-145, 169-173, 264-268, 287-291
| const PACKAGE_MANAGER_TO_COMMAND = { | ||
| npm: 'npx', | ||
| pnpm: 'pnpm dlx', | ||
| yarn1: 'npx', | ||
| yarn2: 'yarn dlx', | ||
| bun: 'bunx', | ||
| [PackageManagerName.NPM]: 'npx', | ||
| [PackageManagerName.PNPM]: 'pnpm dlx', | ||
| [PackageManagerName.YARN1]: 'npx', | ||
| [PackageManagerName.YARN2]: 'yarn dlx', | ||
| [PackageManagerName.BUN]: 'bunx', | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify type safety of computed property names.
The map now uses computed property names with enum values as keys. However, selectPackageManagerCommand (line 13) accepts a string parameter and performs a type assertion when accessing this object. This could lead to runtime errors if the string doesn't match enum values exactly.
Run the following script to verify the enum values match the expected strings:
🏁 Script executed:
#!/bin/bash
# Description: Check PackageManagerName enum values and verify usage
# Find the enum definition
ast-grep --pattern 'enum PackageManagerName {
$$$
}'
# Find all usages of selectPackageManagerCommand to verify callers pass correct values
rg -nP 'selectPackageManagerCommand\(' -A2Length of output: 815
I need to examine the selectPackageManagerCommand function definition and its type signature to verify the type safety concern.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find selectPackageManagerCommand function definition and signature
rg -nP 'function selectPackageManagerCommand|const selectPackageManagerCommand|export.*selectPackageManagerCommand' -A5 -B2Length of output: 705
🏁 Script executed:
#!/bin/bash
# Check the context around the call site to understand packageManager type
cat code/addons/themes/src/postinstall.ts | head -20Length of output: 792
🏁 Script executed:
#!/bin/bash
# Look for any type assertions with 'as' keyword around the PACKAGE_MANAGER_TO_COMMAND map usage
rg -nP 'PACKAGE_MANAGER_TO_COMMAND.*\[.*\]|as.*PackageManagerName|as string' code/addons/themes/src/postinstall.ts -B3 -A3Length of output: 405
I need to verify if there are other call sites and check the return type safety.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find all calls to selectPackageManagerCommand across entire codebase
rg -n 'selectPackageManagerCommand' --type=ts --type=tsx -B2 -A2Length of output: 27
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for selectPackageManagerCommand calls without type filter
rg -n 'selectPackageManagerCommand' -B2 -A2Length of output: 747
Change function parameter type from string to PackageManagerName to improve type safety.
The selectPackageManagerCommand function accepts packageManager: string with a type assertion, but should explicitly accept PackageManagerName instead. This eliminates the unsafe type assertion and clarifies the function's contract. At line 13-14, change:
const selectPackageManagerCommand = (packageManager: string) =>
PACKAGE_MANAGER_TO_COMMAND[packageManager as keyof typeof PACKAGE_MANAGER_TO_COMMAND];To accept the enum type directly, removing the need for type coercion and preventing potential runtime errors from invalid string values.
🤖 Prompt for AI Agents
In code/addons/themes/src/postinstall.ts around lines 5 to 11, the
selectPackageManagerCommand function currently accepts packageManager: string
and uses a type assertion into the PACKAGE_MANAGER_TO_COMMAND lookup; change the
parameter type to packageManager: PackageManagerName (and optionally add an
explicit return type of string | undefined) so you can index
PACKAGE_MANAGER_TO_COMMAND directly without the unsafe "as" cast; update the
function signature accordingly and remove the type coercion, ensuring callers
pass a PackageManagerName value (or add a guard/default if callers may pass
invalid strings).
2 participants
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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/coreteam 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>Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.