Presets: Support extensionless imports in TS-based presets #32641
Conversation
📝 WalkthroughWalkthroughAdds extension resolution and rewriting for relative imports: introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Loader as Loader entry
participant TS as TS transformer
participant Rewriter as addExtensionsToRelativeImports
participant Resolver as resolveWithExtension
participant FS as FileSystem (existsSync)
participant Log as Logger
Loader->>TS: transform(source, filePath)
TS-->>Loader: transformedSource
Loader->>Rewriter: addExtensionsToRelativeImports(transformedSource.code, filePath)
loop for each relative import/export
Rewriter->>Resolver: resolveWithExtension(importPath, filePath)
alt import already has extension
Resolver-->>Rewriter: original importPath
else try supported extensions
Resolver->>FS: existsSync(candidatePath)?
alt found
FS-->>Resolver: true
Resolver->>Log: warn (deprecation + resolved path)
Resolver-->>Rewriter: importPath + ext
else not found
FS-->>Resolver: false
Resolver->>Log: warn (unresolved)
Resolver-->>Rewriter: original importPath
end
end
end
Rewriter-->>Loader: sourceWithExtensions
Loader-->>Loader: return result (code = sourceWithExtensions)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/copilot-instructions.md(2 hunks)code/core/src/bin/loader.test.ts(1 hunks)code/core/src/bin/loader.ts(2 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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.test.tscode/core/src/bin/loader.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/bin/loader.test.tscode/core/src/bin/loader.ts
🧬 Code graph analysis (1)
code/core/src/bin/loader.test.ts (1)
code/core/src/bin/loader.ts (2)
resolveWithExtension(34-62)addExtensionsToRelativeImports(68-90)
⏰ 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
|
View your CI Pipeline Execution ↗ for commit 6f3f65f
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
code/core/src/bin/loader.ts (1)
19-28: Prefer TypeScript extensions before JavaScript when resolving.This issue was already flagged in a previous review. When both
.tsand.jsfiles exist side by side (common in preset repos), resolving.jsfirst will bypass the loader and execute the potentially stale compiled artifact. The order should prioritize TypeScript extensions first, then fall back to JavaScript if no TS file is found.code/core/src/bin/loader.test.ts (1)
13-13: Reset mocks between tests to keep assertions accurate.This issue was already flagged in a previous review. The mock call history is retained between tests, which will cause later assertions expecting zero calls to fail. Add an
afterEach(() => vi.clearAllMocks())after the describe block starts.
🧹 Nitpick comments (2)
code/core/src/bin/loader.ts (1)
40-44: Consider deferring deprecation until a file is found.The deprecation message appears even when the file cannot be resolved, which may confuse users (they see a warning about an import that will fail anyway). Consider moving the
deprecatecall inside the loop when a file is actually found, or structuring the message to distinguish "resolved with extension" from "unresolvable."code/core/src/bin/loader.test.ts (1)
186-248: Consider adding test for imports inside template literals.While the test at lines 242-247 covers comments, there's no test verifying that imports inside template literals or strings are not modified. This could be a useful edge case to validate.
Example test case:
it('should not modify imports inside template literals', () => { const source = `const code = \`import foo from './utils';\`;`; const result = addExtensionsToRelativeImports(source, '/project/src/file.ts'); // Should not be modified since it's inside a template literal expect(result).toBe(source); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/bin/loader.test.ts(1 hunks)code/core/src/bin/loader.ts(2 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/core/src/bin/loader.tscode/core/src/bin/loader.test.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/bin/loader.tscode/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.test.ts
🧠 Learnings (8)
📚 Learning: 2025-09-25T09:21:27.284Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.284Z
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/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.test.ts
🧬 Code graph analysis (1)
code/core/src/bin/loader.test.ts (1)
code/core/src/bin/loader.ts (2)
resolveWithExtension(34-58)addExtensionsToRelativeImports(64-86)
⏰ 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/core/src/bin/loader.ts (1)
98-114: LGTM!The loader integration correctly applies extension rewriting after esbuild transformation, ensuring that the final ESM output has explicit extensions for Node.js compatibility.
code/core/src/bin/loader.test.ts (2)
91-97: LGTM!The
beforeEachblock properly sets up the default mock behavior for the test suite, following the coding guidelines.
99-184: LGTM!These test cases properly rely on the mock setup in
beforeEachand cover a comprehensive range of import scenarios. The test coverage is thorough and well-organized.
…ybookjs/storybook into jeppe/support-extensionless-imports
| it('should handle multi-line dynamic imports', () => { | ||
| const source = `const module = await import( | ||
| './utils' | ||
| );`; | ||
| const result = addExtensionsToRelativeImports(source, '/project/src/file.ts'); | ||
|
|
||
| expect(result).toBe(`const module = await import( | ||
| './utils.ts' | ||
| );`); | ||
| }); |
There was a problem hiding this comment.
Should there be a little bit more coverage on off patterns with import()?
import(`./foo`); // backticks
import('./foo' + foo);
import(foo + 'bar');
import(`${foo}/bar`);
import(`./${foo}/bar`);
const [] = [
import('./foo'),
import('./bar'),
]There was a problem hiding this comment.
I've added support for backticks, but supporting interpolation with runtime values doesn't make much sense, since we'd now have to start looking up those runtime values - probably with AST parsing - do figure out what the final file path is to lookup. that's not worth it imo.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
code/core/src/bin/loader.ts (1)
66-85: Multiline import statements will not be rewritten.As previously noted, the regex patterns do not match multiline import/export statements because
[^'"]+and[^$]+don't match newlines. Real examples exist in the codebase (e.g.,code/addons/vitest/src/preset.ts`). While single-line imports will work, multiline imports will remain extensionless and may fail at runtime in Node.js ESM.
🧹 Nitpick comments (1)
code/core/src/bin/loader.ts (1)
40-44: Consider deduplicating deprecation warnings.The deprecation message is emitted every time
resolveWithExtensionencounters an extensionless import. In large preset configurations or when the same import appears multiple times, this could result in significant log spam. Consider caching which import paths have already triggered a warning (e.g., using a Set) to emit each unique deprecation only once.Example implementation:
+const warnedImports = new Set<string>(); + export function resolveWithExtension(importPath: string, currentFilePath: string): string { // If the import already has an extension, return it as-is if (path.extname(importPath)) { return importPath; } - deprecate(dedent`One or more extensionless imports detected: "${importPath}" in file "${currentFilePath}". + const cacheKey = `${currentFilePath}:${importPath}`; + if (!warnedImports.has(cacheKey)) { + warnedImports.add(cacheKey); + deprecate(dedent`One or more extensionless imports detected: "${importPath}" in file "${currentFilePath}". For maximum compatibility, you should add an explicit file extension to this import. Storybook will attempt to resolve it automatically, but this may change in the future. If adding the extension results in an error from TypeScript, we recommend setting moduleResolution to "bundler" in tsconfig.json or alternatively look into the allowImportingTsExtensions option.`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/bin/loader.test.ts(1 hunks)code/core/src/bin/loader.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/bin/loader.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/bin/loader.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/bin/loader.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:21:27.284Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.284Z
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/bin/loader.ts
⏰ 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/bin/loader.ts (1)
100-115: LGTM!The integration correctly applies extension rewriting after esbuild transformation, ensuring that the rewriting operates on ESM JavaScript output rather than TypeScript input. The sequence (read → transform → rewrite → return) is appropriate for this use case.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
code/core/src/bin/loader.test.ts (3)
11-12: Addspy: trueoption to vi.mock() calls.The coding guidelines require using
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests.As per coding guidelines
Apply this diff:
-vi.mock('node:fs'); -vi.mock('storybook/internal/node-logger'); +vi.mock('node:fs', { spy: true }); +vi.mock('storybook/internal/node-logger', { spy: true });
14-14: Add afterEach to reset mocks between tests.Without resetting mocks between tests, later specs expecting zero calls to
logger.warnwill fail because the mock retains invocations from previous tests.As per coding guidelines
Apply this diff to add mock reset after the vi.mock calls:
vi.mock('node:fs'); vi.mock('storybook/internal/node-logger'); +afterEach(() => { + vi.clearAllMocks(); +}); + describe('loader', () => {
23-99: Move mock implementations to beforeEach blocks.Multiple tests in the
resolveWithExtensiondescribe block have inline mock implementations (lines 27-29, 48-50, 61, 74, 88-90), which violates the coding guidelines. Mock implementations should be placed inbeforeEachblocks for consistency and maintainability.As per coding guidelines
Note that the
addExtensionsToRelativeImportstests (lines 101-351) already follow this pattern correctly with a beforeEach block at lines 102-107.
🧹 Nitpick comments (1)
code/core/src/bin/loader.test.ts (1)
120-124: Consider using it.each for parameterized tests.The forEach pattern works, but Vitest's
it.eachprovides better test isolation and clearer test output when failures occur.Optional refactor:
- it('should not modify imports that already have extensions', () => { - const testCases = [ + it.each([ { input: `import foo from './test.js';`, expected: `import foo from './test.js';` }, { input: `import foo from './test.ts';`, expected: `import foo from './test.ts';` }, { input: `import foo from '../utils.mjs';`, expected: `import foo from '../utils.mjs';` }, { input: `export { bar } from './module.tsx';`, expected: `export { bar } from './module.tsx';`, }, - ]; - - testCases.forEach(({ input, expected }) => { + ])('should not modify imports that already have extensions: $input', ({ input, expected }) => { const result = addExtensionsToRelativeImports(input, '/project/src/file.ts'); expect(result).toBe(expected); expect(deprecate).not.toHaveBeenCalled(); - }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/bin/loader.test.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.test.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/bin/loader.test.ts
🧠 Learnings (14)
📚 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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.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 only a subset of required dependencies
Applied to files:
code/core/src/bin/loader.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} : Place all mocks at the top of the test file before any test cases
Applied to files:
code/core/src/bin/loader.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/core/src/bin/loader.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/core/src/bin/loader.test.ts
🧬 Code graph analysis (1)
code/core/src/bin/loader.test.ts (1)
code/core/src/bin/loader.ts (2)
resolveWithExtension(34-58)addExtensionsToRelativeImports(64-88)
⏰ 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/core/src/bin/loader.test.ts (2)
102-107: LGTM! Good use of beforeEach for default mock setup.This beforeEach correctly establishes a default mock behavior (all
.tsfiles exist) for the test suite, allowing individual tests to focus on specific scenarios. This follows the coding guidelines for mock setup in Vitest tests.
109-351: Excellent test coverage.The test suite comprehensively covers various import/export patterns including static imports, dynamic imports, different quote styles, multi-line imports, template literals, and edge cases. The tests validate both transformation scenarios and cases where imports should remain unchanged (absolute imports, existing extensions, template interpolation).
What I did
SB 10 tries to do as little as possible when loading presets. This requires that users write Node-compliant code as much as possible, including having explicit extensions when importing other files, as has always been the requirement with pure Node. However many users relied on our previous preset loading mechanism previously, that used a bundler to support extensionless imports.
This PR adds support for extensionless imports in TS-based preset files.
It includes a warning to the user, prompting them to add an explicit extenion for future compatibility.
This does not solve the case for JS-based files, as we don't preprocess these at all. We could start doing that, but that would be a big performance penalty as we would also be preprocessing all files within imported packages, etc. so I'd rather not do this. Therefore I've added a migration note instead.
Hopefully we can rely on Node's type stripping in the future and not do any preprocessing at all - this is why we want to warn users today and start migrating them over to explicit extensions.
Checklist for Contributors
Testing
pbpaste | git apply:yarn storybook:uiand verify that it works and shows warningsThe changes in this PR are covered in the following automated tests:
Manual testing
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Documentation
Tests