Core: Fix internal yarn build command#33445
Conversation
|
View your CI Pipeline Execution ↗ for commit ef3f9ca
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds documentation for yarn build flags and refactors the build script into an explicit CLI with added flags, in-memory package task mapping, package name validation, and conditional prompt logic for watch/prod options. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as build-package.ts (CLI)
participant Prompt as Inquirer/Prompts
participant Workspaces as Monorepo Workspaces
participant Yarn as yarn subprocess
Note over CLI,Workspaces: Startup: parse args, load workspaces
User->>CLI: invokes `yarn build [--all|<pkg>...] [--watch|--no-watch] [--prod|--no-prod]`
CLI->>Workspaces: read & sort package list (main first)
alt explicit packages or --all provided
CLI->>CLI: validate package names
else no explicit inputs
CLI->>Prompt: ask user to select packages
Prompt-->>CLI: selected packages
end
alt watch/prod flags missing
CLI->>Prompt: ask for watch/prod choices
Prompt-->>CLI: watch/prod selections
end
CLI->>Yarn: spawn per-package yarn build (with suffixes, watch/prod flags)
Yarn-->>CLI: stream package output (prefixed)
CLI->>User: stdout "Building selected packages..."
Yarn-->>User: package build logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build-package.ts (1)
152-184: Race condition withlastNamein concurrent builds.The
forEachwithasynccallback starts all builds concurrently, and the sharedlastNamevariable is accessed by multiple concurrent event handlers. This can cause output interleaving where the package prefix is incorrectly shown or omitted when multiple processes emit data simultaneously.If parallel execution is intentional, consider either:
- Always prefixing each line with the package name (removing the
lastNameoptimization), or- Using a per-process tracking approach.
Example: Always prefix output
sub.stdout?.on('data', (data) => { - if (lastName !== v.name) { - const prefix = `${picocolors.cyan(v.name)}:\n`; - process.stdout.write(prefix); - } - lastName = v.name; - process.stdout.write(data); + const prefix = `${picocolors.cyan(v.name)}: `; + const lines = data.toString().split('\n'); + lines.forEach((line, i) => { + if (line || i < lines.length - 1) { + process.stdout.write(`${prefix}${line}\n`); + } + }); });
🧹 Nitpick comments (1)
docs/contribute/code.mdx (1)
105-116: Consider documenting--no-watchand--no-prodflags.The documentation covers
--all,--watch, and--prod, but the implementation inbuild-package.tsalso includes--no-watchand--no-prodcounterparts. According to the PR objectives, these flags "skip related prompts when passed without a package name." Consider adding them for completeness:* `--all` will cause all packages to be built * `--watch` will enable watch mode (and skip the watch mode prompt) * `--prod` will build for production (and skip the production mode prompt) +* `--no-watch` will disable watch mode (and skip the watch mode prompt) +* `--no-prod` will disable production mode (and skip the production mode prompt) * individual package names can be passed, without the `@storybook/` prefix, e.g. `storybook`, `addon-docs`, etc.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/contribute/code.mdxscripts/build-package.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
scripts/build-package.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
scripts/build-package.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
scripts/build-package.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
scripts/build-package.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,json,html,ts,tsx,mjs} : Run ESLint checks using `yarn lint:js:cmd <file>` or the full command `cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives` to fix linting errors before committing
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,md,html,css,scss} : Format code using Prettier with `yarn prettier --write <file>`
📚 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:
docs/contribute/code.mdx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to {code/addons,code/frameworks}/**/README.md : Update README files for significant changes and include code examples in addon/framework documentation
Applied to files:
docs/contribute/code.mdx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,md,html,css,scss} : Format code using Prettier with `yarn prettier --write <file>`
Applied to files:
docs/contribute/code.mdx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,json,html,ts,tsx,mjs} : Run ESLint checks using `yarn lint:js:cmd <file>` or the full command `cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives` to fix linting errors before committing
Applied to files:
docs/contribute/code.mdx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Run compilation with NX using `yarn nx run-many -t compile -c production` for better caching and dependency management
Applied to files:
docs/contribute/code.mdx
📚 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:
docs/contribute/code.mdx
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to code/vitest.workspace.ts : Vitest configuration is centralized in `code/vitest.workspace.ts` for workspace setup
Applied to files:
scripts/build-package.ts
🧬 Code graph analysis (1)
scripts/build-package.ts (1)
scripts/task.ts (1)
tasks(81-105)
⏰ 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: nx
🔇 Additional comments (3)
scripts/build-package.ts (3)
63-82: LGTM!The Commander setup correctly handles the
--all,--watch/--no-watch, and--prod/--no-prodflags. UsingallowExcessArguments(true)combined withmain.argsis an appropriate approach for accepting package names as positional arguments.
109-149: LGTM!The prompt flow correctly skips watch/prod prompts when flags are explicitly provided, while still allowing interactive selection when no packages are specified. The conditional prompt inclusion pattern using
watchMode === undefined &&is a valid approach for filtering prompts.
30-33: The sort condition is correct. The main Storybook package incode/core/package.jsonis indeed named'storybook'(without the@storybook/prefix), so the conditiona.name === 'storybook'will correctly identify and place it first in the option list. All other packages follow the@storybook/...naming convention and will be sorted after the main package.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build-package.ts (1)
152-182: Async callbacks inforEachare not awaited, causing premature script exit in non-watch mode.The script spawns subprocesses but doesn't await their completion. The
forEachloop on line 152 creates unresolved promises with async callbacks, andrun()at line 185 is never awaited. In non-watch mode (the default), the script exits immediately after spawning builds, before any build processes can complete. Additionally, subprocess failures are never propagated to the script's exit code.Fix by awaiting the completion of all subprocesses:
- Use
Promise.all()to wait for all builds to finish before returning fromrun()- Add error handling to capture subprocess exit codes and failures
- Ensure the script exits with non-zero status if any build fails
♻️ Duplicate comments (1)
scripts/build-package.ts (1)
88-101: Handle invalid package names with no close match.The exit code fix was applied, but the second part of the past review issue remains: when
findMostMatchTextreturns falsy (no close match found),hasInvalidNameis never set totrue, so the invalid argument is silently ignored and the script continues with an incomplete selection.Proposed fix
for (const arg of main.args) { if (!suffixList.includes(arg)) { const matchText = findMostMatchText(suffixList, arg); + hasInvalidName = true; if (matchText) { - hasInvalidName = true; process.stderr.write( `${picocolors.red('Error')}: ${picocolors.cyan( arg )} is not a valid package name, Did you mean ${picocolors.cyan(matchText)}?\n` ); + } else { + process.stderr.write( + `${picocolors.red('Error')}: ${picocolors.cyan(arg)} is not a valid package name.\n` + ); } } }
🧹 Nitpick comments (1)
scripts/build-package.ts (1)
28-30: Consider using both comparator arguments for clarity.The sort callback only uses the first argument. While this achieves the goal of placing
storybookfirst, it's unconventional and could confuse future readers.Proposed fix
const packages = (await getCodeWorkspaces()) .filter(({ name }) => name !== '@storybook/code') - .sort((a) => (a.name === 'storybook' ? -1 : 0)); // Place main package first in option list + .sort((a, b) => { + if (a.name === 'storybook') return -1; + if (b.name === 'storybook') return 1; + return 0; + }); // Place main package first in option list
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/build-package.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
scripts/build-package.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
scripts/build-package.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
scripts/build-package.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
scripts/build-package.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,json,html,ts,tsx,mjs} : Run ESLint checks using `yarn lint:js:cmd <file>` or the full command `cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives` to fix linting errors before committing
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode across all packages
Applied to files:
scripts/build-package.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,json,html,ts,tsx,mjs} : Run ESLint checks using `yarn lint:js:cmd <file>` or the full command `cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives` to fix linting errors before committing
Applied to files:
scripts/build-package.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to code/vitest.workspace.ts : Vitest configuration is centralized in `code/vitest.workspace.ts` for workspace setup
Applied to files:
scripts/build-package.ts
🧬 Code graph analysis (1)
scripts/build-package.ts (2)
scripts/task.ts (1)
tasks(81-105)scripts/utils/tools.ts (1)
picocolors(118-118)
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
scripts/build-package.ts (2)
110-125: LGTM!The conditional prompt logic correctly skips watch/prod prompts when the respective flags are explicitly provided via CLI. The
promptslibrary handles falsy values in the questions array gracefully.
72-80: LGTM!The options parsing and per-package flag checking logic is clear. Using
main.argsto detect package suffixes and combining withopts.allcorrectly handles both explicit package selection and the--allflag.
2 participants
Closes #33432
What I did
yarn build --allwhich incorrectly caused the script to exit--all storybookwhich incorrectly assumedstorybookis an invalid package name due to incorrect package name filteringstorybookpackage appearing first, as it's the most useful--watchand--prodflags skip relevant prompts when passed without package name--no-watchand--no-prodcounterparts with the same effectyarn buildflagsChecklist for Contributors
Testing
ø
Manual testing
Run commands:
yarn build, you will get fully prompted (watch + prod + packages, `storybook appears first)yarn build --all, build will startyarn build --all storybook, build will startyarn build --all addon-a11y, build will startyarn build storybook addon-a11y, build will start with only 2 packagesyarn build --all --watch, build will start in watch modeyarn build --watch --no-prod, you will be prompted only package names, and build will start in watch mode and development modeDocumentation
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
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.