Skip to content

CLI: Warn when Yarn 1 is detected on a best-effort basis - #36282

Merged
obvious-autobuild[bot] merged 10 commits into
nextfrom
feat/warn-on-yarn1
Sep 14, 2026
Merged

obvious-autobuild[bot] merged 10 commits into
nextfrom
feat/warn-on-yarn1

Conversation

@obvious-autobuild

@obvious-autobuild obvious-autobuild Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Why

Yarn 1 (Classic) is in maintenance mode, and Storybook's package-manager ecosystem increasingly assumes modern managers (npm, pnpm, Yarn Berry). Users on Yarn 1 currently get no signal that support is best-effort only, so avoidable install/upgrade friction goes unexplained. This adds a single, actionable, docs-linked warning — without ever blocking any CLI operation.

What

A shared warnOnYarn1 helper in code/core/src/common/js-package-manager/ that branches on PackageManagerName.YARN1, deduplicates via once.warn, uses getPrettyPackageManagerName for the label ("Yarn Classic (v1)"), and emits one line:

Yarn Classic (v1) is supported on a best-effort basis. Storybook works best with npm, pnpm, or Yarn Berry. For more information, see: https://storybook.js.org/docs/get-started/install

Two call sites, both beside existing package-manager handling:

  • Init preflight (PreflightCheckCommand.ts) — placed strictly after the empty-directory branch returns, so the npm-fallback path never double-warns.
  • Shared project-data path (getStorybookData.ts) — covers upgrade, doctor, automigrate, add, and the agent-facing project-info skill in one place. The warning is unconditional — per review feedback, there is no suppression option; every consumer sees it.

Deliberately excluded: the upgrade autoblock system, dev/build startup flows, and telemetry — Yarn 1 must never become a hard blocker anywhere.

How to Review

  • code/core/src/common/js-package-manager/warnOnYarn1.ts — the whole behavior contract (never throws; single line; once-per-process).
  • code/core/src/cli/getStorybookData.ts — the unconditional warning beside package-manager detection.
  • code/lib/create-storybook/src/commands/PreflightCheckCommand.ts — placement after the empty-dir return.
  • Review history: per @valentinpalkovic's review, the earlier warnOnYarn1?: boolean suppression option (and the project-info.test.ts / warnOnYarn1.test.ts files covering it) was removed in 2d9411b — the warning now always logs.
  • Tests use stringContaining on plain fragments because picocolors can inject ANSI around colored text; upgrade integration tests capture process streams since the dist CLI runtime resolves its own node-logger instance that object spies can't see.

Manual testing

This is backend-only CLI logging work — no rendered UI, so visual/browser evidence is not applicable. To verify manually:

  1. In a scratch folder with a package.json using yarn@^1.22 ("packageManager": "yarn@1.22.22") and no lockfile changes, run the canary CLI's storybook upgrade — expect exactly one "Yarn Classic (v1) is supported on a best-effort basis…" line and a normal, non-blocking completion.
  2. Repeat in an npm/pnpm/Yarn Berry project — expect no such warning.
  3. Run storybook init in an empty directory with Yarn 1 available — expect the single npm-fallback notice and no duplicate best-effort warning.

Automated coverage: 53 tests across three suites (getStorybookData chokepoint, init preflight, upgrade integration).

Test Evidence

Backend-only CLI logging work — no rendered UI, so visual evidence is not applicable. Verified locally at head 74dc6e967483177d0582339edf88430d2cab9b06: all three suites pass (53/53, includes vitest typecheck), oxlint and oxfmt clean on touched files. Acceptance-criterion evidence (tc-1–tc-6) refreshed at this head in the Obvious project.

🔗 Obvious Project · 🧵 Obvious Thread

Shared best-effort warning for Yarn 1 (Classic) projects: emits once per
process via once.warn using the pretty package-manager label and links to
the install docs. Never throws.
Emit the shared warnOnYarn1 notice beside the existing package-manager
notice, strictly after the empty-directory branch returns so the npm
fallback path never double-warns. Init continues as before.
getStorybookData emits the shared warnOnYarn1 notice beside existing
detection, guarded by a warnOnYarn1 option (default on, suppressed only
when explicitly false). The agent-facing project-info skill passes
warnOnYarn1: false. Covers upgrade/doctor/automigrate/add flows; tests
assert single-warning behavior and silence for supported managers.
@obvious-autobuild
obvious-autobuild Bot marked this pull request as ready for review September 11, 2026 15:24
@obvious-autobuild obvious-autobuild Bot added feature request ci:normal Run our default set of CI jobs (choose this for most PRs). qa:needed Pull Requests that will need manual QA prior to release. labels Sep 11, 2026
@obvious-autobuild obvious-autobuild Bot changed the title feat(cli): warn on Yarn 1 (Classic) on a best-effort basis CLI: Warn when Yarn 1 is detected on a best-effort basis Sep 11, 2026
Comment thread code/core/src/cli/skills/project-info.ts Outdated
Comment thread code/core/src/cli/skills/project-info.test.ts Outdated
Comment thread code/core/src/common/js-package-manager/warnOnYarn1.test.ts Outdated
ObviousApp and others added 4 commits September 11, 2026 17:15
Review feedback: the agent-facing project-info path no longer suppresses
the warning, so warnOnYarn1 is unconditional in getStorybookData and the
warnOnYarn1 option is removed from the options interface. Drop the unit
test files that only covered the removed suppression behavior.
Comment thread code/core/src/cli/getStorybookData.test.ts Outdated
Comment thread code/lib/cli-storybook/src/upgrade.test.ts Outdated
…streams

Address review feedback:
- upgrade.test.ts: replace process stdout/stderr interception with a
  file-level node-logger mock that mirrors logger.warn into a spy while
  delegating to the real logger, keeping once() dedupe intact and letting
  assertions observe final emissions through the normal logger seam.
- getStorybookData.test.ts: use Vitest's typed vi.mock(import(...)) form
  for the node-logger mock.

Also drops the now-unused once import from upgrade.test.ts.
@storybook-app-bot

storybook-app-bot Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Package Benchmarks

Commit: 74dc6e9, ran on 14 September 2026 at 11:56:27 UTC

No significant changes detected, all good. 👏

Comment thread code/lib/cli-storybook/src/upgrade.test.ts Outdated
Converts the upgrade.test.ts node-logger and common mocks to the typed
vi.mock(import(...)) form per review feedback, dropping the explicit
importOriginal generic and the as typeof sbcc cast. Class-static overrides
use Object.assign so the factory satisfies Partial<T> without casts; the
versions remap keeps one focused cast since Record<string,string> cannot
satisfy the literal-keyed versions type. Fixes the typescript-validation
TS2769 introduced by the typed mock conversion in getStorybookData.test.ts.
@obvious-autobuild

Copy link
Copy Markdown
Contributor Author

Replying to this comment

Thanks for the report. The @storybook/vue3-vite size delta predates this PR's changes — this branch's seven files (core CLI package-manager warning + tests) do not touch vue3-vite; the delta arrived with the merge from next that carried the @react-aria/* dependency refresh (#36279). No action needed in this PR.

@obvious-autobuild
obvious-autobuild Bot merged commit 748e678 into next Sep 14, 2026
157 of 158 checks passed
@obvious-autobuild
obvious-autobuild Bot deleted the feat/warn-on-yarn1 branch September 14, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:normal Run our default set of CI jobs (choose this for most PRs). feature request qa:needed Pull Requests that will need manual QA prior to release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants