Repository navigation
Vue: Run docgen through component-meta project manager - #35666
Conversation
f284601 to
03f6ea7
Compare
9b65bd2 to
2990f44
Compare
📝 WalkthroughWalkthroughVue 3 adds a project manager backed by ChangesVue 3 docgen integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StoryEntry
participant createDocgenProvider
participant VueComponentMetaManager
participant buildDocgenPayload
participant nextDocgen
StoryEntry->>createDocgenProvider: send docgen request
createDocgenProvider->>VueComponentMetaManager: get checker for component
VueComponentMetaManager-->>buildDocgenPayload: provide Vue checker
buildDocgenPayload->>nextDocgen: merge extracted payload with downstream result
Possibly related PRs
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
code/renderers/vue3/src/docgen/vue-project-manager.test.ts (2)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert checker/docgen behavior instead of parsed-command-line internals.
Both assertions couple the suite to the project manager’s direct-include implementation. Exercise
getCheckerForFile()or payload extraction for each fixture and assert metadata is available.
code/renderers/vue3/src/docgen/vue-project-manager.test.ts#L33-L36: replace thefileNamesassertion with successful checker-backed extraction for the referenced project.code/renderers/vue3/src/docgen/vue-project-manager.test.ts#L73-L82: replace thefileNamesassertion with successful checker-backed extraction for the no-includeVue fixture.As per coding guidelines, tests should verify public contracts and externally observable side effects rather than private implementation details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/renderers/vue3/src/docgen/vue-project-manager.test.ts` around lines 33 - 36, Replace the parsed-command-line fileNames assertions in code/renderers/vue3/src/docgen/vue-project-manager.test.ts at lines 33-36 and 73-82 with public-contract checks: for each fixture, call getCheckerForFile() or the existing payload-extraction path and assert that metadata is successfully returned, including the no-include Vue fixture.Source: Coding guidelines
85-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the established
memfstest setup.This writes an undeleted real temporary directory and makes the test dependent on host filesystem behavior. Seed the virtual SFC with
memfs, resetvolinbeforeEach, and use the established spy-redirection pattern.As per coding guidelines, filesystem tests using
node:fsornode:fs/promisesmust usememfs, resetvolinbeforeEach, and avoid real temporary directories.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/renderers/vue3/src/docgen/vue-project-manager.test.ts` around lines 85 - 92, Update the test setup around “serves files no tsconfig covers from the inferred project” to use the established memfs virtual filesystem instead of node:fs/promises, mkdtemp, and a real temporary directory. Reset vol in beforeEach, seed Loose.vue through memfs, and apply the existing spy-redirection pattern so filesystem access is fully virtualized.Source: Coding guidelines
code/renderers/vue3/src/docgen/vue-project-manager.ts (1)
158-171: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant tsconfig re-parse per created file in a batch.
For every
'created'change in the batch,getCommandLineFn?.()is invoked again (Line 161), re-running the fullparseCommandLine(multiple disk reads + JSON parses per earlier review of Lines 207-231). A batch of several newly created files (e.g. after a branch switch or dependency install touching watched paths) triggers that many redundant re-parses in a singleonFilesChangedcall.♻️ Proposed fix: fetch the command line once per batch
onFilesChanged(changes: FileChange[]): void { + let commandLine: ts.ParsedCommandLine | undefined; + let commandLineFetched = false; for (const { filePath, type } of changes) { const fileName = normalize(filePath); ... // created: - const commandLine = this.getCommandLineFn?.(); + if (!commandLineFetched) { + commandLine = this.getCommandLineFn?.(); + commandLineFetched = true; + } if (commandLine) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/renderers/vue3/src/docgen/vue-project-manager.ts` around lines 158 - 171, Update the onFilesChanged batch handling to obtain the command line once before iterating over created files, then reuse that value for each file’s inclusion check and this.commandLine assignment. Remove the per-file getCommandLineFn invocation while preserving adoption only for files present in the refreshed commandLine.fileNames.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/renderers/vue3/src/docgen/docgen-worker.ts`:
- Around line 55-68: Wrap the per-request extraction flow in the worker around
buildDocgenPayload, including manager.getCheckerForFile, with try/catch so
checker, tsconfig, and filesystem failures fall back to nextDocgen(input).
Preserve the existing heap-pressure recycling behavior and continue using the
successful payload path when extraction completes normally.
In `@code/renderers/vue3/src/docgen/vue-project-manager.test.ts`:
- Around line 22-25: Update the comment above the Vue project manager test to
retain only the maintenance rationale for walking the TypeScript reference chain
and locating the sub-config covering the component. Remove the description of
the previous fallback behavior, the “single-checker path” history, and the
upstream issue reference.
In `@code/renderers/vue3/src/docgen/vue-project-manager.ts`:
- Around line 100-130: Update ensureFresh so a file with no previous mtime is
refreshed immediately rather than only recording its current mtime. Reuse the
existing tryReadFile and checker.updateFile flow for this first sighting, then
store the resulting mtime; preserve the current behavior for unchanged and
previously tracked files.
---
Nitpick comments:
In `@code/renderers/vue3/src/docgen/vue-project-manager.test.ts`:
- Around line 33-36: Replace the parsed-command-line fileNames assertions in
code/renderers/vue3/src/docgen/vue-project-manager.test.ts at lines 33-36 and
73-82 with public-contract checks: for each fixture, call getCheckerForFile() or
the existing payload-extraction path and assert that metadata is successfully
returned, including the no-include Vue fixture.
- Around line 85-92: Update the test setup around “serves files no tsconfig
covers from the inferred project” to use the established memfs virtual
filesystem instead of node:fs/promises, mkdtemp, and a real temporary directory.
Reset vol in beforeEach, seed Loose.vue through memfs, and apply the existing
spy-redirection pattern so filesystem access is fully virtualized.
In `@code/renderers/vue3/src/docgen/vue-project-manager.ts`:
- Around line 158-171: Update the onFilesChanged batch handling to obtain the
command line once before iterating over created files, then reuse that value for
each file’s inclusion check and this.commandLine assignment. Remove the per-file
getCommandLineFn invocation while preserving adoption only for files present in
the refreshed commandLine.fileNames.
🪄 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 Plus
Run ID: 298dac4d-be62-4596-bded-fe63d967b6f4
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
code/renderers/vue3/build-config.tscode/renderers/vue3/package.jsoncode/renderers/vue3/src/docgen/__testfixtures__/references/src/RefButton.stories.tscode/renderers/vue3/src/docgen/__testfixtures__/references/src/RefButton.vuecode/renderers/vue3/src/docgen/__testfixtures__/references/tsconfig.app.jsoncode/renderers/vue3/src/docgen/__testfixtures__/references/tsconfig.jsoncode/renderers/vue3/src/docgen/docgen-worker.tscode/renderers/vue3/src/docgen/vue-project-manager.test.tscode/renderers/vue3/src/docgen/vue-project-manager.ts
03f6ea7 to
32043d5
Compare
2990f44 to
4a99e43
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 103 | 102 | 🎉 -1 🎉 |
| Self size | 30 KB | 30 KB | 🎉 -4 B 🎉 |
| Dependency size | 43.12 MB | 19.52 MB | 🎉 -23.60 MB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 90 | 90 | 0 |
| Self size | 75 KB | 97 KB | 🚨 +22 KB 🚨 |
| Dependency size | 18.08 MB | 18.08 MB | 🚨 +176 B 🚨 |
| Bundle Size Analyzer | Link | Link |
valentinpalkovic
left a comment
There was a problem hiding this comment.
Second review pass. For transparency: I paired this with an AI code-quality review (the Cursor "thermo-nuclear code quality review" skill), then filtered it down to only the findings that are genuinely new on top of my earlier comments. A couple are deliberately low-confidence questions rather than blockers - flagged inline. 🙂
32043d5 to
a6b7241
Compare
5521aa1 to
01ccf9a
Compare
ecfd085 to
39d8d4b
Compare
5fdbca4 to
90678c4
Compare
1c94c86 to
8d33d89
Compare
4045d20 to
2a68344
Compare
38157a4 to
57d6fe8
Compare
620078b to
49e84be
Compare
c3cbcf9 to
7d92b8e
Compare
The extracted manager put typescript-coupled types on core's public surface: `typeof ts` parameters and `ts.ParsedCommandLine` in the project contract. Consumers resolve their own typescript copy (vue3-vite even pins one as a runtime dependency), and across two copies `typeof ts` is not assignable (TS2345) while comparing `ts.ParsedCommandLine` structurally walks the whole compiler-API graph and overflows the checker (TS2321) — exactly the CI failure on this branch. Core's component-meta module now imports nothing from typescript: - `ProjectCommandLine` (fileNames + projectReferences paths) replaces `ts.ParsedCommandLine` in the contract; a `CL` generic on the factory and manager lets renderers keep full command-line fidelity internally. - The constructor takes a structural `ComponentMetaFileSystem` host (`sys.fileExists`/`sys.directoryExists` — all the manager uses); a real `typeof ts` still satisfies it. - `parseTsconfigCommandLine` moves into the React renderer, its only consumer, deleting the last cross-boundary `typeof ts`. Reproduced and verified with a dual-installation probe (typescript 6.0.3 on the contract side, 5.9.3 on the consumer side): the previous contract fails with the CI error verbatim; this contract compiles clean. React's public manager surface is unchanged; all 279 renderer tests pass as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit 745d2c4.
This reverts commit 745d2c4.
This reverts commit 745d2c4.
f802b9b to
f2aa70b
Compare
bd9a59b to
df12640
Compare
Core: Add lazy-docgen-middleware
…cgen_provider Vue: Expose docgen provider, inject in manifest and gate behind vue-component-meta only
Closes #
What I did
This PR implements
ComponentMetaManagerfor Vue and use it for docgen. It is known that Volar has issues with ts reference/monorepo resolutions and that the API of vue component meta requires THE tsconfig for a vue file we want to analmyse.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!
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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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
Tests