Upgrade: Enhance ESM compatibility checks and banner generation#32694
Conversation
- Updated `fix-faux-esm-require` to analyze compatibility needs for ESM files, including checks for `require` and `__dirname` usage. - Added new tests to validate the detection of `__dirname` and `require` usage, as well as the generation of appropriate compatibility banners. - Refactored the `getRequireBanner` function to conditionally include necessary imports and constants based on the analyzed compatibility needs. - Improved overall test coverage for compatibility analysis and banner generation.
ndelangen
added
bug
automigrations
ci:normal
📝 WalkthroughWalkthroughReturn structured BannerConfig (flags for require, __filename, __dirname) from detection, add pattern-based utilities for dirname/filename/definition detection, and update the faux-ESM fix to conditionally insert imports/shims and banner text; tests updated to assert BannerConfig and transformed content. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Fix as fix-faux-esm-require
participant H as mainConfigFile helpers
participant FS as FileSystem
User->>Fix: check(mainConfigContent)
Fix->>H: containsPatternUsage / containsDirnameUsage / containsFilenameUsage / hasDirnameDefined
Fix->>H: hasRequireBanner
Fix-->>User: BannerConfig | null
User->>Fix: prompt(BannerConfig)
User->>Fix: run({BannerConfig, dryRun})
alt changes needed
Fix->>FS: read main config
Note over Fix,FS: Insert imports after last import (if missing)
alt needs __filename
Fix->>Fix: inject fileURLToPath + URL -> __filename
end
alt needs __dirname
Fix->>Fix: inject dirname from __filename (if not defined)
end
alt needs require
Fix->>Fix: inject createRequire-based require binding
end
Fix->>FS: write updated file (skip if dryRun)
Fix-->>User: updated result (BannerConfig)
else no changes
Note right of Fix: No write (or dryRun)
Fix-->>User: null / unchanged
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
⏰ 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)
🔇 Additional comments (1)
Comment |
|
View your CI Pipeline Execution ↗ for commit 337a1c0
☁️ Nx Cloud last updated this comment at |
- Replaced `analyzeCompatibilityNeeds` with direct checks for `require`, `__dirname`, and `__filename` usage in `fix-faux-esm-require`. - Updated tests to reflect changes in compatibility checks and ensure accurate detection of ESM features. - Simplified banner generation logic by removing unnecessary functions and consolidating import handling. - Enhanced overall test coverage for the updated compatibility analysis and migration process.
- Enhanced the `fix-faux-esm-require` functionality by refining the logic for inserting `__filename`, `__dirname`, and `require` declarations after import statements. - Updated test cases to ensure accurate validation of the new insertion behavior and compatibility checks. - Improved code readability and maintainability by restructuring variable declarations and insertion indices.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
223-254: Add tests for alias/type imports and additional string/comment edge cases
- containsDirnameUsage: add cases for single quotes, template literals, and lines with '//' inside strings to avoid false positives/negatives.
- hasDirnameDefined: add cases ensuring definitions in comments/strings are ignored.
- hasImport: add cases for aliased named imports (import { dirname as dn }), and type-only imports (import type { dirname } ...) if you intend to detect them.
I can draft the additional tests if helpful.
Also applies to: 257-289, 291-322
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (2)
8-12: Adopt spy mode for module mocks per guidelinesUse vi.mock(..., { spy: true }) and avoid manually replacing exports with vi.fn(). Then stub via vi.mocked(readFile).mockResolvedValue(...).
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')>(), + { spy: true } +);As per coding guidelines
79-99: Consider test for already-defined __dirname/__filenameAdd a check() scenario where __dirname/__filename are already declared to ensure result flags do not trigger duplicate declarations.
Also applies to: 101-121, 138-143
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
301-306: Consider adding filename counterparts to avoid duplication in fixerAdd hasFilenameDefined and, if needed, containsFilenameUsage mirroring the dirname helpers to support correct detection downstream.
Additional (outside this hunk):
export function hasFilenameDefined(content: string): boolean { return /(?:const|let|var)\s+__filename\s*=/.test(content); } export function containsFilenameUsage(content: string): boolean { // replicate improved sanitization from containsDirnameUsage and test for \b__filename\b // or factor out a generic strip + identifier check const stripStrings = (s: string) => s.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, '""'); const withoutStrings = stripStrings(content); const withoutBlock = withoutStrings.replace(/\/\*[\s\S]*?\*\//g, ''); const clean = withoutBlock.split('\n').map((l) => l.split('//')[0]).join('\n'); return /\b__filename\b/.test(clean); }code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
62-63: Prompt should mention __filename as wellInclude '__filename' to match behavior.
- return dedent`Main config is ESM but uses 'require' or '__dirname'. This will break in Storybook 10; Adding compatibility banner`; + return dedent`Main config is ESM but uses 'require', '__dirname' or '__filename'. This will break in Storybook 10; adding compatibility banner`;
47-48: Remove unused variablehasBanner is computed but unused.
- const hasBanner = !!content.includes(bannerComment);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(5 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
🧰 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/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.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/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.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.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.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.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts
🧬 Code graph analysis (3)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (4)
containsRequireUsage(251-256)bannerComment(308-309)updateMainConfig(207-230)BannerConfig(302-306)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 (1)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
fixFauxEsmRequire(17-127)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
containsDirnameUsage(259-277)hasDirnameDefined(280-284)hasImport(287-299)
🪛 ast-grep (0.39.6)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
[warning] 290-290: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(import\\s*{\\s*[^}]*\\b${importName}\\b[^}]*}\\s*from\\s*["']${fromModule}["'])
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 292-292: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(import\\s+${importName}\\s+from\\s*["']${fromModule}["'])
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 294-294: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(import\\s*\\*\\s*as\\s+${importName}\\s+from\\s*["']${fromModule}["'])
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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
- Updated the `fix-faux-esm-require` implementation to utilize `import.meta.url` directly instead of using string literals for `__filename` and `require` declarations. - Adjusted test cases to reflect the changes in the handling of ESM imports and ensure accurate validation of the new logic.
- Added new tests to validate detection of `__dirname` in various contexts, including strings and comments. - Refactored `containsDirnameUsage` to utilize a new `containsPatternUsage` function for improved pattern matching. - Updated `fix-faux-esm-require` to leverage the new `containsDirnameUsage` and `containsFilenameUsage` functions for better compatibility checks.
- Deleted the `hasImport` function from `mainConfigFile.ts` to streamline import detection logic. - Removed corresponding tests from `mainConfigFile.test.ts` to reflect the function's removal and maintain test accuracy.
- Added checks to ensure that `__filename`, `__dirname`, and `require` declarations are not duplicated when they already exist in the code. - Introduced new tests to validate the prevention of duplicate declarations in various scenarios. - Improved the logic for inserting declarations after import statements, enhancing overall compatibility and correctness.
- Modified the `fixFauxEsmRequire` logic to only add imports for `createRequire`, `dirname`, and `fileURLToPath` based on their actual usage in the code. - Updated tests to reflect the changes in import handling and ensure accurate validation of the new conditional logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (3)
8-16: ImporthasDirnameDefinedto filter definitions in check().Add it to the helper imports:
import { type BannerConfig, bannerComment, - containsDirnameUsage, + containsDirnameUsage, containsESMUsage, containsFilenameUsage, containsRequireUsage, hasRequireBanner, + hasDirnameDefined, updateMainConfig, } from '../helpers/mainConfigFile';
92-107: Avoid flagging already-defined identifiers incheck().Filter out definition sites to reduce false positives and unused imports:
- const hasRequireUsage = containsRequireUsage(content); - const hasUnderscoreFilename = containsFilenameUsage(content); - const hasUnderscoreDirname = containsDirnameUsage(content); + const hasRequireUsage = containsRequireUsage(content); + const hasUnderscoreFilename = + containsFilenameUsage(content) && !/(?:const|let|var)\s+__filename\s*=/.test(content); + const hasUnderscoreDirname = + containsDirnameUsage(content) && !hasDirnameDefined(content);
118-131: LGTM: Only add imports that are actually used.
🧹 Nitpick comments (10)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (2)
282-287: Broaden detection to cover plain assignments.Currently only matches
const|let|var. Consider also__dirname = ...to avoid re-defining when previously assigned.Example:
-const dirnameDefinedRegex = /(?:const|let|var)\s+__dirname\s*=/; +const dirnameDefinedRegex = /(?:(?:const|let|var)\s+)?__dirname\s*=/;
298-303: Banner detection could be stricter.
includes()may match mid-file. If intent is “banner at top”, prefer:-export const hasRequireBanner = (content: string): boolean => { - return content.includes(bannerComment); -}; +export const hasRequireBanner = (content: string): boolean => { + return content.trimStart().startsWith(bannerComment); +};code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (3)
8-12: Usespy: trueon module mock per test guidelines.Enable spy behavior for mocked module:
-vi.mock('node:fs/promises', async (importOriginal) => ({ +vi.mock('node:fs/promises', async (importOriginal) => ({ ...(await importOriginal<typeof import('node:fs/promises')>()), readFile: vi.fn(), writeFile: vi.fn(), -})); +}), { spy: true });As per coding guidelines
231-258: Also assert banner prepend (second write).The first write is the AST rewrite; the second write prepends the banner. Add:
expect(mockWriteFile).toHaveBeenCalledWith( 'main.js', expect.stringContaining('import { createRequire } from "node:module"') ); + // Banner prepend on second write + const finalWritten = mockWriteFile.mock.calls[1][1]; + expect(finalWritten.startsWith(bannerComment)).toBe(true);
146-229: Add edge-case tests: regex literals and template interpolations.
- Ensure
containsDirnameUsageignores//inside regex literals.- Ensure
${__dirname}inside template literals is detected.I can add test cases for both under this block if you want.
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (3)
223-264: Good coverage; consider adding regex literal case.Add a case where
//appears inside a regex literal (e.g.,/https?:\/\/x/) to ensure it isn’t treated as a comment.
266-298: Add template interpolation coverage.Test
${__dirname}inside a template literal to ensure detection works (or define expected behavior explicitly).
300-332: LGTM: hasDirnameDefined tests for const/let/var are solid.Consider adding a test for plain assignment
__dirname = ...if you expand detection.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
110-110: Prompt: mention__filenametoo.- return dedent`Main config is ESM but uses 'require' or '__dirname'. This will break in Storybook 10; Adding compatibility banner`; + return dedent`Main config is ESM but uses 'require', '__dirname', or '__filename'. This will break in Storybook 10; adding compatibility banner.`;
147-186: Remove unusedinsertOffset.The local
insertOffsetis not used after being incremented. Safe to remove.- const declarationsToInsert: t.Statement[] = []; - let insertOffset = 0; + const declarationsToInsert: t.Statement[] = []; ... - insertOffset++; ... - insertOffset++;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(6 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/helpers/mainConfigFile.test.tscode/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/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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 (2)
📚 Learning: 2025-10-13T13:33:14.648Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.648Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
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
🧬 Code graph analysis (3)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
containsPatternUsage(259-271)containsDirnameUsage(274-276)hasDirnameDefined(283-287)
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(67-216)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
containsDirnameUsage(274-276)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (6)
containsRequireUsage(251-256)containsFilenameUsage(278-280)containsDirnameUsage(274-276)updateMainConfig(207-230)bannerComment(298-299)BannerConfig(292-296)
⏰ 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). (3)
- GitHub Check: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (9)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
273-276: LGTM: __dirname detection helper is clear.
278-280: LGTM: __filename detection helper mirrors dirname.
291-296: LGTM: BannerConfig shape is minimal and sufficient.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (4)
259-298: LGTM: Transform injects correct imports and uses unquoted import.meta.url.
300-341: LGTM: No duplicate imports; order adapts to existing ones.
357-398: LGTM: Skips duplicate __filename/__dirname declarations.
400-434: LGTM: Skips duplicate require declaration.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
188-209: LGTM: Correctimport.meta.urlAST usage and idempotent require insertion.
212-214: LGTM: Banner prepend after AST write is clear.Note:
hasRequireBanneralready guards reruns.
| /** Check if the file content contains a pattern matching the given regex */ | ||
| export function containsPatternUsage(content: string, pattern: RegExp): boolean { | ||
| // Remove strings first, then comments | ||
| const stripStrings = (s: string) => s.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, '""'); | ||
| const withoutStrings = stripStrings(content); | ||
| const withoutBlock = withoutStrings.replace(/\/\*[\s\S]*?\*\//g, ''); | ||
| const cleanContent = withoutBlock | ||
| .split('\n') | ||
| .map((line) => line.split('//')[0]) | ||
| .join('\n'); | ||
|
|
||
| // Check for pattern usage in the cleaned content | ||
| return pattern.test(cleanContent); | ||
| } |
There was a problem hiding this comment.
Heuristic misses template interpolations and regex literals (false negatives).
${__dirname}in template literals is stripped entirely, so usage is missed.//inside regex literals (e.g., /https?://.../) is treated as comments and code after is cut.
Prefer AST-based detection (babel parser) for identifiers, or at minimum:
- Preserve
${...}when stripping template literals. - Strip regex literals before splitting on
//.
Add tests for both cases. I can draft an AST-based helper if you want.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (3)
3-3: Good fix: import.meta.url now uses proper AST expressionPrevious string-literal bug is resolved by using MetaProperty + MemberExpression. LGTM.
92-106: Avoid false positives: ignore __dirname/__filename definitions in check()Without filtering, we may insert unused imports and trigger ESLint. Ignore definition sites.
- const hasRequireUsage = containsRequireUsage(content); - const hasUnderscoreFilename = containsFilenameUsage(content); - const hasUnderscoreDirname = containsDirnameUsage(content); + const hasRequireUsage = containsRequireUsage(content); + const hasUnderscoreDirname = + containsDirnameUsage(content) && !hasDirnameDefined(content); + const hasUnderscoreFilename = + containsFilenameUsage(content) && + !/(?:const|let|var)\s+__filename\s*=/.test(content);
7-16: Import missing helper to filter out definitions in check()Bring in hasDirnameDefined so we don’t flag files that already define
__dirname.Apply:
import { type BannerConfig, bannerComment, containsDirnameUsage, containsESMUsage, containsFilenameUsage, containsRequireUsage, hasRequireBanner, - updateMainConfig, + hasDirnameDefined, + updateMainConfig, } from '../helpers/mainConfigFile';
🧹 Nitpick comments (9)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (2)
223-264: Add a block-comment case to strengthen containsPatternUsage testsYou cover strings and single-line comments; add a multi-line comment case to ensure the sanitizer ignores
__dirnameinside/* ... */.
300-332: Test hasDirnameDefined against strings/comments to prevent false positiveshasDirnameDefined currently doesn’t strip strings/comments; add cases where
const __dirname = ...appears inside a string or comment and expect false.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (3)
147-186: Remove unused variable insertOffsetinsertOffset is incremented but never used. Drop it and the increments.
- const declarationsToInsert: t.Statement[] = []; - let insertOffset = 0; + const declarationsToInsert: t.Statement[] = []; @@ - insertOffset++; @@ - insertOffset++;
132-141: Build import.meta.url once and reuseAvoid duplicating the AST for
import.meta.url.const insertIndex = lastImportIndex + 1; + // import.meta.url expression + const importMetaUrl = t.memberExpression( + t.metaProperty(t.identifier('import'), t.identifier('meta')), + t.identifier('url') + );
155-166: Reuse importMetaUrl in declarationsSmall cleanup: use the shared expression for fileURLToPath/createRequire args.
- t.callExpression(t.identifier('fileURLToPath'), [ - t.memberExpression( - t.metaProperty(t.identifier('import'), t.identifier('meta')), - t.identifier('url') - ), - ]) + t.callExpression(t.identifier('fileURLToPath'), [importMetaUrl]) @@ - t.callExpression(t.identifier('createRequire'), [ - t.memberExpression( - t.metaProperty(t.identifier('import'), t.identifier('meta')), - t.identifier('url') - ), - ]) + t.callExpression(t.identifier('createRequire'), [importMetaUrl])Also applies to: 193-199
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (2)
8-12: Align Vitest mocks with repo guidelines (spy: true)Per guidelines, use
vi.mock(..., { spy: true })so original exports are spied and type-safe.As per coding guidelines
-vi.mock('node:fs/promises', async (importOriginal) => ({ +vi.mock('node:fs/promises', async (importOriginal) => ({ ...(await importOriginal<typeof import('node:fs/promises')>()), readFile: vi.fn(), writeFile: vi.fn(), -})); +}), { spy: true });
231-258: Also assert the banner is prependedEnsure the final write includes the banner at the top.
expect(mockWriteFile).toHaveBeenCalledWith( 'main.js', expect.stringContaining('import { createRequire } from "node:module"') ); + const lastCall = mockWriteFile.mock.calls.at(-1)!; + expect(lastCall[1].startsWith(bannerComment)).toBe(true);code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (2)
282-287: Sanitize before checking hasDirnameDefined to avoid false positivesCurrently matches inside strings/comments. Reuse containsPatternUsage.
export function hasDirnameDefined(content: string): boolean { - // Check if __dirname is already defined as a const/let/var - const dirnameDefinedRegex = /(?:const|let|var)\s+__dirname\s*=/; - return dirnameDefinedRegex.test(content); + // Check if __dirname is already defined as a const/let/var (ignoring strings/comments) + return containsPatternUsage(content, /(?:^|\s)(?:const|let|var)\s+__dirname\s*=/); }
301-303: Prefer start-of-file check for bannerReduce false positives by checking prefix rather than any occurrence.
-export const hasRequireBanner = (content: string): boolean => { - return content.includes(bannerComment); -}; +export const hasRequireBanner = (content: string): boolean => { + return content.trimStart().startsWith(bannerComment); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(6 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/helpers/mainConfigFile.test.tscode/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/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/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 (2)
📚 Learning: 2025-10-13T13:33:14.648Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.648Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
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
🧬 Code graph analysis (3)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
containsPatternUsage(259-271)containsDirnameUsage(274-276)hasDirnameDefined(283-287)
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(67-216)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
containsDirnameUsage(274-276)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (6)
containsRequireUsage(251-256)containsFilenameUsage(278-280)containsDirnameUsage(274-276)updateMainConfig(207-230)bannerComment(298-299)BannerConfig(292-296)
⏰ 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/helpers/mainConfigFile.ts (1)
258-271: Solid sanitizer for containsPatternUsageString-first stripping and block/single-line comment handling look good. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
92-106: Filter out definitions in check() to reduce false positivesAvoid flagging when
__dirname/__filenameare defined. This also prevents unused imports later.Apply this diff:
- // Analyze what compatibility features are needed - const hasRequireUsage = containsRequireUsage(content); - const hasUnderscoreFilename = containsFilenameUsage(content); - const hasUnderscoreDirname = containsDirnameUsage(content); + // Analyze what compatibility features are needed + const hasRequireUsage = containsRequireUsage(content); + const hasUnderscoreDirname = + containsDirnameUsage(content) && !hasDirnameDefined(content); + const hasUnderscoreFilename = + containsFilenameUsage(content) && !hasFilenameDefined(content); // Check if any compatibility features are needed if (hasRequireUsage || hasUnderscoreFilename || hasUnderscoreDirname) { return { hasRequireUsage, hasUnderscoreDirname, hasUnderscoreFilename, }; } return null;
🧹 Nitpick comments (5)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
300-332: Extend tests to cover exported definitions and false positivesAdd cases for:
export const/let/var __dirname = ...(should be detected)- String/comment embeddings of
const __dirname = ...(should not be detected once helper is updated)I can add tests and adjust the helper regex to match exported declarations.
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (1)
357-398: Avoid unused imports when declarations already existThis snapshot shows imports for
fileURLToPathanddirnameeven though__filename/__dirnameare pre-defined as literals. Recommend changingrun()to only add those imports if it will insert the corresponding declarations (not when they already exist), to prevent ESLint unused-imports.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
147-186: Use computed need flags and remove unused variableSimplify insertion logic with
needsFilenameDecl/needsDirnameDecland dropinsertOffset.Apply this diff:
- // Add __filename and __dirname if used and not already declared - if (hasUnderscoreFilename || hasUnderscoreDirname) { - const declarationsToInsert: t.Statement[] = []; - let insertOffset = 0; + // Add __filename and __dirname if used and not already declared + if (needsFilenameDecl || needsDirnameDecl) { + const declarationsToInsert: t.Statement[] = []; // Add __filename declaration if needed and not already exists // Note: __filename is always needed if __dirname is used, since __dirname depends on __filename - if ((hasUnderscoreFilename || hasUnderscoreDirname) && !hasExistingFilename) { + if (needsFilenameDecl) { const filenameDeclaration = t.variableDeclaration('const', [ t.variableDeclarator( t.identifier('__filename'), t.callExpression(t.identifier('fileURLToPath'), [ t.memberExpression( t.metaProperty(t.identifier('import'), t.identifier('meta')), t.identifier('url') ), ]) ), ]); declarationsToInsert.push(filenameDeclaration); - insertOffset++; } // Add __dirname declaration if needed and not already exists - if (hasUnderscoreDirname && !hasExistingDirname) { + if (needsDirnameDecl) { const dirnameDeclaration = t.variableDeclaration('const', [ t.variableDeclarator( t.identifier('__dirname'), t.callExpression(t.identifier('dirname'), [t.identifier('__filename')]) ), ]); declarationsToInsert.push(dirnameDeclaration); - insertOffset++; } // Insert declarations after imports declarationsToInsert.forEach((declaration, index) => { body.splice(insertIndex + index, 0, declaration); }); }
188-209: Compute insertion offset based on actual inserted declarationsUse the same
needs*Declflags fordeclarationsAdded.Apply this diff:
- // add require if used and not already declared + // add require if used and not already declared if (hasRequireUsage && !hasExistingRequire) { const requireDeclaration = t.variableDeclaration('const', [ t.variableDeclarator( t.identifier('require'), t.callExpression(t.identifier('createRequire'), [ t.memberExpression( t.metaProperty(t.identifier('import'), t.identifier('meta')), t.identifier('url') ), ]) ), ]); // Calculate insert position: after imports and after any __filename/__dirname declarations that were added - const filenameAdded = - (hasUnderscoreFilename || hasUnderscoreDirname) && !hasExistingFilename; - const dirnameAdded = hasUnderscoreDirname && !hasExistingDirname; - const declarationsAdded = (filenameAdded ? 1 : 0) + (dirnameAdded ? 1 : 0); + const declarationsAdded = (needsFilenameDecl ? 1 : 0) + (needsDirnameDecl ? 1 : 0); const currentInsertIndex = insertIndex + declarationsAdded; body.splice(currentInsertIndex, 0, requireDeclaration); }code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
250-256: Sanitize require() detection to avoid false positives
containsRequireUsageshould reuse the sanitizer to ignore strings/comments.Apply this diff:
export function containsRequireUsage(content: string): boolean { - // Check for require() calls - const requireCallRegex = /\brequire\(/; - const requireDotRegex = /\brequire\./; - return requireCallRegex.test(content) || requireDotRegex.test(content); + // Check for require() calls or property access, ignoring strings/comments + return containsPatternUsage(content, /(?:\brequire\s*\()|(?:\brequire\.)/); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(6 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.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/helpers/mainConfigFile.test.tscode/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/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts
🧠 Learnings (2)
📚 Learning: 2025-10-13T13:33:14.648Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.648Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
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
🧬 Code graph analysis (3)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (6)
containsRequireUsage(251-256)containsFilenameUsage(278-280)containsDirnameUsage(274-276)updateMainConfig(207-230)bannerComment(298-299)BannerConfig(292-296)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
containsPatternUsage(259-271)containsDirnameUsage(274-276)hasDirnameDefined(283-287)
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(67-216)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
containsDirnameUsage(274-276)
⏰ 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 (13)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (2)
223-264: Good coverage for pattern sanitizationThe
containsPatternUsagetests correctly validate ignoring strings and comments and detection on real code. Looks solid.
266-298: LGTM: __dirname detection scenarios coveredThe cases for strings (all quotes), single/multi-line comments, and tricky
//in strings are well covered.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (7)
79-99: check() returning BannerConfig: OKThe test correctly asserts the BannerConfig shape for ESM+require usage.
101-121: Detect __dirname without definition: OKThe BannerConfig expectation for bare
__dirname(no definition) looks correct.
138-143: TS config detection with BannerConfig: OKAsserting the same BannerConfig shape for
.tsfiles is appropriate.
146-229: Dirname detection tests duplicated here: OKCoverage here mirrors helper tests; fine for integration context.
246-251: Also assert the banner is writtenAfter run(), please assert
writtenContentcontainsbannerCommentto verify the banner prepend behavior.
259-298: Snapshot likely missing banner line
run()prependsbannerComment, but the inline snapshot starts with imports. Update the snapshot to include the banner comment as the first line (or add a separate assertion for it).
400-434: Require declaration dedupe: OKThe test correctly ensures we don’t re-declare
require.code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (4)
258-271: String/comment stripping helper: LGTMThe generalized sanitizer is robust and handles ', ", and ` with escapes. Good foundation.
273-276: __dirname usage detection: LGTMDelegating to
containsPatternUsageis correct and avoids string/comment false positives.
278-280: __filename usage detection: LGTMSymmetric with dirname; looks good.
298-303: Banner presence detection: OKSimple include check is fine for a synthetic comment banner.
| import { | ||
| type BannerConfig, | ||
| bannerComment, | ||
| containsDirnameUsage, | ||
| containsESMUsage, | ||
| containsFilenameUsage, | ||
| containsRequireUsage, | ||
| getRequireBanner, | ||
| hasRequireBanner, | ||
| updateMainConfig, | ||
| } from '../helpers/mainConfigFile'; |
There was a problem hiding this comment.
Import missing helpers to filter definitions
Bring in hasDirnameDefined and a new hasFilenameDefined helper to avoid false positives in check().
Apply this diff:
import {
type BannerConfig,
bannerComment,
- containsDirnameUsage,
+ containsDirnameUsage,
containsESMUsage,
- containsFilenameUsage,
+ containsFilenameUsage,
containsRequireUsage,
hasRequireBanner,
- updateMainConfig,
+ updateMainConfig,
+ hasDirnameDefined,
+ hasFilenameDefined,
} from '../helpers/mainConfigFile';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { | |
| type BannerConfig, | |
| bannerComment, | |
| containsDirnameUsage, | |
| containsESMUsage, | |
| containsFilenameUsage, | |
| containsRequireUsage, | |
| getRequireBanner, | |
| hasRequireBanner, | |
| updateMainConfig, | |
| } from '../helpers/mainConfigFile'; | |
| import { | |
| type BannerConfig, | |
| bannerComment, | |
| containsDirnameUsage, | |
| containsESMUsage, | |
| containsFilenameUsage, | |
| containsRequireUsage, | |
| hasRequireBanner, | |
| updateMainConfig, | |
| hasDirnameDefined, | |
| hasFilenameDefined, | |
| } from '../helpers/mainConfigFile'; |
🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts around
lines 7 to 16, the check() logic can report false positives because it doesn't
import helpers that detect if __dirname or __filename are explicitly defined;
import hasDirnameDefined and hasFilenameDefined from ../helpers/mainConfigFile
and use them to short-circuit or filter the
containsDirnameUsage/containsFilenameUsage checks so only undefined, implicit
uses are flagged. Update the import list to include hasDirnameDefined and
hasFilenameDefined, then adjust check() to skip or ignore dirname/filename
usages when those helpers return true (i.e., consider definitions present and do
not treat as faux ESM require).
2 participants
Closes #32598
What I did
fix-faux-esm-requireto analyze compatibility needs for ESM files, including checks forrequireand__dirnameusage.__dirnameandrequireusage, as well as the generation of appropriate compatibility banners.getRequireBannerfunction to conditionally include necessary imports and constants based on the analyzed compatibility needs.Checklist 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-32694-sha-e2746f0e. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-32694-sha-e2746f0e sandboxor in an existing project withnpx storybook@0.0.0-pr-32694-sha-e2746f0e upgrade.More information
0.0.0-pr-32694-sha-e2746f0enorbert/improve-esm-compat-automigratione2746f0e1760439039)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 publish.yml --field pr=32694Summary by CodeRabbit
New Features
Bug Fixes
Tests