Core: Mark pnp support as deprecated #32645
Conversation
📝 WalkthroughWalkthroughAdds Yarn Plug'n'Play (PnP) detection and deprecation notices to init/dev flows, integrates PnP plugins into the builder/CRA webpack resolution, exposes a package-manager API to read installed package versions, extends types with an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / create-storybook
participant Detect as detectPnp()
participant Initiate as initiate / installStorybook
participant Logger as deprecate/logger
CLI->>Detect: check for .pnp.{js,cjs}
Detect-->>CLI: returns boolean pnp
CLI->>Logger: if pnp true -> emit deprecation notice
CLI-->>Initiate: continue init/install flow
sequenceDiagram
autonumber
participant Dev as buildDev
participant Detect as detectPnp()
participant Builder as builder-manager (esbuild)
participant Preset as CRA preset
participant PNPPlugin as pnp-webpack-plugin
Dev->>Detect: detect PnP
Detect-->>Dev: pnp flag
Dev->>Builder: set options.pnp and import esbuild-plugin-pnp
Dev->>Preset: enable PNP integrations
Preset->>PNPPlugin: add moduleLoader to resolveLoader
Preset->>Preset: add PNP to resolve.plugins
Note right of PNPPlugin: resolution hooks applied when PnP present
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Files/areas to pay extra attention to:
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
674-707: Fix cache key inconsistency.The cache key at line 677 only uses
packageName, but the comment at line 680 mentions "both working directory and package name for isolation." Additionally,clearInstalledVersionCacheat lines 486-496 expects keys to include a working directory prefix (checkingkey.endsWith(\::${packageName}`)`), which creates a mismatch.Apply this diff to align the cache key with the documented behavior and clearing logic:
// TODO: Remove pnp compatibility code in SB11 /** Returns the installed (within node_modules or pnp zip) version of a specified package */ public async getInstalledVersion(packageName: string): Promise<string | null> { - const cacheKey = packageName; + const cacheKey = `${this.cwd}::${packageName}`; try { - // Create cache key that includes both working directory and package name for isolation - // Check cache first const cachedVersion = JsPackageManager.installedVersionCache.get(cacheKey);
🧹 Nitpick comments (2)
code/lib/create-storybook/src/generators/REACT_SCRIPTS/index.ts (1)
49-51: Consider moving the TODO closer to the prop-types dependency for clarity.The TODO on line 49 appears to refer to the
prop-typesdependency (lines 50-51) that's explicitly tied to Yarn PnP mode, but its placement between thewebpackandprop-typeslines creates ambiguity about what "this" refers to.Apply this diff to improve clarity:
const extraPackages = []; extraPackages.push('webpack'); - // TODO: Evaluate if this is correct after removing pnp compatibility code in SB11 // Miscellaneous dependency to add to be sure Storybook + CRA is working fine with Yarn PnP mode + // TODO: Evaluate if prop-types is still needed after removing PnP compatibility code in SB11 extraPackages.push('prop-types');code/core/src/cli/detect.ts (1)
173-176: Consider adding search boundary for consistency.The
detectPnp()function doesn't specify alastoption to limit the search scope, unlike other detection functions in this file (e.g.,detectBuilderat lines 115-116). This could lead to inconsistent behavior where PnP detection searches beyond the project root.Apply this diff to align with the pattern used by other detection functions:
// TODO: Remove in SB11 export async function detectPnp() { - return !!find.any(['.pnp.js', '.pnp.cjs']); + return !!find.any(['.pnp.js', '.pnp.cjs'], { last: getProjectRoot() }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
code/core/src/builder-manager/index.ts(1 hunks)code/core/src/cli/detect.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)code/core/src/common/js-package-manager/PNPMProxy.ts(1 hunks)code/core/src/common/js-package-manager/Yarn2Proxy.ts(1 hunks)code/core/src/common/utils/strip-abs-node-modules-path.ts(1 hunks)code/core/src/core-server/build-dev.ts(2 hunks)code/core/src/core-server/typings.d.ts(1 hunks)code/core/src/telemetry/get-framework-info.ts(1 hunks)code/core/src/telemetry/get-package-manager-info.ts(1 hunks)code/core/src/types/modules/core-common.ts(1 hunks)code/frameworks/nextjs/src/swc/next-swc-loader-patch.ts(1 hunks)code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.ts(1 hunks)code/lib/cli-storybook/src/bin/run.ts(1 hunks)code/lib/cli-storybook/src/sandbox-templates.ts(0 hunks)code/lib/create-storybook/src/bin/run.ts(1 hunks)code/lib/create-storybook/src/generators/REACT_SCRIPTS/index.ts(1 hunks)code/lib/create-storybook/src/generators/baseGenerator.ts(2 hunks)code/lib/create-storybook/src/generators/types.ts(1 hunks)code/lib/create-storybook/src/initiate.ts(2 hunks)code/presets/create-react-app/src/index.ts(3 hunks)code/renderers/react/src/preset.ts(1 hunks)scripts/sandbox/utils/yarn.ts(1 hunks)scripts/utils/yarn.ts(1 hunks)test-storybooks/yarn-pnp/.yarnrc.yml(1 hunks)
💤 Files with no reviewable changes (1)
- code/lib/cli-storybook/src/sandbox-templates.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/lib/cli-storybook/src/bin/run.tscode/core/src/telemetry/get-framework-info.tscode/presets/create-react-app/src/index.tscode/lib/create-storybook/src/bin/run.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/preset.tsscripts/sandbox/utils/yarn.tscode/lib/create-storybook/src/initiate.tscode/core/src/cli/detect.tscode/core/src/telemetry/get-package-manager-info.tscode/core/src/common/utils/strip-abs-node-modules-path.tscode/lib/create-storybook/src/generators/types.tscode/core/src/core-server/build-dev.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.tscode/frameworks/nextjs/src/swc/next-swc-loader-patch.tscode/core/src/common/js-package-manager/Yarn2Proxy.tsscripts/utils/yarn.tscode/lib/create-storybook/src/generators/REACT_SCRIPTS/index.tscode/lib/create-storybook/src/generators/baseGenerator.tscode/core/src/builder-manager/index.tscode/core/src/core-server/typings.d.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/lib/cli-storybook/src/bin/run.tscode/core/src/telemetry/get-framework-info.tscode/presets/create-react-app/src/index.tscode/lib/create-storybook/src/bin/run.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/preset.tsscripts/sandbox/utils/yarn.tscode/lib/create-storybook/src/initiate.tscode/core/src/cli/detect.tscode/core/src/telemetry/get-package-manager-info.tscode/core/src/common/utils/strip-abs-node-modules-path.tscode/lib/create-storybook/src/generators/types.tscode/core/src/core-server/build-dev.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.tscode/frameworks/nextjs/src/swc/next-swc-loader-patch.tscode/core/src/common/js-package-manager/Yarn2Proxy.tsscripts/utils/yarn.tscode/lib/create-storybook/src/generators/REACT_SCRIPTS/index.tscode/lib/create-storybook/src/generators/baseGenerator.tscode/core/src/builder-manager/index.tscode/core/src/core-server/typings.d.ts
🧠 Learnings (3)
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
PR: storybookjs/storybook#32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/lib/create-storybook/src/generators/REACT_SCRIPTS/index.ts
📚 Learning: 2025-10-02T09:22:13.185Z
Learnt from: JReinhold
PR: storybookjs/storybook#32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.185Z
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:
test-storybooks/yarn-pnp/.yarnrc.yml
📚 Learning: 2025-09-25T09:21:27.284Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.284Z
Learning: Applies to test-storybooks/** : Maintain test configurations and assets under test-storybooks/ for Storybook testing
Applied to files:
test-storybooks/yarn-pnp/.yarnrc.yml
🧬 Code graph analysis (2)
code/lib/create-storybook/src/initiate.ts (1)
code/core/src/cli/detect.ts (1)
detectPnp(174-176)
code/core/src/core-server/build-dev.ts (1)
code/core/src/cli/detect.ts (1)
detectPnp(174-176)
⏰ 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). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (23)
scripts/sandbox/utils/yarn.ts (1)
9-10: Comment addition looks good.Acknowledging the TODO to revisit PnP handling post-SB11 cleanup; no actionable issues here.
code/core/src/telemetry/get-framework-info.ts (1)
66-68: LGTM! Clear deprecation note for future cleanup.The TODO comment clearly marks the framework name sanitization logic for reevaluation when PnP compatibility code is removed in SB11. This is appropriately placed and will help future maintainers understand the context.
code/frameworks/nextjs/src/swc/next-swc-loader-patch.ts (1)
156-158: LGTM! Helpful deprecation notes for PnP-related logic.The TODO comments clearly mark the PnP version check and related file reading logic for reevaluation in SB11. The placement before the
!process.versions.pnpcondition provides good context for future maintainers.code/lib/create-storybook/src/bin/run.ts (1)
34-35: LGTM! Proper deprecation marking for the PnP flag.The TODO comment clearly marks the
--use-pnpoption for removal in SB11 while keeping it functional for the deprecation period. This is the correct approach for a graceful deprecation path.code/renderers/react/src/preset.ts (1)
39-40: LGTM! Clear deprecation note for PnP workaround logic.The TODO comment appropriately marks the
resolvedReactfunction for reevaluation in SB11, as this function exists specifically to handle Yarn PnP resolution issues (explained in the comment below).test-storybooks/yarn-pnp/.yarnrc.yml (1)
1-2: LGTM! Clear removal plan for PnP test infrastructure.The TODO comment clearly marks the entire
test-storybooks/yarn-pnpdirectory for removal in SB11, which aligns with the broader deprecation of PnP support. This is appropriate for test infrastructure related to a deprecated feature.scripts/utils/yarn.ts (1)
52-53: LGTM! Clear deprecation note for PnP detection logic.The TODO comment appropriately marks the PnP API existence check (
.pnp.cjsfile) for removal in SB11. This aligns with the broader deprecation strategy and is placed correctly before the related logic.code/core/src/common/utils/strip-abs-node-modules-path.ts (1)
12-14: LGTM! Important deprecation notes for path normalization logic.The TODO comments appropriately flag the
node_modulespath logic for reevaluation in SB11. This is particularly important because PnP doesn't use traditionalnode_modulesstructures, so this path-stripping logic may need adjustments when PnP support is removed.code/core/src/types/modules/core-common.ts (1)
155-155: LGTM! Type addition enables PnP deprecation flow.The optional
pnp?: booleanfield addition toLoadOptionsenables passing PnP detection state through the options, which supports the deprecation warning flow implemented in other parts of the PR (e.g.,build-dev.ts). This is backward compatible since it's optional.code/core/src/telemetry/get-package-manager-info.ts (1)
14-14: LGTM! Documentation note for future consideration.The TODO appropriately flags that the
nodeLinkerdetection logic may need reevaluation after PnP compatibility code is removed in SB11.code/core/src/builder-manager/index.ts (2)
7-8: LGTM! PnP plugin import aligns with deprecation strategy.The import of
pnpPluginfrom@yarnpkg/esbuild-plugin-pnpcorrectly enables Yarn Plug'n'Play resolution in the manager builder, and the TODO comment appropriately flags this for removal in SB11.
107-107: LGTM! Plugin correctly integrated into esbuild configuration.The
pnpPlugin()is appropriately added to the plugins array alongsideglobalExternals, enabling PnP resolution for manager entries.code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.ts (2)
5-6: LGTM! Alternative wrapper name supports backward compatibility.The addition of
ALTERNATIVE_GET_ABSOLUTE_PATH_WRAPPER_NAMEwith thewrapForPnpvalue allows detection of existing PnP-specific wrapper functions, appropriately marked for removal in SB11.
59-67: LGTM! Detection logic correctly prioritizes alternative wrapper.The updated
getAbsolutePathWrapperNamecorrectly checks for the alternativewrapForPnpwrapper before the preferredgetAbsolutePathwrapper, providing backward compatibility during the PnP deprecation period.code/core/src/common/js-package-manager/PNPMProxy.ts (1)
145-145: LGTM! TODO appropriately marks PnP compatibility code.The comment correctly flags the
getModulePackageJSONmethod's PnP-specific logic (lines 147-174) for removal in SB11, consistent with the broader deprecation strategy.code/core/src/common/js-package-manager/Yarn2Proxy.ts (1)
152-152: LGTM! TODO appropriately marks PnP compatibility code.The comment correctly flags the
getModulePackageJSONmethod's PnP-specific logic (lines 154-199) for removal in SB11, mirroring the identical annotation inPNPMProxy.ts.code/lib/create-storybook/src/generators/types.ts (1)
10-11: LGTM! TODO appropriately marks the PnP field for removal.The comment correctly flags the
pnpboolean field inGeneratorOptionsfor removal in SB11, consistent with the broader PnP deprecation strategy.code/core/src/core-server/build-dev.ts (2)
27-27: LGTM! Import of PnP detection utility.The import of
detectPnpfrom the CLI detect module is appropriate for checking PnP usage during the dev build.
98-107: No update needed to deprecation version reference.
The deprecation message correctly cites the version when PnP was deprecated (Storybook 10.x) and aligns with the SB11 removal TODO; leave it as is.Likely an incorrect or invalid review comment.
code/core/src/core-server/typings.d.ts (1)
2-3: LGTM! Ambient module declaration enables TypeScript compilation.The ambient module declaration for
pnp-webpack-pluginis necessary for TypeScript to compile code that imports this third-party library, and the TODO comment appropriately flags it for removal in SB11 alongside other PnP-related code.code/lib/create-storybook/src/initiate.ts (1)
87-96: LGTM! Deprecation warning appropriately implemented.The PnP detection and deprecation notice are correctly positioned in the initialization flow and provide clear guidance to users about the upcoming removal.
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
709-712: LGTM!The
isPackageInstalledmethod correctly delegates togetInstalledVersionand provides a clear boolean interface.code/presets/create-react-app/src/index.ts (1)
5-6: LGTM! PnP plugin integration is appropriate for CRA preset.The PnP webpack plugin is correctly integrated in both loader resolution and main module resolution, which is necessary for CRA projects using Yarn PnP. The
as anycast at line 133 is acceptable for a deprecated feature marked for removal.Also applies to: 32-33, 132-133
|
View your CI Pipeline Execution ↗ for commit e055907
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
There is no rationale or issue link provided. Why is this change being made? |
@ndelangen is there a tracking issue somewhere to add an explanation to |
| As of Storybook 10.0, PnP is deprecated. | ||
| If you are using PnP, you can continue to use Storybook 10.0, but we recommend migrating to a different package manager or linker-mode. | ||
| In future versions, PnP compatibility will be removed. |
There was a problem hiding this comment.
Let's say version 11 here if that's what we're planning
There was a problem hiding this comment.
The effort required to continuously support Yarn PnP is significant, and has always been. However usage metrics show that the number of users actually using Yarn PnP is a very tiny fraction of the total. So we'd rather re-prioritize maintainers' time elsewhere. |
|
A lot of the marked deprecations here are either already effectively no-ops (esbuild supports yarn without plugins, for example), or solely related to the CLI. The builder otherwise seems largely unaffected - other than the need to pre-resolve plugin paths (which is required in monorepos, not just pnp). I can appreciate that the effort that goes into maintaining support for custom node loaders for the CLI is substantial, and a sufficiently minority of users using it - but at the same time, I hope that you'll still be open to patch submissions for the builder should any arise. |
ndelangen
added
freeze
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/cli/detect.ts (1)
175-176: Consider removingasynckeyword.The function is declared
asyncbut doesn't useawait, so it unnecessarily wraps the result in a Promise. If API consistency with other detection functions isn't required, removingasyncwould be slightly more efficient.-export async function detectPnp() { +export function detectPnp() { return !!find.any(['.pnp.js', '.pnp.cjs'], { last: getProjectRoot() }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/cli/detect.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/cli/detect.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/cli/detect.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/cli/detect.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/cli/detect.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
| // TODO: Remove in SB11 | ||
| export async function detectPnp() { | ||
| return !!find.any(['.pnp.js', '.pnp.cjs']); | ||
| } |
There was a problem hiding this comment.
Limit search scope to prevent false positives.
The find.any() call searches without a boundary and may traverse parent directories beyond the project root. If a parent directory contains .pnp.js or .pnp.cjs, this function will return true even when the current project doesn't use PnP, triggering incorrect deprecation warnings.
Apply this diff to limit the search to the project root, consistent with detectBuilder (lines 115–116):
// TODO: Remove in SB11
export async function detectPnp() {
- return !!find.any(['.pnp.js', '.pnp.cjs']);
+ return !!find.any(['.pnp.js', '.pnp.cjs'], { last: getProjectRoot() });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: Remove in SB11 | |
| export async function detectPnp() { | |
| return !!find.any(['.pnp.js', '.pnp.cjs']); | |
| } | |
| // TODO: Remove in SB11 | |
| export async function detectPnp() { | |
| return !!find.any(['.pnp.js', '.pnp.cjs'], { last: getProjectRoot() }); | |
| } |
🤖 Prompt for AI Agents
In code/core/src/cli/detect.ts around lines 174–177, the detectPnp function
currently calls find.any() without a boundary which can traverse parent
directories and yield false positives; change the call to constrain the search
to the project root (use the same pattern/options used by detectBuilder at lines
115–116) so that find.any only checks the project root for '.pnp.js' or
'.pnp.cjs' and does not traverse upward.
|
I've opened a GitHub discussion post here, where we can discuss this further: |
What I did
I've added a deprecation warning about us dropping pnp support starting sb11 in both the
create-storybookstep as well as thedevcommand.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 canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Deprecations
Chores