React: respect tsconfigPath in react-docgen-typescript#34406
Closed
bae080311 wants to merge 17 commits into
Closed
React: respect tsconfigPath in react-docgen-typescript#34406bae080311 wants to merge 17 commits into
bae080311 wants to merge 17 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional tsconfigPath to react-docgen TypeScript option types and threads it through parser initialization: resolves/reads the specified tsconfig (relative to process.cwd()), strips tsconfigPath from options passed to react-docgen-typescript, conditions parsing on successful read, and removes a debug log. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ParserModule as getParser / parseWithReactDocgenTypescript
participant FS as Resolver
participant TS as TypeScript API
participant Docgen as react-docgen-typescript Parser
Caller->>ParserModule: parseWithReactDocgenTypescript(filePath, userOptions)
ParserModule->>ParserModule: extract tsconfigPath and restUserOptions
alt tsconfigPath provided
ParserModule->>FS: resolve(process.cwd(), tsconfigPath)
FS-->>ParserModule: resolvedConfigPath
ParserModule->>TS: readConfigFile(resolvedConfigPath)
TS-->>ParserModule: { config, error }
alt no error
ParserModule->>TS: parseJsonConfigFileContent(config, resolvedConfigPath)
TS-->>ParserModule: { compilerOptions, fileNames }
ParserModule->>ParserModule: set cachedCompilerOptions / cachedFileNames
else error
ParserModule-->>Caller: log warning (resolvedConfigPath, error.messageText)
end
else no tsconfigPath
ParserModule->>TS: findTsconfigPath(process.cwd())
TS-->>ParserModule: detectedConfigPath
ParserModule->>TS: readConfigFile(detectedConfigPath)
TS-->>ParserModule: { config, error }
ParserModule->>TS: parseJsonConfigFileContent(config, detectedConfigPath)
TS-->>ParserModule: { compilerOptions, fileNames }
ParserModule->>ParserModule: set cachedCompilerOptions / cachedFileNames
end
ParserModule->>Docgen: create/return Parser(restUserOptions, cachedCompilerOptions)
Docgen-->>Caller: extracted docgen info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts (1)
195-211: Consider logging a warning when the user-providedtsconfigPathfails to load.When
typescript.readConfigFilereturns an error, the code silently falls back to default compiler options. If a user explicitly provides atsconfigPaththat cannot be read (e.g., typo, missing file), they receive no feedback about why their configuration isn't being applied.💡 Proposed improvement to add a warning
import { dirname, resolve } from 'node:path'; +import { logger } from 'storybook/internal/node-logger'; // ... in getParser function: if (configPath) { const { config, error } = typescript.readConfigFile(configPath, typescript.sys.readFile); if (!error) { const parsed = typescript.parseJsonConfigFileContent( config, typescript.sys, dirname(configPath) ); cachedCompilerOptions = { ...parsed.options, noErrorTruncation: true }; cachedFileNames = parsed.fileNames; + } else if (userTsconfigPath) { + // Only warn if user explicitly provided a path that failed to load + logger.warn(`Failed to load tsconfig at "${configPath}": ${error.messageText}`); } }🤖 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 195 - 211, When typescript.readConfigFile fails for a user-provided path (userTsconfigPath) the code silently falls back; add a warning that includes the failing path and the error details. Inside the existing if (configPath) block, handle the case where typescript.readConfigFile(configPath, typescript.sys.readFile) returns an error by calling console.warn (or the project's logger if available) with a clear message referencing userTsconfigPath/configPath and the error.message/object, then continue falling back to cachedCompilerOptions as before. This ensures failures to load user tsconfig are visible while preserving current behavior in the code around userOptions, configPath, and typescript.readConfigFile.
🤖 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 195-211: When typescript.readConfigFile fails for a user-provided
path (userTsconfigPath) the code silently falls back; add a warning that
includes the failing path and the error details. Inside the existing if
(configPath) block, handle the case where typescript.readConfigFile(configPath,
typescript.sys.readFile) returns an error by calling console.warn (or the
project's logger if available) with a clear message referencing
userTsconfigPath/configPath and the error.message/object, then continue falling
back to cachedCompilerOptions as before. This ensures failures to load user
tsconfig are visible while preserving current behavior in the code around
userOptions, configPath, and typescript.readConfigFile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a5f878e-9477-4056-919d-f4cc4a037286
📒 Files selected for processing (6)
code/frameworks/react-vite/src/types.tscode/presets/react-webpack/src/types.tscode/renderers/react/src/componentManifest/getComponentImports.tscode/renderers/react/src/componentManifest/reactDocgen/utils.tscode/renderers/react/src/componentManifest/reactDocgenTypescript.tsdocs/api/main-config/main-config-typescript.mdx
💤 Files with no reviewable changes (1)
- code/renderers/react/src/componentManifest/reactDocgen/utils.ts
Contributor
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/renderers/react/src/componentManifest/reactDocgenTypescript.ts`:
- Around line 213-215: Replace direct usage of error.messageText in the
logger.warn call with TypeScript's flattenDiagnosticMessageText to handle
DiagnosticMessageChain cases; in the branch that checks userTsconfigPath (the
else-if around logger.warn), call
typescript.flattenDiagnosticMessageText(error.messageText, '\n') (or
ts.flattenDiagnosticMessageText if `ts` is the imported alias) and pass that
result into the warning message so the log prints a readable string for both
string and DiagnosticMessageChain types.
🪄 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: 16c9e28b-352c-42d8-a497-1c915045bd86
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts
Contributor
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/renderers/react/src/componentManifest/reactDocgenTypescript.ts`:
- Around line 197-214: Reset cachedFileNames whenever you rebuild the parser
options so a stale file list isn't reused: when setting cachedCompilerOptions
(e.g., in the block around cachedCompilerOptions = { noErrorTruncation: true,
strict: true } and before calling
typescript.readConfigFile/parseJsonConfigFileContent), also set cachedFileNames
= undefined; additionally ensure that if typescript.readConfigFile returns an
error or configPath is falsy (including when userTsconfigPath is provided but
failing), cachedFileNames remains cleared so downstream createProgram cannot
consume a previous run’s fileNames; reference variables/functions:
cachedCompilerOptions, cachedFileNames, configPath, userTsconfigPath,
typescript.readConfigFile, typescript.parseJsonConfigFileContent, and
createProgram.
🪄 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: fd4c6d60-2877-4a11-a00d-f567b9a09c9c
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts (1)
185-241: Add regression tests fortsconfigPathbehavior branches.Please add automated coverage for: (1) explicit
tsconfigPathprecedence, and (2) invalidtsconfigPathwarning path without throw. This path is easy to regress and currently relies on manual verification.Also applies to: 303-306
🤖 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 185 - 241, Add automated tests that cover getParser's tsconfigPath branches: (1) a test asserting that when calling getParser with a user-provided tsconfigPath (userTsconfigPath) the code uses that file (precedence over findTsconfigPath) by mocking/resolving process.cwd and typescript.readConfigFile/parseJsonConfigFileContent to return expected parsed.options and parsed.fileNames and then asserting the created parser.program reflects those compiler options/fileNames; (2) a test for an invalid userTsconfigPath where typescript.readConfigFile returns an error—mock typescript.flattenDiagnosticMessageText and ensure logger.warn is called with the expected message and that getParser does not throw (it falls back to cachedCompilerOptions and returns a parser), using mocks for reactDocgenTypescript.withCompilerOptions to verify parser.fileParser is constructed.
🤖 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 185-241: Add automated tests that cover getParser's tsconfigPath
branches: (1) a test asserting that when calling getParser with a user-provided
tsconfigPath (userTsconfigPath) the code uses that file (precedence over
findTsconfigPath) by mocking/resolving process.cwd and
typescript.readConfigFile/parseJsonConfigFileContent to return expected
parsed.options and parsed.fileNames and then asserting the created
parser.program reflects those compiler options/fileNames; (2) a test for an
invalid userTsconfigPath where typescript.readConfigFile returns an error—mock
typescript.flattenDiagnosticMessageText and ensure logger.warn is called with
the expected message and that getParser does not throw (it falls back to
cachedCompilerOptions and returns a parser), using mocks for
reactDocgenTypescript.withCompilerOptions to verify parser.fileParser is
constructed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b942f41-25f3-4dfe-8fc1-4f6d456beea5
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/reactDocgenTypescript.ts
github-project-automation
Bot
moved this from Empathy Queue (prioritized)
to Done
in Core Team Projects
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34388
What I did
getParser()inreactDocgenTypescript.tsalways calledfindTsconfigPath(process.cwd()), completely ignoring thetsconfigPathvalue users pass throughreactDocgenTypescriptOptions. This broke monorepo setups wherethe root
tsconfig.jsonuses project references with emptyinclude/filesarrays — react-docgen-typescript found no filesand extracted no component documentation.
Changes:
getParser()now prefersuserOptions.tsconfigPath(resolved relative toprocess.cwd())and falls back to auto-detection when not set
readConfigFileerror before applyingparsed compiler options, so an invalid path doesn't crash
tsconfigPathis removed from optionsbefore forwarding to
react-docgen-typescriptsince it's not partof their
ParserOptionstypetsconfigPath?: stringtoreactDocgenTypescriptOptionsingetComponentImports.ts,react-vitetypes, andreact-webpackpreset typesmain-config-typescript.mdxwith a monorepo example
console.logfromreactDocgen/utils.tsChecklist 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. Best verified manually with a
monorepo as described below.
Manual testing
tsconfig.json(empty
include/files, onlyreferences)tsconfig.storybook.jsonthat explicitly includescomponent source files:
{ "extends": "./tsconfig.base.json", "include": ["packages/*/src/**/*.ts","packages//src/**/.tsx"]
}
3. Configure .storybook/main.ts:
typescript: {
reactDocgen: 'react-docgen-typescript',
reactDocgenTypescriptOptions: {
tsconfigPath: './tsconfig.storybook.json',
},
},
4. Run Storybook — component props should now be extracted
correctly in the Controls/Docs panel
Documentation
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Documentation