fix: resolve #34468 storybook eslint defineConfig issue#34487
Open
justismailmemon wants to merge 2 commits intostorybookjs:nextfrom
Open
fix: resolve #34468 storybook eslint defineConfig issue#34487justismailmemon wants to merge 2 commits intostorybookjs:nextfrom
justismailmemon wants to merge 2 commits intostorybookjs:nextfrom
Conversation
Contributor
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/cli/eslintPlugin.ts (1)
224-250:⚠️ Potential issue | 🟠 MajorHandle
export default configwhenconfigcomes fromtseslint.config(...).Case 3 only mutates raw arrays and
defineConfig([...])calls. The patternconst config = tseslint.config(...); export default config;is skipped, creating an inconsistency:export default tseslint.config(...)(Case 2) is rewritten, but the variable-assigned form is not.Fix
if (t.isIdentifier(eslintConfigExpression)) { const binding = path.scope.getBinding(eslintConfigExpression.name); if (binding && t.isVariableDeclarator(binding.path.node)) { const init = unwrapTSExpression(binding.path.node.init); if (t.isArrayExpression(init)) { init.elements.push(createStorybookConfigSpread()); + } else if ( + t.isCallExpression(init) && + t.isMemberExpression(init.callee) && + tsEslintLocalName && + t.isIdentifier(init.callee.object, { name: tsEslintLocalName }) && + t.isIdentifier(init.callee.property, { name: "config" }) + ) { + init.arguments.push(storybookConfig); } else if ( t.isCallExpression(init) && init.arguments.length > 0 && t.isIdentifier(init.callee) && eslintDefineConfigLocalName && init.callee.name === eslintDefineConfigLocalName ) {No test coverage exists for this pattern; add one as part of the fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/cli/eslintPlugin.ts` around lines 224 - 250, The code only handles variable-initialized arrays and defineConfig(...) calls but misses when the variable is initialized by a CallExpression like tseslint.config(...); update the block in eslintPlugin.ts that inspects binding.path.node.init inside the "if (t.isIdentifier(eslintConfigExpression))" branch to also detect when init is a CallExpression whose callee is a MemberExpression with object identifier "tseslint" and property name "config" (or the equivalent import identifier if you have one), then locate the array argument (unwrapTSExpression on the first argument) and push createStorybookConfigSpread({ castToAnyArray: shouldCastStorybookConfig }) into that array just like the existing defineConfig handling; add a unit test that covers "const config = tseslint.config([...]); export default config;" to ensure the transformation runs.
🤖 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/cli/eslintPlugin.ts`:
- Around line 255-275: The Program visitor currently always injects an ESM
import (constructed as importDecl) even for CommonJS flat config files because
isFlatConfig's pattern matches .cjs/.mjs and the rewrite only happens in the
ExportDefaultDeclaration path; fix this by introducing and using a boolean flag
(e.g., fileRewritten or rewrittenFlatConfig) that you set to true when the
ExportDefaultDeclaration rewrite succeeds, then change the Program(path) logic
to only insert the importDecl when alreadyImported is false AND that
rewrittenFlatConfig flag is true so CommonJS files that weren't rewritten won't
receive an ESM import.
---
Outside diff comments:
In `@code/core/src/cli/eslintPlugin.ts`:
- Around line 224-250: The code only handles variable-initialized arrays and
defineConfig(...) calls but misses when the variable is initialized by a
CallExpression like tseslint.config(...); update the block in eslintPlugin.ts
that inspects binding.path.node.init inside the "if
(t.isIdentifier(eslintConfigExpression))" branch to also detect when init is a
CallExpression whose callee is a MemberExpression with object identifier
"tseslint" and property name "config" (or the equivalent import identifier if
you have one), then locate the array argument (unwrapTSExpression on the first
argument) and push createStorybookConfigSpread({ castToAnyArray:
shouldCastStorybookConfig }) into that array just like the existing defineConfig
handling; add a unit test that covers "const config = tseslint.config([...]);
export default config;" to ensure the transformation runs.
🪄 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: b868c6c9-f3c1-4a1c-a596-bd25baafacd4
📒 Files selected for processing (1)
code/core/src/cli/eslintPlugin.ts
Comment on lines
255
to
275
| Program(path) { | ||
| const alreadyImported = path.node.body.some( | ||
| (node) => t.isImportDeclaration(node) && node.source.value === 'eslint-plugin-storybook' | ||
| (node) => | ||
| t.isImportDeclaration(node) && | ||
| node.source.value === "eslint-plugin-storybook", | ||
| ); | ||
|
|
||
| if (!alreadyImported) { | ||
| // Add import: import storybook from 'eslint-plugin-storybook' | ||
| // Add import: import storybook from "eslint-plugin-storybook" | ||
| const importDecl = t.importDeclaration( | ||
| [t.importDefaultSpecifier(t.identifier('storybook'))], | ||
| t.stringLiteral('eslint-plugin-storybook') | ||
| [t.importDefaultSpecifier(t.identifier("storybook"))], | ||
| t.stringLiteral("eslint-plugin-storybook"), | ||
| ); | ||
| (importDecl as any).comments = [ | ||
| { | ||
| type: 'CommentLine', | ||
| type: "CommentLine", | ||
| value: | ||
| ' For more info, see https://github.com/storybookjs/eslint-plugin-storybook#configuration-flat-config-format', | ||
| " For more info, see https://github.com/storybookjs/eslint-plugin-storybook#configuration-flat-config-format", | ||
| }, | ||
| ]; | ||
| path.node.body.unshift(importDecl); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- supported flat-config extensions --\n'
sed -n '18,27p' code/core/src/cli/eslintPlugin.ts
printf '\n-- export/default handling vs CommonJS handling --\n'
rg -n -C2 'ExportDefaultDeclaration|AssignmentExpression|module\.exports' code/core/src/cli/eslintPlugin.ts
printf '\n-- unconditional import insertion --\n'
sed -n '255,276p' code/core/src/cli/eslintPlugin.ts
printf '\n-- CommonJS flat-config coverage in tests --\n'
rg -n -C2 'module\.exports|eslint\.config\.cjs|eslint\.config\.js' code/core/src/cli/eslintPlugin.test.tsRepository: storybookjs/storybook
Length of output: 2988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full configureFlatConfig function
printf '\n-- Full configureFlatConfig function --\n'
sed -n '126,295p' code/core/src/cli/eslintPlugin.ts
# Check if there's any early return or condition that prevents import in CommonJS
printf '\n-- Looking for early returns or conditions around Program visitor --\n'
sed -n '240,280p' code/core/src/cli/eslintPlugin.tsRepository: storybookjs/storybook
Length of output: 7062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what "isFlatConfig" means in tests - does it include CommonJS flat configs?
printf '\n-- Test context for eslint.config.js (flat config) --\n'
sed -n '295,320p' code/core/src/cli/eslintPlugin.test.ts
# Check the CommonJS module.exports test case more carefully
printf '\n-- CommonJS module.exports test --\n'
sed -n '238,285p' code/core/src/cli/eslintPlugin.test.ts
# Look for any test that tests CommonJS with flat config
printf '\n-- Searching for flat config tests with CommonJS --\n'
rg -n -B5 -A10 'isFlatConfig.*true' code/core/src/cli/eslintPlugin.test.ts | head -80Repository: storybookjs/storybook
Length of output: 6207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the exact scenario: check if eslint.config.cjs could be treated as flat config
printf '\n-- How isFlatConfig is determined --\n'
rg -n -B5 -A5 'isFlatConfig' code/core/src/cli/eslintPlugin.ts | grep -A10 -B10 'extractEslintInfo\|eslint\.config'
# Check if a cjs file could trigger isFlatConfig = true
printf '\n-- File detection logic --\n'
sed -n '276,340p' code/core/src/cli/eslintPlugin.tsRepository: storybookjs/storybook
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the extractEslintInfo function to see how isFlatConfig is determined
printf '\n-- extractEslintInfo function (lines 276+) --\n'
sed -n '276,380p' code/core/src/cli/eslintPlugin.ts
# Look for any logic that checks file extensions to determine flat config
printf '\n-- File extension checks for flat config detection --\n'
rg -n 'eslint\.config|isFlatConfig' code/core/src/cli/eslintPlugin.ts | head -30Repository: storybookjs/storybook
Length of output: 3333
Don't add ESM import to CommonJS flat config files that weren't rewritten.
The isFlatConfig detection uses the pattern /eslint\.config\.[^/]+/, which matches .cjs and .mjs extensions in addition to .js and .ts. If a file like eslint.config.cjs uses CommonJS module.exports = [...], the Program visitor still unconditionally adds an ESM import statement even though the file was never rewritten (since only ExportDefaultDeclaration is handled). This creates invalid syntax and breaks ESLint.
Guard the import insertion to only run after a successful rewrite:
Fix
export const configureFlatConfig = async (
code: string,
eslintConfigFile?: string,
) => {
const ast = babelParse(code);
const shouldCastStorybookConfig = isTypeScriptEslintConfig(eslintConfigFile);
+ let didMutateFlatConfig = false;
traverse(ast, {
ExportDefaultDeclaration(path) {
// ... existing code ...
if (t.isArrayExpression(eslintConfigExpression)) {
eslintConfigExpression.elements.push(createStorybookConfigSpread());
+ didMutateFlatConfig = true;
}
if (t.isCallExpression(eslintConfigExpression) && /* tseslint.config */) {
eslintConfigExpression.arguments.push(storybookConfig);
+ didMutateFlatConfig = true;
}
if (t.isCallExpression(eslintConfigExpression) && /* defineConfig */) {
// ... existing code ...
if (unwrappedArg && t.isArrayExpression(unwrappedArg)) {
unwrappedArg.elements.push(...);
+ didMutateFlatConfig = true;
}
}
if (t.isIdentifier(eslintConfigExpression)) {
// ... existing code ...
if (t.isArrayExpression(init)) {
init.elements.push(createStorybookConfigSpread());
+ didMutateFlatConfig = true;
} else if (/* defineConfig case */) {
// ... existing code ...
if (unwrappedArg && t.isArrayExpression(unwrappedArg)) {
unwrappedArg.elements.push(...);
+ didMutateFlatConfig = true;
}
}
}
},
Program(path) {
const alreadyImported = path.node.body.some(
(node) =>
t.isImportDeclaration(node) &&
node.source.value === "eslint-plugin-storybook",
);
- if (!alreadyImported) {
+ if (didMutateFlatConfig && !alreadyImported) {
// Add import
const importDecl = t.importDeclaration(...);
// ... rest of code ...
}
},
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/cli/eslintPlugin.ts` around lines 255 - 275, The Program
visitor currently always injects an ESM import (constructed as importDecl) even
for CommonJS flat config files because isFlatConfig's pattern matches .cjs/.mjs
and the rewrite only happens in the ExportDefaultDeclaration path; fix this by
introducing and using a boolean flag (e.g., fileRewritten or
rewrittenFlatConfig) that you set to true when the ExportDefaultDeclaration
rewrite succeeds, then change the Program(path) logic to only insert the
importDecl when alreadyImported is false AND that rewrittenFlatConfig flag is
true so CommonJS files that weren't rewritten won't receive an ESM import.
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
1 participant
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.
Summary
Fixes #34468
This resolves the
defineConfigtype issue when addingstorybook.configs["flat/recommended"].What changed
eslintConfigFileinto flat config transformation so TS and JS can be handled correctlyWhy
The original fix solves the TypeScript
defineConfigerror, but usingas any[]everywhere would break JS config files. This change fixes the TS error without introducing that JS regression.Result
defineConfigtype error in TS configs.js,.mjs, and.cjsESLint config filesSummary by CodeRabbit
Bug Fixes
Refactor