Storybook: Upgrade Storybook to 10.4#77382
Conversation
github-actions
Bot
added
[Package] Components
|
Size Change: -17 B (0%) Total Size: 8.6 MB 📦 View Changed
|
|
Flaky tests detected in 9f2be2e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27637900453
|
On my machine the old Storybook build takes 1min20s, while with this PR it goes up to 12min. Something horrible is going on. I suspect it's related to Turning off |
|
The build times go back to normal when I disable the |
|
After I disable
|
|
Nice find, @jsnajdr 👍 Yeah, I think it's fine to disable that as long as the regression doesn't return. And it sounds like it's separate problem from the component import resolution. |
|
The "Prop type error" and "No component file found" errors happen because the manifest generator fails to read the component info with The storybookjs/storybook#34386 issue is very closely related. The reporter has the same problem, and tried to work around by creating a custom This is a problem only in the manifest generator, which is quite independent from the main storybook build. There, when generating the stories for components and their props docs, At this moment I don't know how to solve this, it will probably require an upstream patch. |
|
Nice discovery @jsnajdr . I had also stumbled on storybookjs/storybook#34386 but wasn't entirely confident it was related. It was also the reason I added the commented I wonder if there's some option to temporarily patch/override |
The main problem is that the manifest generator code is almost completely independent from the rest of Storybook, and does things its own way. And it's a very new code (2 months old) that is not battle-tested. The main Storybook build uses you can see it does a tremendous amount of preprocessing and wrapping before actually creating the docgen instance with The manifest generator doesn't reuse this at all, it has its own code to load the TS project and create the docgen instance. It's much simpler, but doesn't work for our case. |
aduth
force-pushed
the
update/storybook-10-3
branch
from
72955b0 to
336edb4
Compare
Opt into Storybook 10.4's `experimentalReactComponentMeta` feature, which backs the components manifest with a persistent TypeScript LanguageService (via `@volar/typescript`). The previous manifest extractor used a bespoke tsconfig loader that ignored `references`, so our root `tsconfig.json` (which has `files: []` and delegates everything to references) produced an empty TS program. The result was a manifest peppered with "No component file found" errors, identified in #77382 and the upstream issue storybookjs/storybook#34386. Also drop `EXPERIMENTAL_useProjectService: true` from the `react-docgen-typescript` options. Per investigation in #77382, this flag was the cause of a ~10x build slowdown. Removing it does not reintroduce the prop-extraction regression that #74807 added it to address. The `experimentalReactComponentMeta` flag only affects the manifest pipeline. The in-stories docs UI continues to use `react-docgen-typescript` through the React Vite framework's Vite plugin, including our existing `propFilter`. The new engine has its own source filtering for the manifest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aduth
changed the title
|
I've refreshed this pull request and updated Storybook to the new latest version 10.4.1. One additional notable improvement in this new version is the availability of their new component meta extractor tool for manifests, which aims to replace
However, there were a lot of complications in this upgrade, some of which were already seen in the earlier attempt. A quick summary:
|
| ### `ref` | ||
|
|
||
| - Type: `Ref<any>` | ||
| - Required: No | ||
|
|
There was a problem hiding this comment.
Something to consider for our docgen tool now that we're on React 19 and ref looks like any other prop. Should it be documented? We didn't document it before even though we were forwarding the ref.
There was a problem hiding this comment.
I'm leaning more towards documenting it:
- it's now a prop like any other, less custom code to maintain
- not all components support
ref(especially in@wordpress/components), so if we omit it, we'd be hiding that information
The question is: given we may want to add support for React 18 for some time, are we doing this migration now, or should we wait?
There was a problem hiding this comment.
And also, should we apply the same "fix" to other components flagged with the same issue (Navigator, ColorPicker, Slot, ...)
There was a problem hiding this comment.
I'm leaning more towards documenting it:
Yeah, I tend to agree. I'll see about adding a brief description, assuming it's feasible to do so within the current types. It would probably be ideal to standardize the description, and maybe that should happen inside the API docs tool.
The question is: given we may want to add support for React 18 for some time, are we doing this migration now, or should we wait?
My understanding is that this is safe to do inside @wordpress/components or any window.wp script package, because the code that's included in these packages is linked to the version of React in WordPress (i.e. this code will be shipped with WordPress 7.1 / React 19, and older versions of wp.components shipped in earlier versions of WordPress will continue to use forwardRef).
Though it'd be good to have confirmation on this understanding. There's a few related discussions about how wpScript packages do or don't have to support React 18 (example).
And also, should we apply the same "fix" to other components flagged with the same issue (
Navigator,ColorPicker,Slot, ...)
The problem with these components is different than needing to refactor forwardRef to use ref-as-prop. In fact, for most of them, the problem is that we haven't written a description at all 😅
There was a problem hiding this comment.
And also, should we apply the same "fix" to other components flagged with the same issue (
Navigator,ColorPicker,Slot, ...)The problem with these components is different than needing to refactor
forwardRefto use ref-as-prop. In fact, for most of them, the problem is that we haven't written a description at all 😅
Though it could certainly resurface for those other components, and I think the best way to address this is a pull request that mass-updates forwardRef to ref-as-prop (scoped as needed to keep things sensible).
There was a problem hiding this comment.
I'm leaning more towards documenting it:
Yeah, I tend to agree. I'll see about adding a brief description, assuming it's feasible to do so within the current types. It would probably be ideal to standardize the description, and maybe that should happen inside the API docs tool.
Looking closer at this, the original implementation by Claude to just shove a type intersection & { ref?: Ref< any > } on the component props destructure wasn't ideal. This is a standard React prop, and we should use standard React utility types to document this, and certainly do better than any. In b889e06 I moved this to types.ts and used React.RefAttributes with the appropriate HTML element generic.
Separately, a couple thoughts:
- Especially as we continue to refactor components, we might want to replace
React.ComponentPropsWithoutRefwithReact.ComponentPropsWithRefso that this typing comes throughWordPressComponentPropsinstead of per-component. The only question I have there is whether we want the explicit per-component opt-in as an indication that the component does, in fact, forward the ref (or at least does something with it). - Note that the changes in b889e06 will once again remove
reffromREADME.md, despite us agreeing above that we should document it. I think this is a question for the API docs tool, which currently filters out all library types. Given this revised approach,refis a library type, so how should we reconcile that? Do we want to makerefa special case in the props filtering?
There was a problem hiding this comment.
In b889e06 I moved this to
types.tsand usedReact.RefAttributeswith the appropriate HTML element generic.
This turned out to be a bit more involved to address build errors which emerged after this change. In general, a lot of it stems from additional strictness we get going from plain forwardRef (without generic) to typed Ref values.
A few additional notes:
- I changed from
React.RefAttributestoRefafter seeing that documentation forRefAttributesdiscourages its use except when needing to handle compatibility with legacy refs.- This also brings back the
README.mddocumentation sincerefis defined in the component's own types. I added a brief description, although the same question as above still applies if we want to consider handling this centrally in the API docs tooll
- This also brings back the
- I did a light exploration of what it looks like to convert
WordPressComponentPropsto useReact.ComponentPropsWithRefand I think the biggest part of this work will be reconciling the handling of refs incontextConnect
aduth
requested review from
a team,
ajitbohra,
gigitux,
ntsekouras and
oandregal
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If 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. |
aduth
added
[Type] Developer Documentation
This one is also rather complex, and seems to stem from a few inter-related issues:
A couple thoughts here:
I updated |
Bump Storybook and related framework packages from 10.2.8 to 10.4.1. Bump `react-docgen-typescript` from 2.2.2 to 2.4.0 (root + tools/api-docs) to align with what Storybook 10.4's react-vite framework expects. The `experimentalComponentsManifest` feature flag stabilized to `componentsManifest` in Storybook 10.3, so rename it accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Opt into Storybook 10.4's `experimentalReactComponentMeta` feature, which backs the components manifest with a persistent TypeScript LanguageService (via `@volar/typescript`). The previous manifest extractor used a bespoke tsconfig loader that ignored `references`, so our root `tsconfig.json` (which has `files: []` and delegates everything to references) produced an empty TS program. The result was a manifest peppered with "No component file found" errors, identified in #77382 and the upstream issue storybookjs/storybook#34386. Also drop `EXPERIMENTAL_useProjectService: true` from the `react-docgen-typescript` options. Per investigation in #77382, this flag was the cause of a ~10x build slowdown. Removing it does not reintroduce the prop-extraction regression that #74807 added it to address. The `experimentalReactComponentMeta` flag only affects the manifest pipeline. The in-stories docs UI continues to use `react-docgen-typescript` through the React Vite framework's Vite plugin, including our existing `propFilter`. The new engine has its own source filtering for the manifest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Storybook 10.3 split prop extraction output across engine-specific manifest fields (`reactDocgen`, `reactDocgenTypescript`, and as of 10.4 `reactComponentMeta`). Deployed `@wordpress/design-system-mcp` clients were built against the 10.2 shape and read only the legacy `reactDocgen` field, so the manifest published to GitHub Pages needs to keep carrying that field for old clients to continue working. Add a post-build step that walks the built manifest and synthesizes a `reactDocgen` entry from `reactComponentMeta` data on each component (and subcomponent) that has one. The only field that actually has to change is the rename from `type` to `tsType`. Everything else passes through verbatim, which matches what the legacy field has historically carried (`description`, `displayName`, plus engine-specific extras). The shim is intentionally temporary. Once the next `@wordpress/design-system-mcp` release ships with parser support for the new shape and consumers have rolled forward through their `@latest` npm fetch (a few weeks given the 2-week publish cadence), the shim and its npm script wire-up can be deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Storybook 10.3 split the manifest's prop extraction output into engine-specific fields. Storybook 10.4 added `reactComponentMeta` for its new LanguageService-backed extractor, which we now use. New clients of this package only ever read manifests built from this codebase, so they can target the new field directly. Backwards compatibility for in-the-wild MCP clients still pinned to the previous shape is handled separately by the post-build shim in `storybook/scripts/inject-legacy-docgen.mjs`, which synthesizes a legacy `reactDocgen` field on the published manifest. The new shape uses `type` rather than `tsType` for prop types and has a nullable `defaultValue`. The parser is otherwise unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This is no longer necessary with React 19. Downstream components that access components package will do so through window.wp.components, which in older versions of WordPress will continue to use forwardRef approach, so this only interacts with environments where React 19 is available. This is needed to ensure that Storybook prop description extraction can effectively access the component's metadata, which forwardRef can interfere with.
Not seen locally, but local removal doesn't get reset on install. Might be caused by npm version difference
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
routes aren't structured this way with src directory, and there are no stories here currently. This includes pattern should reflect current stories covered
Always passing the argument anyways, and the script is meant to be temporary, meaning we shouldn't optimize for cases that we don't need. The fallback is wrong because the cwd when this script is evaluated is already inside `storybook`, so `storybook/` relative path would not exist.
Defer to React to provide typings, which we previously explicitly omit through `WordPressComponentProps`
Resolves issue with Storybook docgen documentation extraction inconsistency across web and manifest versions, while maintaining named components in React DevTools. The standalone export was never used.
aduth
force-pushed
the
update/storybook-10-3
branch
from
1223483 to
20b0949
Compare
|
Testing today, I notice the code samples for compound components only show the base name and not the full compound name, but this also happens in the current version. I thought we had tried to improve this kind of issue in the past (#78184, #78212), but that might have been more for the tabs names when viewing the default "Docs" story. 
|
|
I did a manual smoke test between local and current live site as well as an AI-assisted check (results below), and this appears to be in good shape now 👍 AI representative sample test resultsYes — checking all 232 docs pages isn’t realistic for a human or a one-off agent pass. A representative smoke set (~10–15 components covering different docgen patterns) is the right approach, and I ran that against both:
What I tested (12 components)
Results: local 10.4.3 vs live 10.2.8
*Divider timed out on the first automated pass (iframe load flake); a retry showed 5 props and matched live. Historical issues — status
No render errors on live. Local had one timeout (Divider only). Expected 10.4 differences (not regressions)
|
4 participants
What?
Updates Storybook to the latest version (10.4.3 at time of writing).
Why?
As identified in #77112, current component manifests produce incorrect results. One of the proposed action items there was to explore whether newer versions of Storybook and its related dependencies improve the outcome without workarounds like proposed in that pull request.
Additionally, this is also just general good code health maintenance to keep up to date with the latest features and bug fixes. Of note, Storybook 10.3 includes stable support for its MCP feature (which we indirectly leverage through the
componentsManifestfeature) as well as general accessibility improvements. Storybook 10.4 introduces a new, faster, more accurate "component meta" docgen tool which benefits the manifest.For the manifests (which, in turn, are used by
@wordpress/design-system-mcp) the difference is notable:Missing descriptions and prop details are particularly problematic for the MCP server, which relays this information to AI agents to ensure the agents understand appropriateness of a component and how it should be used. Increasing the availability of descriptions for parity with how these components are documented through the live Storybook site improves the reliability of these tools.
Full details with script (AI assisted)
Descriptions gained (8)
ConfirmDialogis built of top of [Modal](/packages/components/src/modal/REA…Renders a button that opens a floating content modal when clicked. ```jsx impor…
Popoverrenders its content in a floating modal. If no explicit anchor is pas…BaseControlis a low-level component used to generate labels and help text fo…ToggleGroupControlis a form component that lets users choose options represe…Truncateis a typography primitive that trims text content. For almost all ca…ScrollLock is a content-free React component for declaratively preventing scrol…
ItemGroupdisplays a list ofItems grouped and styled together. ```jsx impo…Props gained (42)
Props lost (regressions) (1)
Prop count changed (42)
Prop descriptions gained (7)
Script:
Testing Instructions
Verify that development and built outputs produce navigable stories, preserving existing expectations of Storybook that commonly regress in upgrades like like component descriptions, prop tables, code snippets.
Use of AI Tools
Used Claude Code and Claude Opus 4.7 to upgrade, iterate and analyze regressions, and explore workarounds to restore parity of documentation and build times.