Cleanup: Remove duplicated entrypoints in core #32507
Conversation
WalkthroughPrunes exports of internal entrypoints under Sequence Diagram(s)No sequence diagram provided — these changes are import/export and global-mapping adjustments without modifications to runtime control flow or new interaction sequences. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 4fac215
☁️ Nx Cloud last updated this comment at |
![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/build-config.ts (1)
100-161: Ensure type declarations are emitted for these public subpathsIf
BuildEntriesdoesn’t default to DTS for browser entries, explicitly enable it to avoid missing.d.tsin consumers.Example (repeat where needed):
{ exportEntries: ['./actions'], entryPoint: './src/actions/index.ts', + dts: true, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/build-config.ts(2 hunks)code/core/package.json(0 hunks)
💤 Files with no reviewable changes (1)
- code/core/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/core/build-config.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). (3)
- GitHub Check: normal
- GitHub Check: Danger JS
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/core/build-config.ts (2)
100-102: Confirm ‘test’ belongs under browser exportsIf
./src/testincludes any Node‑only deps, consider moving it toentries.nodeinstead. Otherwise ensure it’s tree‑shaken so it doesn’t bloat runtime bundles.
100-123: No internal imports found in repo — add MIGRATION.md for external consumersRepo-wide git/ripgrep searches for '@storybook/core/internal/*' and the listed subpaths returned no matches; removal appears safe inside this repo, but this is a breaking public change — add a MIGRATION.md mapping old→new subpaths and update docs/examples before publishing.
…book/internal/theming' across multiple components
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
code/addons/pseudo-states/src/manager/PseudoStateTool.tsx (2)
41-47: Guard against undefinedpseudoin toggleOption (prevents runtime crash).
Destructuring from possibly undefined throws; default to an empty object.Apply:
- const { [option]: value, ...rest } = pseudo; + const { [option]: value, ...rest } = (pseudo ?? {}) as Record<string, boolean>;
3-3: Consider switching to public components entrypoint.
If available, preferstorybook/componentsto avoid internal imports.-import { Form, IconButton, TooltipLinkList, WithTooltip } from 'storybook/internal/components'; +import { Form, IconButton, TooltipLinkList, WithTooltip } from 'storybook/components';code/addons/pseudo-states/src/stories/CSSAtRules.stories.tsx (1)
3-3: Verify core-events import; migrate to public path if supported.
Align with cleanup by avoidinginternal.-import { FORCE_REMOUNT } from 'storybook/internal/core-events'; +import { FORCE_REMOUNT } from 'storybook/core-events';code/core/src/manager/components/layout/Layout.stories.tsx (1)
4-4: Verify router import; prefer public entrypoint if available.
Ifstorybook/routeris public here, switch to it.-import { LocationProvider } from 'storybook/internal/router'; +import { LocationProvider } from 'storybook/router';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
code/addons/docs/src/blocks/components/TableOfContents.stories.tsx(1 hunks)code/addons/pseudo-states/src/manager/PseudoStateTool.tsx(1 hunks)code/addons/pseudo-states/src/stories/CSSAtRules.stories.tsx(1 hunks)code/addons/vitest/src/components/SidebarContextMenu.tsx(1 hunks)code/builders/builder-vite/src/constants.ts(0 hunks)code/core/src/components/components/Button/Button.stories.tsx(1 hunks)code/core/src/components/components/Form/Checkbox.tsx(1 hunks)code/core/src/components/components/Form/Radio.tsx(1 hunks)code/core/src/highlight/useHighlights.stories.tsx(1 hunks)code/core/src/manager/components/layout/Layout.stories.tsx(1 hunks)code/core/src/preview/globals/globals.ts(0 hunks)code/core/src/preview/globals/runtime.ts(0 hunks)code/renderers/react/template/stories/decorators.stories.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- code/core/src/preview/globals/globals.ts
- code/builders/builder-vite/src/constants.ts
- code/core/src/preview/globals/runtime.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 (11)
code/addons/pseudo-states/src/manager/PseudoStateTool.tsx (1)
8-8: LGTM: moved to public theming API.code/addons/pseudo-states/src/stories/CSSAtRules.stories.tsx (1)
7-8: LGTM: preview API import updated to public entrypoint.code/core/src/components/components/Form/Radio.tsx (1)
3-3: LGTM: theming import now from public path.code/core/src/highlight/useHighlights.stories.tsx (1)
3-3: LGTM: useChannel moved to public preview-api.code/core/src/components/components/Button/Button.stories.tsx (1)
5-6: LGTM: styled now from public theming.code/addons/docs/src/blocks/components/TableOfContents.stories.tsx (1)
6-6: LGTM: styled import updated to public theming.code/core/src/components/components/Form/Checkbox.tsx (1)
3-3: LGTM: theming import updated to public path.code/core/src/manager/components/layout/Layout.stories.tsx (2)
9-9: LGTM: ManagerContext now from public manager-api.
1-1: Only intended internal entrypoints remain. Verified that imports for deprecated patterns—storybook/internal/theming,manager-api,preview-api,actions,highlight,test, andviewport—no longer appear, and the onlystorybook/internal/*entrypoints in use arecore-events,components, androuteras intentionally kept.code/renderers/react/template/stories/decorators.stories.tsx (1)
6-6: LGTM! Import updated to public API.The import has been correctly updated from the internal
storybook/internal/preview-apito the publicstorybook/preview-apientrypoint, aligning with the PR's objective to remove internal API usage.code/addons/vitest/src/components/SidebarContextMenu.tsx (1)
6-6: LGTM! Import updated to public API.The
APItype import has been correctly updated from the internalstorybook/internal/manager-apito the publicstorybook/manager-apientrypoint, consistent with the broader migration effort across the codebase.
| import React from 'react'; | ||
|
|
||
| import { type API } from 'storybook/internal/manager-api'; | ||
| import type { API_HashEntry } from 'storybook/internal/types'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Consider migrating API_HashEntry import to public API.
The API_HashEntry type is still being imported from storybook/internal/types. For consistency with the PR's goal to eliminate internal API usage, verify if this type is available through a public entrypoint.
🏁 Script executed:
#!/bin/bash
# Description: Check if API_HashEntry is available in public APIs
# Search for API_HashEntry export in public entrypoints
echo "Searching for API_HashEntry in public manager-api..."
rg -n "export.*API_HashEntry" --type=ts -g "!**/internal/**"
echo "Searching for API_HashEntry type definition..."
ast-grep --pattern 'export type API_HashEntry = $$$'
echo "Checking current usage of API_HashEntry..."
rg -n "API_HashEntry" --type=ts -C2Length of output: 12059
Replace internal API_HashEntry imports with the public manager-api type
API_HashEntry is defined in code/core/src/types/modules/api-stories.ts and is re‑exported by manager-api as HashEntry — replace imports from 'storybook/internal/types' with a public import (e.g. import type { HashEntry as API_HashEntry } from 'storybook/manager-api') in:
- code/addons/vitest/src/components/SidebarContextMenu.tsx
- code/addons/vitest/src/types.ts
- code/addons/vitest/src/components/TestProviderRender.tsx
🤖 Prompt for AI Agents
In code/addons/vitest/src/components/SidebarContextMenu.tsx (line 4) and the
other affected files code/addons/vitest/src/types.ts and
code/addons/vitest/src/components/TestProviderRender.tsx, replace the internal
import "import type { API_HashEntry } from 'storybook/internal/types';" with the
public manager-api re-export by importing HashEntry from 'storybook/manager-api'
and aliasing it to API_HashEntry (for example: import type { HashEntry as
API_HashEntry } from 'storybook/manager-api'), ensuring the type names remain
the same where used.
… export entries and define its types and default path
ndelangen
requested review from
ghengeveld,
kasperpeulen,
shilman,
valentinpalkovic,
vanessayuenn and
yannbf
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Awesome @ndelangen 🙌
It looks like you've mitigated the problem through a combination of automigration AND also swapping out the old imports dynamically using esbuild.
I am good to merge this provided that we thoroughly test it before release. There are currently no manual testing instructions in the PR. I would propose the following tests:
- Install a third-party addon that uses the old and API (e.g.
storybook-addon-test-codegen) and verify that it does not break - Create a local addon in
main.jsthat uses the old API and:
- verify that it doesn't break if not automigrated
- verify that it gets automigrated
- Create a local addon in a separate file that uses the old API and gets imported into
main.jsand:
- verify that it doesn't break if not automigrated
- verify that it gets automigrated
In addition, I would propose as part of the release process that we reach out to the top 5 community addons that use the old API and try to get them to update. This way we can consider removing the compatibility completely in a future major release if we want.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/fixes/consolidated-imports.ts (1)
90-132: Consider triggering this fix when only internal entrypoints are presentThe current check only activates when consolidated packages are found in package.json. Repos that only import
storybook/internal/*(and none of the consolidated packages) won’t get migrated.Two options:
- Split the internal-entrypoints migration into its own Fix that always scans sources.
- Or extend this check to also scan source files for
storybook/internal/imports and return a non-null result to triggerrun.I can draft a follow-up patch if you prefer the second approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
code/core/build-config.ts(2 hunks)code/core/package.json(0 hunks)code/core/src/manager/components/layout/Layout.stories.tsx(2 hunks)code/core/src/manager/globals/globals-module-info.ts(2 hunks)code/core/src/preview/globals/globals.ts(1 hunks)code/core/src/preview/globals/runtime.ts(0 hunks)code/lib/cli-storybook/src/automigrate/fixes/consolidated-imports.ts(1 hunks)
💤 Files with no reviewable changes (2)
- code/core/src/preview/globals/runtime.ts
- code/core/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/preview/globals/globals.ts
- code/core/build-config.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/globals/globals-module-info.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/globals/globals-module-info.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/core/src/manager/components/layout/Layout.stories.tsx
🧬 Code graph analysis (1)
code/core/src/manager/globals/globals-module-info.ts (1)
code/core/src/preview/globals/globals.ts (1)
globalsNameReferenceMap(3-18)
🔇 Additional comments (4)
code/core/src/manager/components/layout/Layout.stories.tsx (2)
66-74: Mock for getCurrentStoryData looks correctThe mock shape and nested renderLabel are fine for stories using ManagerContext.
Please confirm Layout only relies on { id, name, renderLabel } from getCurrentStoryData; if it reads more fields, expand the mock accordingly.
4-4: Import from @storybook/router (public) — not storybook/routerNo
storybook/routerentrypoint was found; MIGRATION.md maps@storybook/router→storybook/internal/router. The repo currently importsstorybook/internal/routerin multiple places (e.g. code/core/src/manager/index.tsx, code/core/src/manager/components/layout/Layout.stories.tsx). If you want to migrate, change imports to@storybook/routerand ensure the package/exports are available and update all usages accordingly.Likely an incorrect or invalid review comment.
code/lib/cli-storybook/src/automigrate/fixes/consolidated-imports.ts (1)
150-161: Fix mappings to public entrypoints; several are no-ops and contradict the PR’s intentEntries for actions/highlight/test/viewport currently map to internal paths (no-ops); map them to public entrypoints and add the
internal/internal/*variants.{ ...consolidatedPackages, 'storybook/internal/manager-api': 'storybook/manager-api', 'storybook/internal/preview-api': 'storybook/preview-api', 'storybook/internal/theming': 'storybook/theming', 'storybook/internal/theming/create': 'storybook/theming/create', - 'storybook/internal/test': 'storybook/test', - 'storybook/internal/actions': 'storybook/internal/actions', - 'storybook/internal/actions/decorator': 'storybook/internal/actions/decorator', - 'storybook/internal/highlight': 'storybook/internal/highlight', - 'storybook/internal/viewport': 'storybook/internal/viewport', + 'storybook/internal/test': 'storybook/test', + 'storybook/internal/internal/test': 'storybook/test', + 'storybook/internal/actions': 'storybook/actions', + 'storybook/internal/actions/decorator': 'storybook/actions/decorator', + 'storybook/internal/highlight': 'storybook/highlight', + 'storybook/internal/internal/highlight': 'storybook/highlight', + 'storybook/internal/viewport': 'storybook/viewport', + 'storybook/internal/internal/viewport': 'storybook/viewport', },code/core/src/manager/globals/globals-module-info.ts (1)
44-51: Add double-internal aliases for back-compat; consider avoiding the unsafe castA lot of legacy imports use storybook/internal/internal/* (see PR description). Currently only storybook/internal/* gets an alias; add the double-internal form too so manager externals still work. Also, casting the computed key to typeof key lies to the type system; consider widening the result type or merging in a separately typed alias map.
Apply this minimal addition for double-internal paths:
if (duplicatedKeys.includes(key)) { acc[key.replace('storybook', 'storybook/internal') as typeof key] = { type: 'esm', varName: globalsNameReferenceMap[key], namedExports: Exports[key], defaultExport: true, }; + // Back-compat for legacy "storybook/internal/internal/*" imports + acc[key.replace('storybook', 'storybook/internal/internal') as typeof key] = { + type: 'esm', + varName: globalsNameReferenceMap[key], + namedExports: Exports[key], + defaultExport: true, + }; }Optional, safer typing (outside the hunk): build base as the strict Record<keyof typeof globalsNameReferenceMap, ...>, then compute aliases in a separate Record<string, ModuleInfo> and spread them in; this avoids the cast on the index.
To confirm whether double-internal aliases are still needed in-repo or by addons:
What I did
I removed the core package exports that have been renamed.
storybook/internal/themingshould not be used, please usestorybook/theming.storybook/internal/theming/createshould not be used, please usestorybook/theming/create.storybook/internal/manager-apishould not be used, please usestorybook/manager-api.storybook/internal/preview-apishould not be used, please usestorybook/preview-api.storybook/internal/actionsshould not be used, please usestorybook/actions.storybook/internal/actions/decoratorshould not be used, please usestorybook/actions/decorator.storybook/internal/internal/highlightshould not be used, please usestorybook/highlight.storybook/internal/internal/testshould not be used, please usestorybook/test.storybook/internal/internal/viewportshould not be used, please usestorybook/viewport.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
Refactor
Chores
Impact