MDX: Replace @storybook/docs-mdx with inline implementation#34611
Conversation
Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/9ae1b60f-09f9-449b-ab81-d6766abb62e2 Co-authored-by: JReinhold <5678122+JReinhold@users.noreply.github.com>
Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/9ae1b60f-09f9-449b-ab81-d6766abb62e2 Co-authored-by: JReinhold <5678122+JReinhold@users.noreply.github.com>
Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/9ae1b60f-09f9-449b-ab81-d6766abb62e2 Co-authored-by: JReinhold <5678122+JReinhold@users.noreply.github.com>
Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/9ae1b60f-09f9-449b-ab81-d6766abb62e2 Co-authored-by: JReinhold <5678122+JReinhold@users.noreply.github.com>
@storybook/docs-mdx
JReinhold
changed the title
@storybook/docs-mdx@storybook/docs-mdx with inline implementation
|
@copilot replace the PR description with one that follows the official PR template at https://raw.githubusercontent.com/storybookjs/storybook/refs/heads/next/.github/PULL_REQUEST_TEMPLATE.md |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 1.67 MB | 1.25 MB | 🎉 -414 KB 🎉 |
| Dependency size | 9.26 MB | 9.26 MB | 🎉 -12 B 🎉 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 20.58 MB | 20.38 MB | 🎉 -205 KB 🎉 |
| Dependency size | 16.56 MB | 16.56 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 536 | 536 | 0 |
| Self size | 651 KB | 651 KB | 🚨 +78 B 🚨 |
| Dependency size | 60.94 MB | 60.97 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 93 | 93 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🚨 +78 B 🚨 |
| Dependency size | 23.78 MB | 23.81 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 122 | 122 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +18 B 🚨 |
| Dependency size | 24.85 MB | 24.88 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 83 | 83 | 0 |
| Self size | 36 KB | 36 KB | 🚨 +18 B 🚨 |
| Dependency size | 21.56 MB | 21.59 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 273 | 273 | 0 |
| Self size | 23 KB | 23 KB | 🚨 +12 B 🚨 |
| Dependency size | 45.54 MB | 45.56 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 839 KB | 839 KB | 0 B |
| Dependency size | 68.27 MB | 68.06 MB | 🎉 -205 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 66.79 MB | 66.58 MB | 🎉 -205 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 1.05 MB | 1.05 MB | 0 B |
| Dependency size | 37.14 MB | 36.93 MB | 🎉 -205 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 59 | 59 | 0 |
| Self size | 1.44 MB | 1.47 MB | 🚨 +28 KB 🚨 |
| Dependency size | 13.27 MB | 13.27 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced the external Storybook MDX analyzer with a local implementation. Added MDX parsing/dev dependencies, implemented ChangesLocal MDX Analyzer Implementation
Sequence Diagram(s)(Skipped — changes are internal module replacement and parsing logic; no multi-actor runtime sequence requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Review rate limit: 3/5 reviews remaining, refill in 16 minutes and 51 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/core-server/utils/docs-mdx.ts`:
- Around line 156-159: The duplicate Meta detection in the JSX/MDX parsing
(inside the block that checks if (name === 'Meta')) only checks result.title,
result.name, and result.of; update that condition to also include result.summary
and result.isTemplate (i.e., if (result.title || result.name || result.of ||
result.summary || result.isTemplate)) so any prior Meta with summary or
isTemplate triggers the "Meta can only be declared once" error; update the check
in the Meta handling code path (where the Error is thrown) and add unit tests
covering sequences like <Meta isTemplate /> followed by <Meta summary="..."/>
and vice versa to assert the duplicate error is thrown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b031d95-d4f8-4e4f-be44-a2a0d9d249b0
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
code/core/package.jsoncode/core/src/core-server/index.tscode/core/src/core-server/utils/StoryIndexGenerator.tscode/core/src/core-server/utils/docs-mdx.test.tscode/core/src/core-server/utils/docs-mdx.ts
|
The size increase is suspicious, but real. I'll need to investigate why this is happening. |
|
It's indeed real, here's some insight @JReinhold Before: After: |
|
We can see that But I suspect we're hitting that In this new iteration (this PR), it's changed to the full graph, un-minified. |
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/core/src/core-server/utils/analyze-mdx.ts (1)
237-238: 💤 Low valueConsider removing unnecessary async wrapper.
parseMdxandanalyzeParsedMdxare synchronous, so theasync/Promisewrapper adds overhead without benefit. If the previous@storybook/docs-mdxAPI was async for a reason (e.g., future async parsing), this is fine to keep for compatibility. Otherwise, simplifying to a sync function would be cleaner.♻️ Optional: Make synchronous if API compatibility isn't required
-export const analyzeMdx = async (code: string): Promise<MdxAnalysisResult> => - analyzeParsedMdx(parseMdx(code)); +export const analyzeMdx = (code: string): MdxAnalysisResult => + analyzeParsedMdx(parseMdx(code));Note: This would require updating callers to remove
await, so skip if maintaining the existing async API is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/analyze-mdx.ts` around lines 237 - 238, The analyzeMdx function currently wraps two synchronous calls (parseMdx and analyzeParsedMdx) in an unnecessary async/Promise, so change analyzeMdx to be synchronous by removing the async keyword and the Promise<MdxAnalysisResult> return type and returning the result of analyzeParsedMdx(parseMdx(code)) directly; if you choose to make this change, update any callers that await analyzeMdx to remove awaiting, otherwise keep the async signature for backward compatibility.code/core/src/core-server/utils/analyze-mdx.test.ts (1)
7-8: 💤 Low valueTests use Babel parser while implementation uses Acorn.
The
extractImportstests parse code with@babel/parserbut the actual implementation usesacorn. Both produce ESTree-compatible ASTs, so this works, but subtle differences could cause test/production divergence. Consider using acorn directly for more accurate testing.♻️ Optional: Use acorn parser for consistency
-import { parse } from '@babel/parser'; +import { Parser } from 'acorn'; +import acornJsx from 'acorn-jsx'; -const estreeParse = (code: string) => - parse(code, { sourceType: 'module', plugins: ['jsx', 'estree'] }).program; +const estreeParse = (code: string) => + Parser.extend(acornJsx()).parse(code, { + ecmaVersion: 2024, + sourceType: 'module', + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/analyze-mdx.test.ts` around lines 7 - 8, Tests currently parse code with `@babel/parser` in the estreeParse helper while the implementation uses acorn, risking subtle AST divergences; update the estreeParse function used by tests to parse with acorn (and acorn-jsx if JSX is needed) configured for module source type and ESTree-compatible output so the test AST matches the runtime AST consumed by extractImports, ensuring you replace the parse(...) call in estreeParse with an acorn-based parse invocation (including the JSX plugin/extension if JSX is present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/utils/analyze-mdx.test.ts`:
- Around line 7-8: Tests currently parse code with `@babel/parser` in the
estreeParse helper while the implementation uses acorn, risking subtle AST
divergences; update the estreeParse function used by tests to parse with acorn
(and acorn-jsx if JSX is needed) configured for module source type and
ESTree-compatible output so the test AST matches the runtime AST consumed by
extractImports, ensuring you replace the parse(...) call in estreeParse with an
acorn-based parse invocation (including the JSX plugin/extension if JSX is
present).
In `@code/core/src/core-server/utils/analyze-mdx.ts`:
- Around line 237-238: The analyzeMdx function currently wraps two synchronous
calls (parseMdx and analyzeParsedMdx) in an unnecessary async/Promise, so change
analyzeMdx to be synchronous by removing the async keyword and the
Promise<MdxAnalysisResult> return type and returning the result of
analyzeParsedMdx(parseMdx(code)) directly; if you choose to make this change,
update any callers that await analyzeMdx to remove awaiting, otherwise keep the
async signature for backward compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78e1984a-001d-4243-840d-f1c154bde535
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
code/core/package.jsoncode/core/src/core-server/index.tscode/core/src/core-server/utils/StoryIndexGenerator.tscode/core/src/core-server/utils/analyze-mdx.test.tscode/core/src/core-server/utils/analyze-mdx.tsscripts/build/utils/generate-bundle.ts
✅ Files skipped from review due to trivial changes (1)
- code/core/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/core-server/index.ts
- code/core/src/core-server/utils/StoryIndexGenerator.ts
|
I got completely nerd sniped here, and managed to both (theoretically) improve the speed of the MDX static analysis as well as remove 250kb from the existing solution (instead of increasing it by 500 kb) I noticed a couple of things:
It's hard to tell from the PR, but the unit tests stays exactly the same. |
Closes #
What I did
This PR inlines the
@storybook/docs-mdxanalyzer implementation into Storybook core and updates the core server to use that local implementation.It also removes the external
@storybook/docs-mdxdependency, adds the MDX parsing dependencies needed for the in-repo analyzer, and ports the upstream analyzer test coverage into core so behavior stays aligned with the original package.The inline implementation is slightly different from the original one. Originally we would compile the MDX with a custom extraction plugin, that would get all the information out, but without using the compilation output at all. This was wasteful, so instead it now just parses the MDX to AST, that we then extract information on. This is both faster and smaller (-250kb).
AI summary on benchmarks on our internal MDX files:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
See the Docs2-based stories in the main Storybook, that they have correct names and titles. In the
index.json, search foraddons-docs-docs2entries and see that tags, names and titles are correctly extracted.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Chores