Repository navigation
Core: Remove deprecated CSF and manager type aliases [SB-2017] - #36706
Conversation
Remove the six deprecated surfaces from the storybook package and migrate every in-repo consumer to the supported replacements: - StoryKind -> ComponentTitle - StoryIdentifier.kind / .story -> title / name (members deleted, along with the back-compat writes in prepareStory, StoryRender, and the sanitizeStoryContextUpdate protected keys) - AnyFramework -> Renderer - StoryContextForLoaders -> StoryContext (LoaderFunction signature) - PlayFunctionContext -> StoryContext (PlayFunction signature) - StoriesHash -> IndexHash (manager-api re-export and 7 manager sites) Also strips the legacy manager wire-format accommodations those fields fed: the SELECT_STORY kind/story fallbacks, the setStories transform's kindParameters lookup and kind/story index migrations, and the composed refs defaultStoryMapper kind normalization. Addon-links migrates type-only; its data-sb-kind / data-sb-story runtime contract is intact. MIGRATION.md documents the alias -> replacement pairs.
…napshots SELECT_STORY no longer accepts the pre-CSF3 `kind`/`story` payload spellings: the handler destructures and discards them so they neither resolve a story nor leak into the forwarded options. StoryStore and PreviewWeb inline snapshots drop the removed back-compat keys.
The three fixtures still wrote the removed StoryIdentifier back-compat fields, breaking the repo typecheck: the angular makeContext literals carried kind: 'kind' and the react RenderToCanvas play fixture passed context.kind/context.story into RenderContext. Use the surviving title/name-based fields only.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Acknowledged — the Codex review did not run because repository code-review credits are exhausted, so there is no review content to act on. Raising repository credit limits is a repo-admin action outside this PR; flagging it to the maintainers here rather than acting on it myself. |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot; a human maintainer approved this message before it was posted.
Review phase: Architecture review (stage 1 of 3) and Thermo-nuclear code quality review (stage 2 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. A human maintainer approves each public message and makes the final decision. Please tell us whether the feedback and evidence are useful.
Result for 5f2c015: architecture and code quality both request changes. Runtime verification (stage 3) has not run. All findings below come from reading the code at this head; we did not run the tests.
The mechanical part of this PR is correct and complete: PlayFunctionContext → StoryContext in the template and addon stories, StoriesHash → IndexHash in the sidebar, kind → title in the html, web-components and ember renderers, and kind/story removed from prepareStory, StoryRender and sanitizeStoryContextUpdate. Pointing LoaderFunction/PlayFunction straight at StoryContext is a real simplification. The open points are about the SELECT_STORY channel contract and some code the PR edited but should have deleted.
1. linkTo(title, name) in @storybook/addon-links now goes to the wrong story (blocking). The emitter wasn't migrated. code/addons/links/src/utils.ts:80-86 still sends { kind, story }, and so does the data-sb-kind/data-sb-story listener (utils.ts:95-98). The new handler in manager-api/modules/stories.ts:1260-1290 discards those fields and calls selectStory(undefined, undefined, …), which lands on the first story of the current component with no warning. The emitter tests (utils.test.ts:30-56) and the new receiver test both pass, so nothing catches it. Please:
- Add one exported
SelectStoryPayloadtype incode/core/src/types/modules/channelApi.ts: a union of{ storyId; viewMode? }and{ title; name?; viewMode? }. - Type addon-links
navigate()with it, send{ title, name }fromlinkToand the listener, and removeParamsId/ParamsComboand thekind/storyprops onLinkTo(react/components/link.tsx:24-29). Updateutils.test.ts. - In the handler, pick the option fields explicitly (
viewMode,scrollTo,ref) instead of forwarding...rest, and delete thekind?: unknown/story?: unknownmembers and their comment.
With a shared type, this drift becomes a compile error.
2. Composed refs built with Storybook 7.x–10.x (question for maintainers, then one change). Those refs run their own addon-links, which sends { kind, story }, and they can't be migrated from this repo. Their cross-story links will hit the same wrong-target path. Index loading is not affected. Either:
- keep compatibility in a single
parseSelectStoryPayload(raw: unknown)at the channel boundary (title ?? kind,name ?? story), or - drop it on purpose, say so in MIGRATION.md ("links from composed Storybooks built with ≤10.x no longer navigate"), and warn in the handler instead of navigating silently.
Also, MIGRATION.md calls kind/story "the pre-Storybook 3 wire format", and the handler comment says "pre-CSF3". Both are inaccurate: this was the SELECT_STORY payload through 10.x.
3. Don't edit dead code; delete it (blocking). The PR changes denormalizeStoryParameters (manager-api/lib/stories.ts:35-44) so it silently drops kindParameters, and rewrites the "v2 SET_STORIES event" test (stories.test.ts:1405-1440) to expect that. The only consumer, setRef({ setStoriesData }), now ignores the data (refs.ts:315-316). The preferred fix is to remove the legacy setStories composition receiver: the SET_STORIES handler (stories.ts:1248-1258), denormalizeStoryParameters, API_SetRefData.setStoriesData, SetStoriesPayload/SetStoriesStory*, API_StoryMapper/the StoryMapper config key, the fetchIndex "awaiting SET_STORIES" branch and the two SET_STORIES tests. Narrow setRef to if (storyIndex) so both @ts-expect-errors go away. Keep the SET_STORIES event constant: manager-api/root.tsx:441 uses it, and it's a public core-events export. If you'd rather keep the scope narrow, at least revert the edits to denormalizeStoryParameters and its test.
4. This is a breaking change. It removes exported types from storybook/internal/csf, storybook/internal/types and storybook/manager-api, plus runtime fields from the story context. A maintainer decided it should be labelled BREAKING CHANGE instead of maintenance, so it appears in the changelog's breaking-changes section. Please bring MIGRATION.md in line:
- "Six deprecated type aliases", but the table has seven rows; also fix the table's column alignment.
- Add the section to the 11.0.0 table of contents.
- List the other removals:
Addon_StoryApi.kind,Addon_AddStoryArgs.kind,SetStoriesStory.kind,stories.jsonv2 support for refs (StoryIndexV2is still exported fromtypes/modules/indexer.ts:169, so remove it or keep the v2 path), theStoryMapperoption, andRenderContext.kind/.storyfor third-party renderers. - Add the addon-links/composition statement from point 2, and keep
docs/addons/addon-migration-guide.mdxconsistent.
5. Smaller fixes.
- Tests:
stories.test.ts:1517-1537pinsselectStory(undefined, undefined, { viewMode: 'story' }), a navigation with no target. Assert the outcome you pick in point 2 instead. The three new receiver tests repeat the same setup, so use asetup()helper orit.each. - Formatting: please revert the line re-wrapping in
csf/story.ts,StoryRender.ts:87-89,sidebar/types.ts,decorators.test.ts:128-140andstories.test.ts:1432-1435. Those lines already fitprintWidth: 100, so the re-wraps only add review noise.
Push when ready and we'll run both review stages again on the new head, then runtime verification.
…ype-aliases # Conflicts: # code/core/src/manager-api/modules/refs.ts
… legacy setStories receiver
- Emit { title, name } payloads from linkTo and the data-sb-kind listener; the DOM attribute contract is unchanged
- Type the receiver with SelectStoryPayload; warn and ignore payloads without storyId, title, or name
- Remove the dead SET_STORIES composition receiver, denormalizeStoryParameters, API_StoryMapper, and StoryIndexV2
- Narrow setRef to storyIndex and fail unsupported index versions into the index error path
- Add the missing MIGRATION.md table-of-contents anchor
…inkTo props - Type navigate() with SelectStoryPayload and remove ParamsId/ParamsCombo. - Remove LinkTo's deprecated kind/story props; the data-sb-* attributes are unchanged. - Document the additional removals in MIGRATION.md and fix the table/wording. - Revert gratuitous line re-wraps flagged in review.
|
Thank you for the thorough review — all five points are addressed in 404c89c and the commits before it:
|
|
Noted, thanks — informational for this PR: the changes are type-only removals and manager simplifications, so no performance-sensitive path is intentionally altered. |
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot. This message was posted automatically. A human maintainer makes the final decision on this PR.
Review phase: Architecture review (stage 1 of 3) and Thermo-nuclear code quality review (stage 2 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. Messages are posted automatically, and a human maintainer makes the final decision. Please tell us whether the feedback and evidence are useful.
Result for 404c89c: architecture and code quality both request changes. Runtime verification (stage 3) has not run. We read the code at this head; we did not run the tests or type checks.
Thanks for the quick turnaround. All five points from our last review are addressed: the shared SelectStoryPayload, explicit option picking in the handler, the setStories removal, the BREAKING CHANGE label and the MIGRATION.md table.
Since then, maintainers decided two compatibility points for Storybook 11, and this head goes against both:
extract()output keeps the deprecatedkind/storyfields, so the Chromatic CLI keeps working. This is also why the*---chromaticjobs fail on this head.- The manager keeps navigating on legacy
SELECT_STORY{ kind, story }payloads from composed Storybooks built with 10.x or earlier. Documenting the break is not enough.
This replaces the "document the break" option we offered for composed refs last round. Sorry for the back-and-forth. The type cleanup itself stays: only the extract() output and the manager's channel boundary keep the legacy fields.
Details are in the inline comments: 4 are blocking (they cover 3 changes) and 3 are optional suggestions. When you push, we'll check these points on the new head, then run runtime verification, including extraction with the repo's pinned Chromatic CLI and a legacy link from a composed ref.
…vigation StoryStore.extract() keeps the deprecated kind/story keys on its output for external extractors such as the Chromatic CLI, via a named ExtractedStory return type; prepared story context types stay free of them. The SELECT_STORY receiver normalizes legacy kind/story payloads from composed Storybooks built with 10.x or earlier so cross-story links in those refs keep navigating, and warns only on payloads without any story identifier. The story-index v2 rejection now lives in transformStoryIndexToStoriesHash so local and composed indexes share one check. MIGRATION.md and the addon-links README document the supported behavior.
…ype-aliases # Conflicts: # code/core/src/manager/components/sidebar/SearchResults.stories.tsx # code/core/src/manager/components/sidebar/StatusContext.tsx # code/core/src/manager/components/sidebar/Tree.tsx # code/core/src/manager/components/sidebar/types.ts # code/core/src/manager/components/sidebar/useExpanded.ts
|
All three blocking items are implemented and pushed (e673239, plus the origin/next merge and a test-id fix): |
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot. This message was posted automatically; a human maintainer makes the final decision.
Review phase: Runtime verification (stage 3 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. Messages are posted automatically, and a human maintainer makes the final decision. Please tell us whether the feedback and evidence are useful.
Thank you for the quick follow-up! All three review stages passed on 5c5c2a31bdb90cbc95eb3eefa3cbeefce4e07a65: architecture, code quality, and runtime verification. No stage was skipped.
What we checked at runtime
extract()output: in a browser build of the internal Storybook,preview.extract()returnskind/storyfor all 1721 entries (storyis the parent name for test entries). The rows are identical to thenextbase build.- Legacy links from composed Storybooks: a real Storybook 10.6.2 composed as a ref sends
SELECT_STORYwith{ kind, story }, and the manager now navigates to the right story. With the normalization step removed, the same check fails, so the check really covers this path. - Modern links:
storyIdandtitle/namepayloads, pluslinkToand<LinkTo>from addon-links, all navigate as before. - Smoke: story and docs pages load with no new console errors compared to the base build.
Limits
- We did not run Chromatic's cloud extraction ourselves, because that needs a project token. The browser
extract()check above is the closest local equivalent, and the*---chromaticCI jobs pass on this head. ci/circleci: benchmark-packagesfails because npm crashed (Cannot read properties of null (reading 'edgesOut')) while installing@storybook/angularinto the benchmark project. This looks unrelated to the diff, but we did not re-run it on the base branch.
There are two small optional suggestions in the inline comments. They don't block this PR. We're adding review:verified; a maintainer makes the final decision on merging.
…in README examples The ignore branch accepts a payload with only a name, so the warning now names all three accepted identifiers and the test checks the new wording. The last two LinkTo README examples still passed the removed story prop; they now use name= like the rest.
|
Thank you — the runtime verification evidence (extract() parity across all 1721 entries and the composed-ref navigation control) is exactly what we wanted to see for this compatibility round. Both Optional follow-ups from this review are now addressed in 22d3664: the last two README |
Why
Six deprecated TypeScript surfaces still ship in the
storybookpackage:StoryKind, thekind/storymembers ofStoryIdentifier,AnyFramework,StoryContextForLoaders,PlayFunctionContext, and theStoriesHashre-export onstorybook/manager-api. Two of them (StoryIdentifier.kind/.story) still have runtime back-compat producers writing stale data, three renderers still destructure the deprecatedkindfield, and the shipped addon migration guide already claimsStoriesHashis removed whilenextstill exports it — the docs are ahead of the code and both are wrong together. For Storybook 11 the type surface should be deprecated-free and the documentation honest.What
Removes the six aliases and migrates every in-repo consumer to the supported replacement:
StoryKindComponentTitleStoryIdentifier.kind/.storytitle/nameAnyFrameworkRendererStoryContextForLoadersStoryContextPlayFunctionContextStoryContextStoriesHashIndexHashThree coordinated layers:
csf/story.ts, the re-export mirrors (storybook/internal/types,types/modules/addons.ts,types/modules/channelApi.ts), andmanager-api/root.tsx(IndexHashstays).prepareStoryandStoryRenderstop writingkind/story, andsanitizeStoryContextUpdateno longer protects those keys. The html, web-components, and ember renderers readtitle(matching the seven renderers that already did).SELECT_STORYresolves only fromtitle/name(plusstoryId), thesetStoriestransform drops itskindParameterslookup andkind/storyindex migrations, and composed refs stop normalizing legacykind. The legacy payload types (SetStoriesStory,Addon_AddStoryArgs,Addon_StoryApi) lose theirkindmembers.LoaderFunctionandPlayFunctionnow takeStoryContextdirectly, likeTestFunction,BeforeEach, andAfterEachalready did.Breaking, intentionally: third-party addons that read
context.kindat runtime or import any of the six identifiers will fail typecheck/source on SB11. That is the point of the change; the newMIGRATION.mdsection documents the path for each alias.Out of scope by design: addon-links'
data-sb-kind/data-sb-storyHTML attributes are the addon's own documented runtime contract and are unchanged. Per review,LinkTo's deprecatedkind/storyprops are removed in favor oftitle/name, and the addon'snavigate()now sends the sameSelectStoryPayloadthe manager accepts.Note for grep-based checks:
transformStoryIndexToStoriesHash(and itsToStoriesHashOptions) is the supported composed-refs helper — its name merely contains the substringStoriesHash. It is not a deprecated alias and was left unchanged.How to Review
code/core/src/csf/story.ts— the declarations; everything else follows from the typecheck.code/core/src/manager-api/—modules/stories.ts(SELECT_STORY),lib/stories.ts(transform),modules/refs.ts(refs mapper),root.tsx(re-export).code/core/src/preview-api/— the back-compat producers.kind→titledestructure.MIGRATION.md— the new section documenting all six pairs.Note: this PR is deletion-heavy by nature (307 of 511 changed lines are removals) and is intentionally one PR per the SB-2017 spec's locked scope decision.
Visual evidence: not applicable - type-level removal only; no runtime rendering change is expected or observed.
Test changes: legacy V2 setStories coverage removed together with the transform branch it exercised; SELECT_STORY legacy kind/story fallback tests replaced by title/name/storyId resolution tests; remaining expectation updates drop the removed back-compat keys.
Manual testing
yarn nx run core:check— typecheck green on core and its dependency chain.yarn test code/core/src/manager-api— SELECT_STORY resolves fromtitle/name/storyIdonly, no legacy fallback; transform tests pass with the legacy branches gone.yarn test code/core/src/preview-api/modules/store— decorator sanitize and prepareStory tests pass without the legacy keys.yarn test code/renderers/html code/renderers/web-componentsand the ember preview tests — pass withRenderContextlacking the deprecated fields.Known CI failures
Two Chromatic checks fail on the current head (
22d3664babf):UI Tests: storybook-uiandUI Review: storybook-uireport an interaction failure —expected 2982 to be +0in the manager/Sidebar/Tree story "Row Heights And Section Gaps" (Tree.stories.tsx:174). The failing story file is not part of this PR's diff and is identical atnext's tip, so the failure is classified as flaky or pre-existing, not caused by this PR; a Chromatic retest is being arranged. All CircleCI jobs on this head succeeded (86 of 86 builds, including thebenchmark-packagesrerun — the earlier npm Arborist crash was a one-off infrastructure flake; the extractor failures seen on earlier commits were resolved by keeping the deprecatedkind/storyfields inStoryStore.extract()output). TheDangerstatus is now green: the required Core approval has landed (ndelangen).🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact