Storybook: Upgrade Storybook to 10.4#77382
Conversation
|
Size Change: -16 B (0%) Total Size: 8.21 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 8129b89. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/26776943834
|
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. |
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.
72955b0 to
336edb4
Compare
|
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:
|
Not seen locally, but local removal doesn't get reset on install. Might be caused by npm version difference
| ### `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
|
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. |
|
Updated script with percentage of coverage for descriptions and props on components:
Script (AI-produced)#!/usr/bin/env node
/**
* Compare a freshly built components manifest against the one currently
* hosted at `wordpress.github.io/gutenberg/manifests/components.json`,
* to evaluate how much the upgraded prop extractor recovers compared
* to what's live today.
*
* Usage:
* node storybook/scripts/compare-with-hosted-manifest.mjs [path]
*
* The local manifest defaults to `storybook/build/manifests/components.json`.
* The hosted manifest is read from `reactDocgen` (Storybook 10.2 shape).
* The local manifest is read from `reactComponentMeta` (Storybook 10.4),
* so the comparison reflects the real engine output rather than the
* synthesized legacy field added by `inject-legacy-docgen.mjs`.
*
* Intended as a one-shot validation aid. Not wired into any build.
*/
import { readFile } from 'node:fs/promises';
const HOSTED_URL =
'https://wordpress.github.io/gutenberg/manifests/components.json';
const LOCAL_PATH =
process.argv[ 2 ] ?? 'storybook/build/manifests/components.json';
function getDocgen( component, legacy ) {
if ( legacy ) {
return component.reactDocgen;
}
return component.reactComponentMeta;
}
function getProps( component, legacy ) {
return getDocgen( component, legacy )?.props ?? {};
}
function descriptionFor( component, legacy ) {
return (
component.description ||
getDocgen( component, legacy )?.description ||
''
).trim();
}
function propDescriptionCount( props ) {
return Object.values( props ).filter( ( p ) =>
( p.description ?? '' ).trim()
).length;
}
function summarize( label, components, legacy ) {
const entries = Object.values( components );
let withDescription = 0;
let withProps = 0;
let totalProps = 0;
let propsWithDescription = 0;
for ( const component of entries ) {
if ( descriptionFor( component, legacy ) ) {
withDescription += 1;
}
const props = getProps( component, legacy );
const propCount = Object.keys( props ).length;
if ( propCount > 0 ) {
withProps += 1;
}
totalProps += propCount;
propsWithDescription += propDescriptionCount( props );
}
return {
label,
total: entries.length,
withDescription,
withProps,
totalProps,
propsWithDescription,
};
}
function formatSummary( old, fresh ) {
// Each row: [label, hostedCount, newCount, hostedDenominator, newDenominator].
// Denominator left undefined when a percentage doesn't apply.
const rows = [
[ 'Components', old.total, fresh.total ],
[
'With description',
old.withDescription,
fresh.withDescription,
old.total,
fresh.total,
],
[
'With prop data',
old.withProps,
fresh.withProps,
old.total,
fresh.total,
],
[ 'Total props', old.totalProps, fresh.totalProps ],
[
'Props with description',
old.propsWithDescription,
fresh.propsWithDescription,
old.totalProps,
fresh.totalProps,
],
];
const labelWidth = 24;
const numWidth = 10;
const pctWidth = 10;
const fmtPct = ( num, denom ) =>
denom > 0 ? `${ ( ( num / denom ) * 100 ).toFixed( 1 ) }%` : '';
const lines = [
`${ 'Metric'.padEnd( labelWidth ) }${ 'Hosted'.padStart(
numWidth
) }${ 'New'.padStart( numWidth ) }${ 'Δ'.padStart(
numWidth
) }${ 'Hosted%'.padStart( pctWidth ) }${ 'New%'.padStart(
pctWidth
) }${ 'Δ%'.padStart( pctWidth ) }`,
'-'.repeat( labelWidth + numWidth * 3 + pctWidth * 3 ),
];
for ( const [ name, a, b, denomA, denomB ] of rows ) {
const delta = b - a;
const sign = delta > 0 ? '+' : '';
const pctA = fmtPct( a, denomA );
const pctB = fmtPct( b, denomB );
const deltaPct =
pctA && pctB
? ( () => {
const d =
( b / denomB ) * 100 - ( a / denomA ) * 100;
return `${ d > 0 ? '+' : '' }${ d.toFixed( 1 ) }`;
} )()
: '';
lines.push(
`${ name.padEnd( labelWidth ) }${ String( a ).padStart(
numWidth
) }${ String( b ).padStart( numWidth ) }${ (
sign + delta
).padStart( numWidth ) }${ pctA.padStart(
pctWidth
) }${ pctB.padStart( pctWidth ) }${ deltaPct.padStart(
pctWidth
) }`
);
}
return lines.join( '\n' );
}
function listMissingDescriptions( components ) {
return Object.values( components )
.filter( ( c ) => {
if ( ! c.path?.startsWith( '../packages/' ) ) {
return false;
}
const desc = ( c.description ?? '' ).trim();
const rcmDesc = ( c.reactComponentMeta?.description ?? '' ).trim();
return ! desc && ! rcmDesc;
} )
.map( ( c ) => `${ c.name } — ${ c.path }` )
.sort();
}
function indexByKey( components ) {
const result = new Map();
for ( const component of Object.values( components ) ) {
const key = `${ component.path }:${ component.name }`;
result.set( key, component );
}
return result;
}
function diffPerComponent( oldIndex, newIndex ) {
const findings = {
descriptionGained: [],
descriptionLost: [],
descriptionChanged: [],
propsGained: [],
propsLost: [],
propsCountChanged: [],
propDescriptionsGained: [],
propDescriptionsLost: [],
};
for ( const [ key, fresh ] of newIndex ) {
const old = oldIndex.get( key );
if ( ! old ) {
continue;
}
const oldDesc = descriptionFor( old, true );
const newDesc = descriptionFor( fresh, false );
if ( ! oldDesc && newDesc ) {
findings.descriptionGained.push( { key, newDesc } );
} else if ( oldDesc && ! newDesc ) {
findings.descriptionLost.push( { key, oldDesc } );
} else if ( oldDesc && newDesc && oldDesc !== newDesc ) {
findings.descriptionChanged.push( { key, oldDesc, newDesc } );
}
const oldProps = getProps( old, true );
const newProps = getProps( fresh, false );
const oldNames = new Set( Object.keys( oldProps ) );
const newNames = new Set( Object.keys( newProps ) );
const added = [ ...newNames ].filter( ( n ) => ! oldNames.has( n ) );
const removed = [ ...oldNames ].filter( ( n ) => ! newNames.has( n ) );
if ( added.length ) {
findings.propsGained.push( { key, names: added } );
}
if ( removed.length ) {
findings.propsLost.push( { key, names: removed } );
}
if ( oldNames.size !== newNames.size ) {
findings.propsCountChanged.push( {
key,
before: oldNames.size,
after: newNames.size,
} );
}
const commonProps = [ ...oldNames ].filter( ( n ) =>
newNames.has( n )
);
const gainedDescriptions = [];
const lostDescriptions = [];
for ( const name of commonProps ) {
const had = ( oldProps[ name ].description ?? '' ).trim();
const has = ( newProps[ name ].description ?? '' ).trim();
if ( ! had && has ) {
gainedDescriptions.push( name );
} else if ( had && ! has ) {
lostDescriptions.push( name );
}
}
if ( gainedDescriptions.length ) {
findings.propDescriptionsGained.push( {
key,
names: gainedDescriptions,
} );
}
if ( lostDescriptions.length ) {
findings.propDescriptionsLost.push( {
key,
names: lostDescriptions,
} );
}
}
return findings;
}
function formatList( title, items, render ) {
if ( ! items.length ) {
return '';
}
return [
'',
`## ${ title } (${ items.length })`,
'',
...items.map( render ),
].join( '\n' );
}
function truncate( text, max = 80 ) {
const flat = text.replace( /\s+/g, ' ' );
return flat.length > max ? `${ flat.slice( 0, max - 1 ) }…` : flat;
}
const [ hostedManifest, localRaw ] = await Promise.all( [
fetch( HOSTED_URL ).then( ( r ) => {
if ( ! r.ok ) {
throw new Error( `Failed to fetch ${ HOSTED_URL }: ${ r.status }` );
}
return r.json();
} ),
readFile( LOCAL_PATH, 'utf8' ).then( JSON.parse ),
] );
const oldIndex = indexByKey( hostedManifest.components ?? {} );
const newIndex = indexByKey( localRaw.components ?? {} );
const oldOnly = [ ...oldIndex.keys() ].filter( ( k ) => ! newIndex.has( k ) );
const newOnly = [ ...newIndex.keys() ].filter( ( k ) => ! oldIndex.has( k ) );
const oldStats = summarize( 'hosted', hostedManifest.components ?? {}, true );
const newStats = summarize( 'new', localRaw.components ?? {}, false );
const findings = diffPerComponent( oldIndex, newIndex );
console.log( '# Manifest comparison' );
console.log( '' );
console.log( `Hosted: ${ HOSTED_URL }` );
console.log( `Local: ${ LOCAL_PATH }` );
console.log( '' );
console.log( formatSummary( oldStats, newStats ) );
console.log(
formatList(
'Components still missing descriptions (new manifest)',
listMissingDescriptions( localRaw.components ?? {} ),
( k ) => `- ${ k }`
)
);
console.log(
formatList(
'Components only in hosted manifest',
oldOnly,
( k ) => `- ${ k }`
)
);
console.log(
formatList(
'Components only in new manifest',
newOnly,
( k ) => `- ${ k }`
)
);
console.log(
formatList(
'Descriptions gained',
findings.descriptionGained,
( { key, newDesc } ) => `- ${ key }\n ${ truncate( newDesc ) }`
)
);
console.log(
formatList(
'Descriptions lost (regressions)',
findings.descriptionLost,
( { key, oldDesc } ) => `- ${ key }\n was: ${ truncate( oldDesc ) }`
)
);
console.log(
formatList(
'Descriptions changed',
findings.descriptionChanged,
( { key, oldDesc, newDesc } ) =>
`- ${ key }\n - ${ truncate( oldDesc ) }\n + ${ truncate(
newDesc
) }`
)
);
console.log(
formatList(
'Props gained',
findings.propsGained,
( { key, names } ) =>
`- ${ key }: ${ names.slice( 0, 10 ).join( ', ' ) }${
names.length > 10 ? ` (+${ names.length - 10 } more)` : ''
}`
)
);
console.log(
formatList(
'Props lost (regressions)',
findings.propsLost,
( { key, names } ) =>
`- ${ key }: ${ names.slice( 0, 10 ).join( ', ' ) }${
names.length > 10 ? ` (+${ names.length - 10 } more)` : ''
}`
)
);
console.log(
formatList(
'Prop count changed',
findings.propsCountChanged,
( { key, before, after } ) => `- ${ key }: ${ before } → ${ after }`
)
);
console.log(
formatList(
'Prop descriptions gained',
findings.propDescriptionsGained,
( { key, names } ) =>
`- ${ key }: ${ names.slice( 0, 10 ).join( ', ' ) }${
names.length > 10 ? ` (+${ names.length - 10 } more)` : ''
}`
)
);
console.log(
formatList(
'Prop descriptions lost (regressions)',
findings.propDescriptionsLost,
( { key, names } ) =>
`- ${ key }: ${ names.slice( 0, 10 ).join( ', ' ) }${
names.length > 10 ? ` (+${ names.length - 10 } more)` : ''
}`
)
);Although many of those missing descriptions are because we haven't written them 🙃
|
| ### `ref` | ||
|
|
||
| - Type: `Ref<any>` | ||
| - Required: No | ||
|
|
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?
| * REMOVEME: This shim can be removed once `@wordpress/design-system-mcp` has | ||
| * been published, since installation instructions for the MCP server point to | ||
| * the latest version automatically. |
| ### `ref` | ||
|
|
||
| - Type: `Ref<any>` | ||
| - Required: No | ||
|
|
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, ...)
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.
|
Another side benefit of this change is that the manifest is now "prettified" (tabs and newlines) by default, so linking to https://wordpress.github.io/gutenberg/manifests/components.json won't be as much of a monstrosity for people to try to grok 😅 |
Defer to React to provide typings, which we previously explicitly omit through `WordPressComponentProps`
What?
Updates Storybook to the latest version (10.4.1 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.