Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions code/addons/themes/src/postinstall.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
import { PackageManagerName } from 'storybook/internal/common';

import { spawnSync } from 'child_process';

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',
};
Comment on lines 5 to 11
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.

⚠️ Potential issue | 🟡 Minor

🧩 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\(' -A2

Length 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 -B2

Length 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 -20

Length 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 -A3

Length 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 -A2

Length of output: 27


[/run_scripts]


🏁 Script executed:

#!/bin/bash
# Search for selectPackageManagerCommand calls without type filter
rg -n 'selectPackageManagerCommand' -B2 -A2

Length 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).


const selectPackageManagerCommand = (packageManager: string) =>
PACKAGE_MANAGER_TO_COMMAND[packageManager as keyof typeof PACKAGE_MANAGER_TO_COMMAND];

export default async function postinstall({ packageManager = 'npm' }) {
export default async function postinstall({ packageManager = PackageManagerName.NPM }) {
const commandString = selectPackageManagerCommand(packageManager);
const [command, ...commandArgs] = commandString.split(' ');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import type { InstallationMetadata } from './types';

export enum PackageManagerName {
NPM = 'npm',
YARN1 = 'yarn',
YARN1 = 'yarn1',
YARN2 = 'yarn2',
PNPM = 'pnpm',
BUN = 'bun',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,11 @@ export class JsPackageManagerFactory {

/** Look up map of package manager proxies by name */
private static PROXY_MAP: Record<PackageManagerName, PackageManagerProxy> = {
npm: NPMProxy,
pnpm: PNPMProxy,
yarn: Yarn1Proxy,
yarn2: Yarn2Proxy,
bun: BUNProxy,
[PackageManagerName.NPM]: NPMProxy,
[PackageManagerName.PNPM]: PNPMProxy,
[PackageManagerName.YARN1]: Yarn1Proxy,
[PackageManagerName.YARN2]: Yarn2Proxy,
[PackageManagerName.BUN]: BUNProxy,
};

/**
Expand Down
42 changes: 29 additions & 13 deletions code/lib/cli-storybook/src/bin/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { globalSettings } from 'storybook/internal/cli';
import {
HandledError,
JsPackageManagerFactory,
PackageManagerName,
isCI,
optionalEnvToBoolean,
removeAddon as remove,
Expand All @@ -11,7 +12,7 @@ import { withTelemetry } from 'storybook/internal/core-server';
import { CLI_COLORS, logTracker, logger } from 'storybook/internal/node-logger';
import { addToGlobalContext, telemetry } from 'storybook/internal/telemetry';

import { program } from 'commander';
import { Option, program } from 'commander';
import envinfo from 'envinfo';
import leven from 'leven';
import picocolors from 'picocolors';
Expand Down Expand Up @@ -88,7 +89,11 @@ command('init')
.description('Initialize Storybook into your project')
.option('-f --force', 'Force add Storybook')
.option('-s --skip-install', 'Skip installing deps')
.option('--package-manager <npm|pnpm|yarn1|yarn2>', 'Force package manager for installing deps')
.addOption(
new Option('--package-manager <type>', 'Force package manager for installing deps').choices(
Object.values(PackageManagerName)
)
)
// TODO: Remove in SB11
.option('--use-pnp', 'Enable PnP mode for Yarn 2+')
.option('-p --parser <babel | babylon | flow | ts | tsx>', 'jscodeshift parser')
Expand All @@ -108,9 +113,10 @@ command('init')

command('add <addon>')
.description('Add an addon to your Storybook')
.option(
'--package-manager <npm|pnpm|yarn1|yarn2|bun>',
'Force package manager for installing dependencies'
.addOption(
new Option('--package-manager <type>', 'Force package manager for installing deps').choices(
Object.values(PackageManagerName)
)
)
.option('-c, --config-dir <dir-name>', 'Directory where to load Storybook configurations from')
.option('--skip-install', 'Skip installing deps')
Expand All @@ -132,9 +138,10 @@ command('add <addon>')

command('remove <addon>')
.description('Remove an addon from your Storybook')
.option(
'--package-manager <npm|pnpm|yarn1|yarn2|bun>',
'Force package manager for installing dependencies'
.addOption(
new Option('--package-manager <type>', 'Force package manager for installing deps').choices(
Object.values(PackageManagerName)
)
)
.option('-c, --config-dir <dir-name>', 'Directory where to load Storybook configurations from')
.option('-s --skip-install', 'Skip installing deps')
Expand All @@ -159,9 +166,10 @@ command('remove <addon>')

command('upgrade')
.description(`Upgrade your Storybook packages to v${versions.storybook}`)
.option(
'--package-manager <npm|pnpm|yarn1|yarn2|bun>',
'Force package manager for installing dependencies'
.addOption(
new Option('--package-manager <type>', 'Force package manager for installing deps').choices(
Object.values(PackageManagerName)
)
)
.option('-y --yes', 'Skip prompting the user')
.option('-f --force', 'force the upgrade, skipping autoblockers')
Expand Down Expand Up @@ -253,7 +261,11 @@ command('automigrate [fixId]')
.description('Check storybook for incompatibilities or migrations and apply fixes')
.option('-y --yes', 'Skip prompting the user')
.option('-n --dry-run', 'Only check for fixes, do not actually run them')
.option('--package-manager <npm|pnpm|yarn1|yarn2|bun>', 'Force package manager')
.addOption(
new Option('--package-manager <type>', 'Force package manager for installing deps').choices(
Object.values(PackageManagerName)
)
)
.option('-l --list', 'List available migrations')
.option('-c, --config-dir <dir-name>', 'Directory of Storybook configurations to migrate')
.option('-s --skip-install', 'Skip installing deps')
Expand All @@ -272,7 +284,11 @@ command('automigrate [fixId]')

command('doctor')
.description('Check Storybook for known problems and provide suggestions or fixes')
.option('--package-manager <npm|pnpm|yarn1|yarn2|bun>', 'Force package manager')
.addOption(
new Option('--package-manager <type>', 'Force package manager for installing deps').choices(
Object.values(PackageManagerName)
)
)
.option('-c, --config-dir <dir-name>', 'Directory of Storybook configuration')
.action(async (options) => {
withTelemetry('doctor', { cliOptions: options }, async () => {
Expand Down
3 changes: 2 additions & 1 deletion scripts/sandbox/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import pLimit from 'p-limit';
import prettyTime from 'pretty-hrtime';
import { dedent } from 'ts-dedent';

import { PackageManagerName } from '../../code/core/src/common/js-package-manager';
import { temporaryDirectory } from '../../code/core/src/common/utils/cli';
import storybookVersions from '../../code/core/src/common/versions';
import {
Expand Down Expand Up @@ -109,7 +110,7 @@ const addStorybook = async ({
await addResolutions(tmpDir);
}

await sbInit(tmpDir, env, [...flags, '--package-manager=yarn1'], debug);
await sbInit(tmpDir, env, [...flags, `--package-manager=${PackageManagerName.YARN1}`], debug);
} catch (e) {
console.log('error', e);
await rm(tmpDir, { recursive: true, force: true });
Expand Down