Repository navigation
Storybook: Make declared IDs the source of truth for URLs - #82123
Mustafabharmal wants to merge 24 commits into
Conversation
There was a problem hiding this comment.
Two small things, neither blocking:
icons-iconis deliberately declared in two files (packages/icons/src/icon/stories/index.story.tsxandstorybook/stories/icons/library.story.tsx, which share atitleand already merge into one entry). A one-line comment in each file pointing at the other would make that coupling visible, since changing only one of them would silently split the group.- The "Report the
MetaPropstype gap upstream" checkbox: fine to split it out, but let's open the follow-up issue and link it here so it doesn't get lost, since this PR closes #82110.
Overall the PR looks good to me. Nice work @Mustafabharmal ! However, given that this PR modifies several files related to the Storybook documentation, I'd like to get approval from the @WordPress/gutenberg-design team before moving forward with it.
🤖 PR meta 🤖🎉 PropsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Updated as activity occurs, without notifying anyone named here. Add the |
ciampo
left a comment
There was a problem hiding this comment.
The "Report the MetaProps type gap upstream" checkbox: fine to split it out, but let's open the follow-up issue and link it here so it doesn't get lost, since this PR closes #82110.
This still needs to be done.
Also, it would be good to get an approval from @WordPress/gutenberg-design
Replaces the hand-written glob parser and directory walk.
|
I've been a bit removed from this, so it's hard for me to give too much design feedback. My understanding is that this makes it so any FUTURE structural changes we make (adding or more likely removing a few folders of nesting) don't cause broken permalinks, is that right? In that vein, sounds good to me, but I'll defer to others who have strong opinions or additional nuance. |
mirka
left a comment
There was a problem hiding this comment.
My honest impression is that we're doing too much than is warranted at this time.
"Unintentional deletion" has not really been a thing. The enforcement of an explicit id already catches unintentional permalink changes. I don't think deletion protection is necessary on top of that.
And even if we did want snapshots, it isn't clear to me why we'd prefer a specifically generated and committed snapshot instead of reading storybook/build/index.json, which is already generated by Storybook.
But in any case, I would recommend starting with just the per-file check for the id.
| it( 'covers every story and doc', () => { | ||
| // Guards against the walk silently matching nothing, which would let | ||
| // the check below pass without reading a single file. | ||
| expect( files.length ).toBeGreaterThan( 250 ); | ||
| } ); |
There was a problem hiding this comment.
I'm not sure the magic number is necessary. Would a simple check for no matches suffice?
| // Shares its `id` and `title` with `packages/icons/src/icon/stories/index.story.tsx` | ||
| // so both files merge into one entry. Change them together. | ||
| id: 'icons-icon', | ||
| title: 'Design System/Icons/Icon', |
There was a problem hiding this comment.
This doesn't seem like a good pattern. Normally if we wanted to split the library story into its own file, we would just put the code (e.g. just the render function) in a separate file and import it in the main story file. Having two duplicated meta objects like this is not recommended.
There was a problem hiding this comment.
At first glance this feels like something that should be done in eslint, but I guess that wouldn't handle mdx files. Just noting that decision here explicitly for posterity, in case the mdx concerns disappears in the future.
What?
Closes #82110
Declares an explicit
idon every Storybook story and doc that previously relied on an automatically generated ID.This keeps existing Storybook URLs stable when stories are moved between sidebar categories, and adds checks to prevent IDs from being accidentally omitted or removed in the future.
Why?
A Storybook URL is based on the entry's ID.
When an entry does not declare an explicit
id, Storybook derives the ID from itstitle. Since the title also controls the sidebar hierarchy, moving a story to a different category can change its URL.For example:
can become:
and consequently change the generated ID from:
to:
This can break existing links, bookmarks, documentation references, and other references to the Storybook page.
The existing
idmechanism already provides a way to decouple the URL from the sidebar title. This change makes that behaviour explicit and consistent across the Storybook stories and docs.How?
idto the stories and docs that were relying on generated IDs.storiesglobs intostorybook/story-globs.tsso the test uses the same set of files as the Storybook configuration.storybook/README.md.The IDs were obtained from Storybook's own indexer rather than being independently reconstructed, ensuring that the change does not alter the existing index.
For MDX docs attached to a CSF file with
of={...}, the existing inherited ID behaviour is preserved.Testing
Story index
The Storybook index was compared before and after the change:
This confirms that declaring the IDs does not change the existing Storybook URLs.
ID enforcement
The test verifies that all indexed stories and docs declare an explicit ID.
Story ID snapshot
The snapshot is generated from the Storybook index without requiring a full Storybook build.
It also detects when a previously published story URL disappears.
Manual regression check
Changing a story title while keeping its explicit
idconfirms that the URL remains unchanged.Testing Instructions
id.Scope
idfor stories and docs that require stable URLsMetaPropstype gap upstream ref: [Bug]: Meta block's MetaProps type is missing the id prop supported by the indexer storybookjs/storybook#36668The
MetaPropsgap is unrelated to the Gutenberg runtime change. Storybook's MDXMetaPropstype currently does not exposeid, although the runtime/indexer supports it. This can be addressed separately upstream.Notes
The changes are intentionally limited to the Storybook URL stability problem. No changes are made to the generated IDs themselves, so existing Storybook URLs remain unchanged.
Use of AI Tools
Claude Code was used to implement and verify this change.