Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/storybook-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ jobs:
uses: ./.github/setup-node

- name: Build Storybook
env:
NODE_ENV: test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know we were running Storybook-based tests?

@aduth aduth Jan 15, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know we were running Storybook-based tests?

This is specifically targeted at the conditions which already existed in the Storybook setup, but which never previously had anything to set the Node environment:

process.env.NODE_ENV !== 'test' ? './stories/**/*.story.@(jsx|tsx)' : '',
process.env.NODE_ENV !== 'test' ? './stories/**/*.mdx' : '',

But I hadn't actually understood the purpose, why those files specifically are excluded, and where the environment was originally meant to be applied.

This seems to stem back to using Storyshots (3cbb291), and I imagine the Jest pipeline would have been setting NODE_ENV. We no longer use Storyshots as of c84a55a, but the same general premise of "do the Stories load without crashing" still carries applies with @storybook/test-runner, we just never instrumented the NODE_ENV being applied.

As to the files themselves, my best guess is that excluding playground and .mdx files is a combination of (a) playground stories being complex in how they setup a full editor and may be flakier or non-performant and (b) .mdx files being plain documents that are unlikely to have any problems rendering compared to component Stories.

Judging by the fact that these files have been included in smoke tests since c84a55a, it clearly hasn't been too problematic to let them be tested regardless, so an argument could be made to just include all of them. I'd personally be inclined to keep the conditional inclusion, but maybe I'll at least add a code comment explaining the purpose.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.mdx files being plain documents that are unlikely to have any problems rendering compared to component Stories.

The original code in #18031 included a code comment explaining it a bit:

// StoryShots addon doesn't support MDX files at the moment.
// It should ignore the playground in the initial pass as well.

run: npm run storybook:build

- name: Install test-runner and Playwright dependencies
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/storybook-pages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ jobs:
uses: ./.github/setup-node

- name: Build Storybook
env:
NODE_ENV: production
run: npm run storybook:build

- name: Deploy
Expand Down
24 changes: 18 additions & 6 deletions storybook/main.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
import { mergeConfig, transformWithEsbuild } from 'vite';
import { type InlineConfig, mergeConfig, transformWithEsbuild } from 'vite';
import react from '@vitejs/plugin-react';
import type { StorybookConfig } from '@storybook/react-vite';

const { NODE_ENV = 'development' } = process.env;

const stories = [
process.env.NODE_ENV !== 'test' ? './stories/**/*.story.@(jsx|tsx)' : '',
process.env.NODE_ENV !== 'test' ? './stories/**/*.mdx' : '',
// Smoke tests ensure that the stories are rendered without any errors, but
// we don't need to test everything:
// - `.mdx` documentation is generally plain text and unlikely to break.
// - Playground stories are complex renderings of many components, which is
// both slow and redundant with individual component stories.
NODE_ENV === 'test' ? '' : './stories/playground/**/*.story.@(jsx|tsx)',
NODE_ENV === 'test' ? '' : './stories/**/*.mdx',
'../packages/block-editor/src/**/stories/*.story.@(js|jsx|tsx|mdx)',
'../packages/components/src/**/stories/*.story.@(jsx|tsx)',
'../packages/components/src/**/stories/*.mdx',
Expand Down Expand Up @@ -36,7 +43,9 @@ export default {
import.meta.resolve( './addons/design-system-theme/preset.ts' ),
],
framework: '@storybook/react-vite',
docs: {},
features: {
experimentalComponentsManifest: NODE_ENV === 'production',
},
typescript: {
reactDocgen: 'react-docgen-typescript',
// Should match defaults in Storybook except for the propFilter.
Expand Down Expand Up @@ -81,10 +90,13 @@ export default {
},
},
],
build: {
minify: NODE_ENV === 'production',
},
define: {
// Ensures that `@wordpress/warning` can properly detect dev mode.
'globalThis.SCRIPT_DEBUG': JSON.stringify(
process.env.NODE_ENV === 'development'
NODE_ENV === 'development'
),
},
optimizeDeps: {
Expand All @@ -94,6 +106,6 @@ export default {
},
},
},
} );
} satisfies InlineConfig );
Comment on lines -97 to +109

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity: This change applies type-checking and autocompletion for the custom options object, since mergeConfig is strangely typed with Record<string, any> as its second argument. The auto-completion support is a nice developer experience improvement and type-checking through satisfies helps guard against potential errors.

},
} satisfies StorybookConfig;
Loading