React: Respect tsconfigPath option in react-docgen-typescript parser#34390
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes TypeScript config resolution for the React react-docgen-typescript parser used in component manifest/docgen extraction by honoring a user-supplied tsconfigPath (instead of always auto-detecting from process.cwd()), which unblocks solution-style monorepos.
Changes:
- Respect
reactDocgenTypescriptOptions.tsconfigPathby resolving it (relative toprocess.cwd()) and using it forreadConfigFile/parseJsonConfigFileContent. - Strip
tsconfigPathout of the options object before forwarding options toreact-docgen-typescript(since it’s not part of theirParserOptions). - Keep existing auto-detection behavior (
findTsconfigPath(process.cwd())) whentsconfigPathisn’t provided.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughPrefer a user-provided Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
|
Hi @Muhammad-Nur-Alamsyah-Anwar, Thank you for your contribution. I've looked up the issue and the mentioned configuration: Currently, We should update the documentation and internal typings (by removing the custom alias) to officially support the option. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts (1)
203-209:⚠️ Potential issue | 🟠 MajorAdd error handling for tsconfig read/parse before building TypeScript program.
When
configPathis provided (lines 203–208),readConfigFile()can return errors for invalid paths or malformed JSON, but the error is discarded via destructuring andconfigwill be undefined. This flows intoparseJsonConfigFileContent()without validation. With customtsconfigPathnow supported, users can easily hit this path with invalid input, causing unclear failures downstream.The correct pattern already exists elsewhere in the codebase (
reactDocgen/utils.ts): checkreadResult.errorbefore using the config, and also validateparseJsonConfigFileContent().errorsto surface issues early.Suggested hardening
if (configPath) { - const { config } = typescript.readConfigFile(configPath, typescript.sys.readFile); - const parsed = typescript.parseJsonConfigFileContent( - config, + const readResult = typescript.readConfigFile(configPath, typescript.sys.readFile); + if (readResult.error || !readResult.config) { + throw new Error(`Failed to read tsconfig at: ${configPath}`); + } + const parsed = typescript.parseJsonConfigFileContent( + readResult.config, typescript.sys, dirname(configPath) ); + if (parsed.errors.length > 0) { + throw new Error(`Failed to parse tsconfig at: ${configPath}`); + } cachedCompilerOptions = { ...parsed.options, noErrorTruncation: true }; cachedFileNames = parsed.fileNames; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/reactDocgenTypescript.ts` around lines 203 - 209, When configPath is provided, validate the result of typescript.readConfigFile (do not destructure directly); capture its return value (e.g., readResult) and if readResult.error is present surface/throw a clear error before calling typescript.parseJsonConfigFileContent; after parsing, check parsed.errors and surface/throw those errors as well so you never pass an undefined config into typescript.parseJsonConfigFileContent or proceed to build the TypeScript program; update the logic around configPath, typescript.readConfigFile and typescript.parseJsonConfigFileContent in reactDocgenTypescript.ts accordingly to fail fast with clear messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/renderers/react/src/componentManifest/reactDocgenTypescript.ts`:
- Around line 203-209: When configPath is provided, validate the result of
typescript.readConfigFile (do not destructure directly); capture its return
value (e.g., readResult) and if readResult.error is present surface/throw a
clear error before calling typescript.parseJsonConfigFileContent; after parsing,
check parsed.errors and surface/throw those errors as well so you never pass an
undefined config into typescript.parseJsonConfigFileContent or proceed to build
the TypeScript program; update the logic around configPath,
typescript.readConfigFile and typescript.parseJsonConfigFileContent in
reactDocgenTypescript.ts accordingly to fail fast with clear messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 690b3f81-5a4e-4e09-9b72-26ef8b437247
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts
|
Fixed formatting and added error handling for invalid tsconfig files (matches existing pattern in utils.ts). Let me know if you'd like me to add docs/types for the tsconfigPath option or if you'll handle that separately. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts (1)
203-214: Consider logging a warning when tsconfig cannot be read.When
typescript.readConfigFilereturns an error, the code silently falls back to default compiler options. This could cause confusing behavior for users who provide an explicittsconfigPaththat fails to parse — they may not realize their config isn't being used.Consider adding a warning using Storybook's logger:
💡 Optional improvement
+import { logger } from 'storybook/internal/node-logger'; + // ... in getParser function: if (configPath) { const { config, error } = typescript.readConfigFile(configPath, typescript.sys.readFile); - if (!error && config) { + if (error) { + logger.warn(`Failed to read tsconfig at ${configPath}: ${error.messageText}`); + } else if (config) { const parsed = typescript.parseJsonConfigFileContent( config, typescript.sys, dirname(configPath) ); cachedCompilerOptions = { ...parsed.options, noErrorTruncation: true }; cachedFileNames = parsed.fileNames; } }As per coding guidelines: "Use Storybook loggers (storybook/internal/node-logger for server-side) instead of raw console.* methods in normal code paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/reactDocgenTypescript.ts` around lines 203 - 214, The code silently ignores errors from typescript.readConfigFile in reactDocgenTypescript.ts; update the block that calls typescript.readConfigFile to log a warning when an error is returned (including the error message and the configPath) instead of silently falling back, using Storybook's server logger (e.g., the node/server logger import used across the project such as logger from storybook/internal/node-logger or `@storybook/node-logger`), and keep existing behavior of using default cachedCompilerOptions/cachedFileNames when config parsing fails; refer to the readConfigFile call, the parsed/ error destructuring, and the cachedCompilerOptions/cachedFileNames variables to place the warning.
🤖 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/renderers/react/src/componentManifest/reactDocgenTypescript.ts`:
- Around line 203-214: The code silently ignores errors from
typescript.readConfigFile in reactDocgenTypescript.ts; update the block that
calls typescript.readConfigFile to log a warning when an error is returned
(including the error message and the configPath) instead of silently falling
back, using Storybook's server logger (e.g., the node/server logger import used
across the project such as logger from storybook/internal/node-logger or
`@storybook/node-logger`), and keep existing behavior of using default
cachedCompilerOptions/cachedFileNames when config parsing fails; refer to the
readConfigFile call, the parsed/ error destructuring, and the
cachedCompilerOptions/cachedFileNames variables to place the warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82fdc2c0-4c2e-4d30-92e8-4835e3ee4419
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts
|
Please add docs/types for the tsconfigPath option. That would be amazing. Thanks! |
|
@Muhammad-Nur-Alamsyah-Anwar Did you see the comment of @valentinpalkovic? |
|
Closing due to inactivity |
Closes #34386
What I did
getParser()inreactDocgenTypescript.tswas hardcoded to always callfindTsconfigPath(process.cwd())for locating the TypeScript config. It completely ignored thetsconfigPathvalue that users can pass throughreactDocgenTypescriptOptionsin their Storybook config.This broke monorepo setups where the root
tsconfig.jsonuses project references with emptyinclude/filesarrays — the auto-detected tsconfig had no files listed, soreact-docgen-typescriptcouldn't extract any component documentation.The fix pulls
tsconfigPathout of the user options and resolves it relative toprocess.cwd()when provided. When it's not set, the existing auto-detection behavior kicks in as before. I also made suretsconfigPathgets stripped from the options object before it's forwarded toreact-docgen-typescript, since it's not part of theirParserOptionstype.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
No automated tests added — the change is a small conditional branch in config resolution (
tsconfigPathprovided vs auto-detected), and the existing parser behavior is unchanged when the option isn't set. Best verified manually with an actual monorepo setup as described below.Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
tsconfig.json(emptyinclude/files, onlyreferences)tsconfig.storybook.jsonthat explicitly includes the component source files:{ "extends": "./tsconfig.base.json", "include": ["packages/*/src/**/*.ts", "packages/*/src/**/*.tsx"] }.storybook/main.ts:Without this fix, step 4 shows no props because the auto-detected root tsconfig has no files to parse.
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
Improvements