AutoMigration: Fix sb10 migration when main config contains require
#32558
Conversation
… in automigrate fixes
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a runtime fallback in loadMainConfig to handle "require is not defined" errors by reading the main config, detecting whether a compatibility banner is already present, and when absent creating a temporary file that prepends an ES-module-compatible header (providing __filename, __dirname, and a require shim via createRequire), dynamically importing the temp file, and deleting it; if the banner exists the original error is rethrown. Introduced an automigrate fix Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Loader as loadMainConfig
participant FS as FS (readFile/writeFile/rm)
participant Node as Node Runtime (import/eval)
Caller->>Loader: loadMainConfig(mainPath)
activate Loader
Loader->>Node: dynamic import(mainPath)
alt Import succeeds
Node-->>Loader: module
Loader-->>Caller: return module
else Error includes "require is not defined"
Note right of Loader #f6f8ff: Read main file and\ncheck for existing banner
Loader->>FS: readFile(mainPath)
FS-->>Loader: originalContent
alt Banner already present
Loader-->>Caller: throw MainFileEvaluationError
else Banner absent
Note right of Loader #e8f5e9: Create header with __filename,\n__dirname, and createRequire shim
Loader->>FS: writeFile(tempPath = mainPath + ".tmp.<ext>", modifiedContent)
FS-->>Loader: ok
Loader->>Node: dynamic import(tempPath)
Node-->>Loader: module
Loader->>FS: rm(tempPath)
FS-->>Loader: ok
Loader-->>Caller: return module
end
else Other error
Loader-->>Caller: propagate error
end
deactivate Loader
sequenceDiagram
autonumber
participant CLI as Automigrate CLI
participant Fix as fix-faux-esm-require
participant FS as FS (readFile/writeFile)
CLI->>Fix: check({ mainConfigPath })
alt no path / not ESM / no require / already bannered
Fix-->>CLI: null
else applicable
Fix-->>CLI: { storybookVersion, opts }
CLI->>Fix: prompt()
Fix-->>CLI: user message
CLI->>Fix: run({ dryRun, mainConfigPath })
alt dryRun
Fix-->>CLI: report (no file write)
else apply
Fix->>FS: readFile(mainConfigPath)
FS-->>Fix: content
Fix->>FS: writeFile(mainConfigPath, bannerComment + getRequireBanner() + content)
FS-->>Fix: ok
Fix-->>CLI: applied
end
end
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit f635045
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
3-11: Remove unused import loadMainConfig.Now that readConfig is used, loadMainConfig is unused in this module.
Apply:
getStorybookInfo, - loadMainConfig, rendererPackages,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/lib/cli-storybook/src/automigrate/fixes/index.ts(1 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
code/lib/cli-storybook/src/automigrate/fixes/index.ts (1)
code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.ts (1)
wrapGetAbsolutePath(24-84)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
code/core/src/csf-tools/ConfigFile.ts (1)
ConfigFile(147-1160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
207-230: updateMainConfig flow LGTM.Using readConfig/writeConfig with the ConfigFile callback is consistent with other fixes and safely guarded with logging.
code/lib/cli-storybook/src/automigrate/fixes/index.ts (1)
22-39: No ordering dependencies found; moving wrapGetAbsolutePath first is safe.
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
Outdated
Show resolved
Hide resolved
…lutePath in automigrate fixes" This reverts commit 24f1e61.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/common/utils/load-main-config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/common/utils/load-main-config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/common/utils/load-main-config.ts
🧬 Code graph analysis (1)
code/core/src/common/utils/load-main-config.ts (2)
scripts/utils/tools.ts (1)
dedent(118-118)code/frameworks/svelte-vite/src/plugins/generateDocgen.ts (2)
readFile(276-287)writeFile(387-389)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/common/utils/load-main-config.ts (1)
1-1: LGTM on fs/promises usageImporting readFile/writeFile from node:fs/promises is appropriate for the async I/O below.
…ove error handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/common/utils/load-main-config.ts (2)
51-56: Ensure the temp main file is always cleaned up.If
importModule(modifiedMainPath)throws, we never reach thermcall, so the straymain.tmp..*file lingers in the config directory. Wrap the import in atry/finallyand always attempt to delete the temp file (best-effort) to avoid leaving debris around failed runs.Apply this diff:
- const out = await importModule(modifiedMainPath); - await rm(modifiedMainPath); - return out; + try { + return await importModule(modifiedMainPath); + } finally { + await rm(modifiedMainPath).catch(() => {}); + }
52-52: Tidy up the temporary filename.
path.parse().extalready contains the leading dot, so interpolating it as${name}.tmp.${ext}producesmain.tmp..ts. It still works, but tightening it to${name}.tmp${ext}avoids the awkward double dot and keeps the extension exactly what downstream tooling expects.Apply this diff:
- const modifiedMainPath = join(dir, `${name}.tmp.${ext}`); + const modifiedMainPath = join(dir, `${name}.tmp${ext}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/common/utils/load-main-config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/common/utils/load-main-config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/common/utils/load-main-config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/core/src/common/utils/load-main-config.ts
🧬 Code graph analysis (1)
code/core/src/common/utils/load-main-config.ts (1)
code/core/src/shared/utils/module.ts (1)
importModule(54-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/common/utils/load-main-config.ts (1)
8-8: Fix thets-dedentimport (default export).
ts-dedentonly exposes a default export; the named import resolves toundefinedat runtime and will trip strict TS configs. Please switch to the default import.Apply this diff:
-import { dedent } from 'ts-dedent'; +import dedent from 'ts-dedent';
- Introduced `fix-faux-esm-require` utility to handle ESM files with require references. - Added tests for the new utility to ensure correct behavior in various scenarios. - Updated main configuration helpers to include functions for ESM detection and require usage checks. - Removed unused import from `addon-a11y-addon-test.test.ts`.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.test.ts(0 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(1 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(1 hunks)code/lib/cli-storybook/src/automigrate/fixes/index.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
💤 Files with no reviewable changes (1)
- code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/lib/cli-storybook/src/automigrate/fixes/index.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/lib/cli-storybook/src/automigrate/fixes/index.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
🧠 Learnings (10)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to test-storybooks/** : Maintain test configurations and assets under test-storybooks/ for Storybook testing
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without vi.mocked()
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should be placed in beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
🧬 Code graph analysis (4)
code/lib/cli-storybook/src/automigrate/fixes/index.ts (1)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
fixFauxEsmRequire(18-68)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (4)
containsESMUsage(233-260)hasRequireBanner(275-279)containsRequireUsage(263-272)getRequireBanner(282-292)code/lib/cli-storybook/src/automigrate/types.ts (1)
Fix(52-60)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (2)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
fixFauxEsmRequire(18-68)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (4)
containsESMUsage(233-260)containsRequireUsage(263-272)hasRequireBanner(275-279)getRequireBanner(282-292)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
scripts/utils/tools.ts (1)
dedent(118-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/lib/cli-storybook/src/automigrate/fixes/index.ts (1)
10-41: New fix wired into automigrations correctly.Importing and registering
fixFauxEsmRequireinallFixesensures it runs with the rest of the suite. Looks good.
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
Outdated
Show resolved
Hide resolved
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
Outdated
Show resolved
Hide resolved
- Updated `containsESMUsage` function to accept only content as an argument, removing file path dependency. - Enhanced `fixFauxEsmRequire` to streamline checks for ESM syntax and require usage. - Removed unnecessary `isConfigTypescript` option from `FixFauxEsmRequireRunOptions` as it is no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
33-33: Remove stray console.log or switch to logger.debug.Avoid noisy stdout in migrations.
Apply:
- console.log({ isESM, isWithRequire, isWithBanner }); + // optional: use logger.debug if needed + // logger.debug(`fix-faux-esm-require flags: ${JSON.stringify({ isESM, isWithRequire, isWithBanner })}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(1 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(1 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
🧠 Learnings (17)
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to test-storybooks/** : Maintain test configurations and assets under test-storybooks/ for Storybook testing
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the spy: true option
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without vi.mocked()
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to type and access mocked functions
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with vi.mocked()
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should be placed in beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
🧬 Code graph analysis (2)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (1)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
fixFauxEsmRequire(17-71)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (4)
containsESMUsage(233-248)containsRequireUsage(251-260)hasRequireBanner(263-267)getRequireBanner(270-280)code/lib/cli-storybook/src/automigrate/types.ts (1)
Fix(52-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (1)
13-22: Add spy: true to vi.mock() per our Vitest policy.Ensures spies behave correctly with vi.mocked().
Apply:
-vi.mock('node:fs/promises', async (importOriginal) => ({ - ...(await importOriginal<typeof import('node:fs/promises')>()), - readFile: vi.fn(), - writeFile: vi.fn(), -})); +vi.mock( + 'node:fs/promises', + async (importOriginal) => ({ + ...(await importOriginal<typeof import('node:fs/promises')>()), + readFile: vi.fn(), + writeFile: vi.fn(), + }), + { spy: true } +); -vi.mock('storybook/internal/csf-tools', async (importOriginal) => ({ - ...(await importOriginal<typeof import('storybook/internal/csf-tools')>()), - readConfig: vi.fn(), -})); +vi.mock( + 'storybook/internal/csf-tools', + async (importOriginal) => ({ + ...(await importOriginal<typeof import('storybook/internal/csf-tools')>()), + readConfig: vi.fn(), + }), + { spy: true } +);code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
262-267: Detect both banner phrasings to avoid duplicate header insertions.Currently only the legacy phrasing is recognized; the fixer writes a different text.
Apply:
export function hasRequireBanner(content: string): boolean { - const comment = - '// end of storybook 10 migration assistant header, you can delete the above code'; - return content.includes(comment); + const legacyComment = + '// end of storybook 10 migration assistant header, you can delete the above code'; + const newComment = + '// end of storybook 10 migration assistant header, delete this if you are not using require'; + return content.includes(legacyComment) || content.includes(newComment); }code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
64-66: Align banner marker with detection logic to prevent duplicates.Use the same text as hasRequireBanner’s legacy phrase.
Apply:
- const comment = - '// end of storybook 10 migration assistant header, delete this if you are not using require'; + const comment = + '// end of storybook 10 migration assistant header, you can delete the above code';
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
Outdated
Show resolved
Hide resolved
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
Outdated
Show resolved
Hide resolved
- Introduced `bannerComment` constant in `mainConfigFile` to standardize the migration assistant header. - Updated `fix-faux-esm-require` utility to utilize the new `bannerComment` for consistency. - Simplified test cases to reflect changes in banner handling.
…ithub.com/storybookjs/storybook into norbert/sb10-upgrade-with-require-in-main
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (2)
232-248: Add extension-based ESM detection and optional path param.Short-circuit on .mjs/.mts/.cjs/.cts and keep content heuristics as fallback. This avoids false negatives/positives and lets callers pass the path when available.
-/** Check if a file is in ESM format based on its content */ -export function containsESMUsage(content: string): boolean { +/** Check if a file is in ESM format based on its path and/or content */ +export function containsESMUsage(pathOrContent: string, maybeContent?: string): boolean { + const content = maybeContent ?? pathOrContent; + const filePath = maybeContent ? pathOrContent : undefined; + // Prefer decisive detection by extension when path is known + if (filePath) { + const lower = filePath.toLowerCase(); + if (lower.endsWith('.mjs') || lower.endsWith('.mts')) return true; + if (lower.endsWith('.cjs') || lower.endsWith('.cts')) return false; + } // For .js/.ts files, check the content for ESM syntax // Check for ESM syntax indicators (multiline aware) const hasImportStatement = /^\s*import\s+/m.test(content) || /^\s*import\s*{/m.test(content) || /^\s*import\s*\(/m.test(content); const hasExportStatement = /^\s*export\s+/m.test(content) || /^\s*export\s*{/m.test(content) || /^\s*export\s*default/m.test(content); const hasImportMeta = /import\.meta/.test(content); // If any ESM syntax is found, it's likely an ESM file return hasImportStatement || hasExportStatement || hasImportMeta; }
262-267: Detect both legacy and alternate banner phrasings.Prevents duplicate headers if an older/newer banner variant already exists.
/** Check if the file already has the require banner */ export const bannerComment = '// end of storybook 10 migration assistant header, you can delete the above code'; export function hasRequireBanner(content: string): boolean { - return content.includes(bannerComment); + const alt = + '// end of storybook 10 migration assistant header, delete this if you are not using require'; + return content.includes(bannerComment) || content.includes(alt); }code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
22-52: Leverage path-aware ESM detection and return isConfigTypescript.Also passes path to containsESMUsage after helper update.
async check({ storybookVersion, mainConfigPath }) { if (!mainConfigPath) { return null; } // Read the raw file content to check for ESM syntax and require usage const content = await readFile(mainConfigPath, 'utf-8'); - const isESM = containsESMUsage(content); + const isESM = containsESMUsage(mainConfigPath, content); const isWithRequire = containsRequireUsage(content); const isWithBanner = hasRequireBanner(content); - logger.debug({ isESM, isWithRequire, isWithBanner }); + logger.debug({ isESM, isWithRequire, isWithBanner }); // Check if the file is ESM format based on content if (!isESM) { return null; } // Check if the file already has the require banner if (isWithBanner) { return null; } // Check if the file contains require usage if (!isWithRequire) { return null; } - return { storybookVersion }; + const isConfigTypescript = /\.(ts|mts|cts)$/.test(mainConfigPath); + return { storybookVersion, isConfigTypescript }; },
63-70: Defensive write: skip if banner appeared between check and run.Avoids double-prepend on concurrent edits.
const content = await readFile(mainConfigPath, 'utf-8'); + if (hasRequireBanner(content)) { + return; + } const banner = getRequireBanner(); const comment = bannerComment; const newContent = [banner, comment, content].join('\n\n'); await writeFile(mainConfigPath, newContent);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(1 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(1 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
🧠 Learnings (3)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to test-storybooks/** : Maintain test configurations and assets under test-storybooks/ for Storybook testing
Applied to files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to code/**/playwright.config.@(js|ts|mjs|cjs) : Use Playwright 1.52.0 for E2E tests and keep Playwright configuration aligned
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
🧬 Code graph analysis (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
scripts/utils/tools.ts (1)
dedent(118-118)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (5)
containsESMUsage(233-248)containsRequireUsage(251-260)hasRequireBanner(265-267)getRequireBanner(270-280)bannerComment(263-264)code/lib/cli-storybook/src/automigrate/types.ts (1)
Fix(52-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
269-280: LGTM: banner content is correct and minimal.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
54-56: Prompt copy reads well.
14-16: No need to expose isConfigTypescript in FixFauxEsmRequireRunOptions
FixFauxEsmRequire.check/run neither reads nor acts on TS format, so adding isConfigTypescript has no effect.Likely an incorrect or invalid review comment.
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
Outdated
Show resolved
Hide resolved
- Updated `containsRequireUsage` function to check for both `require()` calls and `require.` usage directly in the content, eliminating the need to remove comments. - Simplified the logic for detecting require usage to improve accuracy.
- Eliminated console.log statement that logged ESM and require detection results, streamlining the function's output for cleaner execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (4)
1-4: Use Storybook logger for diagnostics.We'll log read failures in check(); import the logger.
import { readFile, writeFile } from 'node:fs/promises'; import { dedent } from 'ts-dedent'; +import { logger } from 'storybook/internal/node-logger';
27-33: Harden check() against FS errors (avoid crashing the automigration).Gracefully handle unreadable config files and emit a debug line.
- // Read the raw file content to check for ESM syntax and require usage - const content = await readFile(mainConfigPath, 'utf-8'); + // Read the raw file content to check for ESM syntax and require usage + let content: string; + try { + content = await readFile(mainConfigPath, 'utf-8'); + } catch (err) { + logger.debug({ mainConfigPath, err }, 'fix-faux-esm-require: unable to read main config'); + return null; + }
61-63: Make run() idempotent (re-check banner before writing).Prevents duplicate headers on repeated runs/TOCTOU scenarios.
const content = await readFile(mainConfigPath, 'utf-8'); + if (hasRequireBanner(content)) { + return; + } const banner = getRequireBanner(); const comment = bannerComment;
65-67: Preserve original EOL style when rewriting the file.Reduces noisy diffs on Windows checkouts.
- const newContent = [banner, comment, content].join('\n\n'); + const eol = content.includes('\r\n') ? '\r\n' : '\n'; + const newContent = [banner, comment, content].join(`${eol}${eol}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
🧠 Learnings (2)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to code/**/playwright.config.@(js|ts|mjs|cjs) : Use Playwright 1.52.0 for E2E tests and keep Playwright configuration aligned
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
🧬 Code graph analysis (1)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (5)
containsESMUsage(233-248)containsRequireUsage(251-256)hasRequireBanner(261-263)getRequireBanner(266-276)bannerComment(259-260)code/lib/cli-storybook/src/automigrate/types.ts (1)
Fix(52-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
14-16: Include isConfigTypescript in the result and options; compute it from the path.Tests and callers expect this flag.
export interface FixFauxEsmRequireRunOptions { storybookVersion: string; + isConfigTypescript: boolean; }- return { storybookVersion }; + const isConfigTypescript = /\.(ts|mts|cts)$/.test(mainConfigPath); + return { storybookVersion, isConfigTypescript };Also applies to: 49-50
20-20: Docs link verified
Anchor exists and is correct; no update required.
ndelangen
requested review from
shilman,
yannbf,
valentinpalkovic and
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with Storybook 10 migration when main configuration files contain require statements in faux-ESM format by adding a temporary compatibility workaround and an auto-migration fix.
- Adds a temporary compatibility flow that retries loading main configuration files with a compatibility banner when
requireis not defined - Introduces an auto-migration (
fix-faux-esm-require) that detects ESM files usingrequireand adds a compatibility banner - Provides utility functions to detect ESM usage, require usage, and presence of compatibility banners
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts |
Adds utility functions for detecting ESM usage, require usage, and compatibility banners |
code/lib/cli-storybook/src/automigrate/fixes/index.ts |
Registers the new fix-faux-esm-require auto-migration |
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts |
Implements the auto-migration to add compatibility banners to faux-ESM files |
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts |
Comprehensive test coverage for the new auto-migration |
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.test.ts |
Removes unused jscodeshift import |
code/core/src/common/utils/load-main-config.ts |
Adds temporary compatibility flow to retry loading main config with require banner when needed |
| const hasImportStatement = | ||
| /^\s*import\s+/m.test(content) || | ||
| /^\s*import\s*{/m.test(content) || | ||
| /^\s*import\s*\(/m.test(content); | ||
| const hasExportStatement = | ||
| /^\s*export\s+/m.test(content) || | ||
| /^\s*export\s*{/m.test(content) || | ||
| /^\s*export\s*default/m.test(content); | ||
| const hasImportMeta = /import\.meta/.test(content); | ||
|
|
||
| // If any ESM syntax is found, it's likely an ESM file | ||
| return hasImportStatement || hasExportStatement || hasImportMeta; |
There was a problem hiding this comment.
[nitpick] The regex patterns for detecting ESM syntax could be simplified and made more robust. Consider combining them into a single regex with alternation: /^\s*(?:import(?:\s+|\s*[{(])|export(?:\s+|\s*[{]|\s*default))/m to reduce code duplication and improve maintainability.
| const hasImportStatement = | |
| /^\s*import\s+/m.test(content) || | |
| /^\s*import\s*{/m.test(content) || | |
| /^\s*import\s*\(/m.test(content); | |
| const hasExportStatement = | |
| /^\s*export\s+/m.test(content) || | |
| /^\s*export\s*{/m.test(content) || | |
| /^\s*export\s*default/m.test(content); | |
| const hasImportMeta = /import\.meta/.test(content); | |
| // If any ESM syntax is found, it's likely an ESM file | |
| return hasImportStatement || hasExportStatement || hasImportMeta; | |
| const esmRegex = /^\s*(?:import(?:\s+|\s*[{(])|export(?:\s+|\s*[{]|\s*default))/m; | |
| const hasESMSyntax = esmRegex.test(content); | |
| const hasImportMeta = /import\.meta/.test(content); | |
| // If any ESM syntax is found, it's likely an ESM file | |
| return hasESMSyntax || hasImportMeta; |
Copilot uses AI. Check for mistakes.
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
Outdated
Show resolved
Hide resolved
| } | ||
| if (e.message.includes('require is not defined')) { | ||
| logger.info( | ||
| 'loading main config failed, trying a temporary fix, please ensure the main config is valid ESM' |
There was a problem hiding this comment.
| 'loading main config failed, trying a temporary fix, please ensure the main config is valid ESM' | |
| 'Loading main config failed, trying a temporary fix. Please ensure the main config is valid ESM' |
| 'loading main config failed, trying a temporary fix, please ensure the main config is valid ESM' | ||
| ); | ||
| const comment = | ||
| '// end of storybook 10 migration assistant header, you can delete the above code'; |
There was a problem hiding this comment.
| '// end of storybook 10 migration assistant header, you can delete the above code'; | |
| '// end of Storybook 10 migration assistant header, you can delete the above code'; |
|
|
||
| /** Check if the file already has the require banner */ | ||
| export const bannerComment = | ||
| '// end of storybook 10 migration assistant header, you can delete the above code'; |
There was a problem hiding this comment.
| '// end of storybook 10 migration assistant header, you can delete the above code'; | |
| '// end of Storybook 10 migration assistant header. You can delete the above code'; |
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
Outdated
Show resolved
Hide resolved
| return null; | ||
| } | ||
|
|
||
| return { storybookVersion }; |
There was a problem hiding this comment.
Returning true is enough, since storybookVersion isn't really used in the run function.
Co-authored-by: Valentin Palkovic <[email protected]>
Co-authored-by: Copilot <[email protected]>
…ant header - Improved the log message in `loadMainConfig` for clarity and consistency. - Standardized the casing in the migration assistant header comment in `mainConfigFile` for better readability.
…ithub.com/storybookjs/storybook into norbert/sb10-upgrade-with-require-in-main
- Removed the storybookVersion parameter from the check method, returning true instead of the version. - Updated type satisfaction to use Fix<true> for improved clarity and consistency.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
code/core/src/common/utils/load-main-config.ts (2)
32-35: Harden error check and refine log message.Use a regex and case-insensitive match; tighten the log copy.
- if (e.message.includes('require is not defined')) { - logger.info( - 'Loading main config failed, trying a temporary fix, Please ensure the main config is valid ESM' - ); + if (e.message && /\brequire is not defined\b/i.test(e.message)) { + logger.info('Loading main config failed. Applying a temporary compatibility banner. Ensure the main config is valid ESM.');
36-38: Align banner sentinel text with automigration helpers to avoid mismatches.The helpers use “You can delete…”, while this uses “you can…”. Make them identical to prevent false negatives.
- const comment = - '// end of Storybook 10 migration assistant header, you can delete the above code'; + const comment = + '// end of Storybook 10 migration assistant header, You can delete the above code';code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
232-248: Optional: simplify and strengthen ESM detection.Consolidate regex and keep import.meta check; keeps behavior while reducing duplication.
- const hasImportStatement = - /^\s*import\s+/m.test(content) || - /^\s*import\s*{/m.test(content) || - /^\s*import\s*\(/m.test(content); - const hasExportStatement = - /^\s*export\s+/m.test(content) || - /^\s*export\s*{/m.test(content) || - /^\s*export\s*default/m.test(content); - const hasImportMeta = /import\.meta/.test(content); - - // If any ESM syntax is found, it's likely an ESM file - return hasImportStatement || hasExportStatement || hasImportMeta; + const hasESMSyntax = /^\s*(?:import(?:\s+|\s*[{(])|export(?:\s+|\s*[{]|\s*default))/m.test( + content + ); + const hasImportMeta = /import\.meta/.test(content); + return hasESMSyntax || hasImportMeta;
250-256: Reduce false positives; detect require property usages.Strip comments/strings and detect both require(…) and require.*.
-export function containsRequireUsage(content: string): boolean { - // Check for require() calls - const requireCallRegex = /\brequire\(/; - const requireDotRegex = /\brequire\./; - return requireCallRegex.test(content) || requireDotRegex.test(content); -} +export function containsRequireUsage(content: string): boolean { + const noComments = content + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/.*$/gm, ''); + const noStrings = noComments.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, ''); + return /\brequire\s*(?:\(|\.)/.test(noStrings); +}
258-263: Make banner detection resilient to minor text variations (case/wording).Use a case-insensitive regex to avoid mismatch with loader’s sentinel.
-export const bannerComment = - '// end of Storybook 10 migration assistant header, You can delete the above code'; -export function hasRequireBanner(content: string): boolean { - return content.includes(bannerComment); -} +export const bannerComment = + '// end of Storybook 10 migration assistant header, You can delete the above code'; +export function hasRequireBanner(content: string): boolean { + return /\/\/\s*end of storybook 10 migration assistant header,\s*you can delete the above code/i.test( + content + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/common/utils/load-main-config.ts(2 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(1 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/common/utils/load-main-config.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/common/utils/load-main-config.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
🧠 Learnings (3)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/core/src/common/utils/load-main-config.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to test-storybooks/** : Maintain test configurations and assets under test-storybooks/ for Storybook testing
Applied to files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to code/**/playwright.config.@(js|ts|mjs|cjs) : Use Playwright 1.52.0 for E2E tests and keep Playwright configuration aligned
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts
🧬 Code graph analysis (3)
code/core/src/common/utils/load-main-config.ts (2)
scripts/utils/tools.ts (1)
dedent(118-118)code/core/src/shared/utils/module.ts (1)
importModule(54-81)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
scripts/utils/tools.ts (1)
dedent(118-118)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (5)
containsESMUsage(233-248)containsRequireUsage(251-256)hasRequireBanner(261-263)getRequireBanner(266-276)bannerComment(259-260)code/lib/cli-storybook/src/automigrate/types.ts (1)
Fix(52-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
265-276: Ensure ts-dedent is imported as a default in this file.getRequireBanner uses dedent; confirm the file imports the default export.
Apply at the top of this file (outside the changed hunk):
import dedent from 'ts-dedent';Based on learnings
code/core/src/common/utils/load-main-config.ts (3)
8-8: Fix ts-dedent import (default export, not named).Using a named import will be undefined at runtime and fail type checks in strict setups.
Apply:
-import { dedent } from 'ts-dedent'; +import dedent from 'ts-dedent';Based on learnings
54-56: Ensure temp file is cleaned up even if import fails.Use try/finally around import; avoid leaking temporary files.
- const out = await importModule(modifiedMainPath); - await rm(modifiedMainPath); - return out; + let out; + try { + out = await importModule(modifiedMainPath); + } finally { + try { + await rm(modifiedMainPath); + } catch {} + } + return out;
51-53: Fix temp filename (extra dot before ext) and prefer uniqueness.
${ext}already includes a leading dot. Also reduce collision risk across processes.- const { ext, name, dir } = parse(mainPath); - const modifiedMainPath = join(dir, `${name}.tmp.${ext}`); + const { ext, name, dir } = parse(mainPath); + const modifiedMainPath = join(dir, `${name}.tmp.${Date.now()}.${process.pid}${ext}`);
- Removed unnecessary mock parameters from the check method tests. - Changed assertions to return true instead of the storybookVersion for ESM with require usage detection. - Simplified test cases for clarity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
code/core/src/common/utils/load-main-config.ts (4)
32-36: Harden error detection for "require is not defined".Use a regex and guard against non-string messages to avoid false negatives.
Apply:
- if (e.message.includes('require is not defined')) { + if (typeof e.message === 'string' && /\brequire is not defined\b/.test(e.message)) { logger.info( 'Loading main config failed, trying a temporary fix, Please ensure the main config is valid ESM' );
38-39: Use canonical utf8 encoding token.Prefer 'utf8' for consistency with Node conventions.
- const content = await readFile(mainPath, 'utf-8'); + const content = await readFile(mainPath, 'utf8');
54-60: Don’t let cleanup override the import error; tighten types.If rm throws in finally, it can mask the real failure. Also avoid an implicit any for out. Return directly with typed value and ignore rm errors.
As per coding guidelines
- let out; - try { - out = await importModule(modifiedMainPath); - } finally { - await rm(modifiedMainPath); - } - return out; + try { + const out = (await importModule(modifiedMainPath)) as StorybookConfig; + return out; + } finally { + try { + await rm(modifiedMainPath); + } catch { + // ignore cleanup errors + } + }I can also wrap the temp import in a MainFileEvaluationError for consistent UX if you want.
36-41: Idempotency check is OK, consider also detecting prior shims.You gate on the sentinel comment only. Optionally also skip if content includes createRequire(import.meta.url) to reduce false positives.
- if (!content.includes(comment)) { + if (!content.includes(comment) && !content.includes('createRequire(import.meta.url)')) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/common/utils/load-main-config.ts(2 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/common/utils/load-main-config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/common/utils/load-main-config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to **/*.{ts,tsx} : Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Applied to files:
code/core/src/common/utils/load-main-config.ts
🧬 Code graph analysis (1)
code/core/src/common/utils/load-main-config.ts (1)
code/core/src/shared/utils/module.ts (1)
importModule(54-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/common/utils/load-main-config.ts (5)
1-2: LGTM on new fs/path imports.Usage aligns with Node ESM and project style.
4-4: LGTM on logger usage.Good diagnostic message path.
8-8: Verify ts-dedent import style (named vs default).ts-dedent’s export shape varies by module resolution; ensure the named import works with our bundling/ESM setup. If not, switch to default import.
Given conflicting guidance in prior comments, please confirm with a quick check in our environment. If needed, apply:
-import { dedent } from 'ts-dedent'; +import dedent from 'ts-dedent';Based on learnings
41-49: LGTM on injected banner content.Header provides __filename, __dirname, and require via createRequire as intended.
51-53: Fix temp filename (double dot) and add uniqueness to avoid collisions.parse().ext includes a leading dot; current interpolation yields "main.tmp..ts". Also, make the temp name unique.
- const { ext, name, dir } = parse(mainPath); - const modifiedMainPath = join(dir, `${name}.tmp.${ext}`); + const { ext, name, dir } = parse(mainPath); + const suffix = `.tmp.${Date.now()}.${process.pid}`; + const modifiedMainPath = join(dir, `${name}${suffix}${ext}`);
What I did
This ensures 2 things:
The work around is: we create a temp copy of the main config file, prepend a compat-banner, load and then delete it.
requireChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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.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 pull request has been released as version
0.0.0-pr-32558-sha-aadcab7f. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-32558-sha-aadcab7fnorbert/sb10-upgrade-with-require-in-mainaadcab7f1758808072)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=32558Summary by CodeRabbit
New Features
Bug Fixes
requireis not available while loading the main config.Tests