Core: Resolve .js imports to .ts files in TypeScript loader#33698
Merged
valentinpalkovic merged 10 commits intoMar 20, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces existence checks with a directory-content cache, adds JS→TS extension mapping in resolution logic, introduces Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/core/src/bin/loader.ts`:
- Around line 47-68: The resolution logic around extImportPath in loader.ts
currently checks for the original JS file first; update the logic in the block
using extImportPath, importPath, currentDir, jsToTsExtensionMap and existsSync
so it first iterates tsExtensions = jsToTsExtensionMap[extImportPath] and checks
each candidate `${basePath}${tsExt}` (resolved with path.resolve) for existence,
returning the first match, and only if none exist fall back to checking the
original importPath (e.g., `${basePath}${extImportPath}`) / returning
importPath; apply the same ordering for .mjs→.mts and .cjs→.cts cases so
TypeScript alternatives are preferred before the JS file.
… TypeScript alternatives over JS
valentinpalkovic
changed the title
ndelangen
reviewed
Feb 2, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
code/core/src/bin/loader.test.ts (3)
14-15: Addspy: trueoption tovi.mock()calls.As per coding guidelines, all package and file mocks should use the
spy: trueoption:-vi.mock('node:fs'); -vi.mock('storybook/internal/node-logger'); +vi.mock('node:fs', { spy: true }); +vi.mock('storybook/internal/node-logger', { spy: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/bin/loader.test.ts` around lines 14 - 15, Update the two test mocks in loader.test.ts to use the spy option: modify the vi.mock calls for 'node:fs' and 'storybook/internal/node-logger' to pass the { spy: true } option (i.e., vi.mock('node:fs', { spy: true }) and vi.mock('storybook/internal/node-logger', { spy: true })) so both mocks comply with the codebase guideline requiring spy: true.
22-85: Consider moving mock setup tobeforeEachfor consistency.The
resolveWithExtensiontests set mock behavior inline within each test case (lines 31, 42, 53, 66, 77), while theaddExtensionsToRelativeImportsblock uses abeforeEach(lines 89-100). For consistency and per coding guidelines recommending mock implementations inbeforeEachblocks, consider restructuring:♻️ Suggested pattern
describe('resolveWithExtension', () => { + beforeEach(() => { + vi.mocked(readdirSync).mockReturnValue([] as any); + }); + it('should return the path as-is if it already has an extension', () => { const result = resolveWithExtension('./test.js', '/project/src/file.ts'); // ... }); it('should resolve extensionless import to .ts extension when file exists', () => { vi.mocked(readdirSync).mockReturnValue(['utils.ts'] as any); // Override for specific test case🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/bin/loader.test.ts` around lines 22 - 85, Move the inline readdirSync mocks used in the resolveWithExtension tests into a describe-level beforeEach: in the describe('resolveWithExtension', ...) block add a beforeEach that sets a default vi.mocked(readdirSync).mockReturnValue(...) and then remove the repetitive mockReturnValue calls from individual it(...) tests, only overriding the mock inside a specific test when needed (so tests like the ones referencing './utils', './missing', '../utils' still override the default). This centralizes the mock setup for resolveWithExtension while allowing per-test overrides and references the resolveWithExtension function and readdirSync mock used throughout the test cases.
102-117: Update test description for accuracy.The test name says "non-mapped extensions" but
.mjsis now a mapped extension (it maps to.mtsperjsToTsExtensionMap). The test actually verifies that when no TypeScript alternative exists, the original import is preserved. Consider updating the description:-it('should not modify imports that already have non-mapped extensions', () => { +it('should not modify imports with TypeScript extensions or when no TS alternative exists', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/bin/loader.test.ts` around lines 102 - 117, Update the test title to accurately describe the behavior being verified: it checks that addExtensionsToRelativeImports preserves the original import when there is no TypeScript alternative (even if the original extension appears in jsToTsExtensionMap), so rename the test string from "should not modify imports that already have non-mapped extensions" to something like "should not modify imports when no TypeScript alternative exists"; leave the assertions against addExtensionsToRelativeImports(...) and expect(deprecate).not.toHaveBeenCalled() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/bin/loader.test.ts`:
- Around line 14-15: Update the two test mocks in loader.test.ts to use the spy
option: modify the vi.mock calls for 'node:fs' and
'storybook/internal/node-logger' to pass the { spy: true } option (i.e.,
vi.mock('node:fs', { spy: true }) and vi.mock('storybook/internal/node-logger',
{ spy: true })) so both mocks comply with the codebase guideline requiring spy:
true.
- Around line 22-85: Move the inline readdirSync mocks used in the
resolveWithExtension tests into a describe-level beforeEach: in the
describe('resolveWithExtension', ...) block add a beforeEach that sets a default
vi.mocked(readdirSync).mockReturnValue(...) and then remove the repetitive
mockReturnValue calls from individual it(...) tests, only overriding the mock
inside a specific test when needed (so tests like the ones referencing
'./utils', './missing', '../utils' still override the default). This centralizes
the mock setup for resolveWithExtension while allowing per-test overrides and
references the resolveWithExtension function and readdirSync mock used
throughout the test cases.
- Around line 102-117: Update the test title to accurately describe the behavior
being verified: it checks that addExtensionsToRelativeImports preserves the
original import when there is no TypeScript alternative (even if the original
extension appears in jsToTsExtensionMap), so rename the test string from "should
not modify imports that already have non-mapped extensions" to something like
"should not modify imports when no TypeScript alternative exists"; leave the
assertions against addExtensionsToRelativeImports(...) and
expect(deprecate).not.toHaveBeenCalled() unchanged.
Contributor
|
Hi @yatishgoel, Thank you for the follow-up! Could you fix the remaining eslint/prettier violations? |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/bin/loader.ts (1)
43-49: Narrow the catch block to expected directory-miss errors.Line 47 currently swallows all
readdirSyncfailures and silently treats them as “directory empty,” which can mask real runtime problems (for example permission issues).Suggested hardening
function getDirectoryFiles(dir: string): Set<string> { if (!directoryCache.has(dir)) { try { directoryCache.set(dir, new Set(readdirSync(dir))); - } catch { - directoryCache.set(dir, new Set()); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOENT' || code === 'ENOTDIR') { + directoryCache.set(dir, new Set()); + } else { + throw error; + } } } return directoryCache.get(dir)!; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/bin/loader.ts` around lines 43 - 49, The catch in getDirectoryFiles currently swallows all errors from readdirSync which masks real failures; change it to catch the error object (e.g., catch (err)) and only treat missing-directory cases as empty by checking err.code for expected values (like 'ENOENT' and 'ENOTDIR'), setting directoryCache.set(dir, new Set()) for those cases, and rethrow or propagate/log all other errors so permission and unexpected failures are not silently ignored; reference getDirectoryFiles, directoryCache and the readdirSync call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/bin/loader.ts`:
- Around line 43-49: The catch in getDirectoryFiles currently swallows all
errors from readdirSync which masks real failures; change it to catch the error
object (e.g., catch (err)) and only treat missing-directory cases as empty by
checking err.code for expected values (like 'ENOENT' and 'ENOTDIR'), setting
directoryCache.set(dir, new Set()) for those cases, and rethrow or propagate/log
all other errors so permission and unexpected failures are not silently ignored;
reference getDirectoryFiles, directoryCache and the readdirSync call when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebb90741-3d42-4064-ad27-59ac0ff831b1
📒 Files selected for processing (1)
code/core/src/bin/loader.ts
valentinpalkovic
moved this from Empathy Queue (prioritized)
to In Progress
in Core Team Projects
ndelangen
approved these changes
Mar 19, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/bin/loader.ts (1)
96-106: Consider reorderingsupportedExtensionsto prioritize TypeScript files.For extensionless imports, the current
supportedExtensionsorder checks.jsbefore.ts. In a TypeScript project, users likely expect TypeScript files to be resolved first. While this is a minor concern since extensionless imports are deprecated, reordering to['.ts', '.tsx', '.mts', '.cts', '.js', '.mjs', '.cjs', '.jsx']would provide more intuitive behavior during the deprecation period.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/bin/loader.ts` around lines 96 - 106, Change the resolution order of the supportedExtensions array in loader.ts to prioritize TypeScript variants before JavaScript so extensionless imports resolve to .ts/.tsx first; update the supportedExtensions symbol to the suggested order ['.ts', '.tsx', '.mts', '.cts', '.js', '.mjs', '.cjs', '.jsx'] (affects the logic that iterates over supportedExtensions in the code block that computes absolutePath, dirFiles, and baseFileName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/bin/loader.ts`:
- Around line 96-106: Change the resolution order of the supportedExtensions
array in loader.ts to prioritize TypeScript variants before JavaScript so
extensionless imports resolve to .ts/.tsx first; update the supportedExtensions
symbol to the suggested order ['.ts', '.tsx', '.mts', '.cts', '.js', '.mjs',
'.cjs', '.jsx'] (affects the logic that iterates over supportedExtensions in the
code block that computes absolutePath, dirFiles, and baseFileName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81566f51-2ff8-4645-8bc4-3371c766a418
📒 Files selected for processing (1)
code/core/src/bin/loader.ts
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 121 | 124 | 🚨 +3 🚨 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 23.54 MB | 23.78 MB | 🚨 +240 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
Contributor
|
Tested out the canary successfully on https://stackblitz.com/edit/vitejs-vite-pkn6dr6b?file=package.json. |
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
17 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #32757
What I did
Added support for resolving JavaScript extension imports to their TypeScript equivalents in the esbuild loader.
The Problem:
When using TypeScript with
moduleResolution: "Node16"or"NodeNext", it's standard practice to use.jsextensions in imports even when source files are.ts. The loader was returning the.jspath as-is without checking if a.tsfile should be used instead, causing "Cannot find module" errors.The Fix:
jsToTsExtensionMapto map JS extensions to TS equivalentsresolveWithExtension()to check if.jsfile exists; if not, try TypeScript alternatives.js→.ts/.tsx,.mjs→.mts,.cjs→.cts,.jsx→.tsxExample:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
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-33698-sha-b652f4b7. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33698-sha-b652f4b7 sandboxor in an existing project withnpx storybook@0.0.0-pr-33698-sha-b652f4b7 upgrade.More information
0.0.0-pr-33698-sha-b652f4b7fix/32757-ts-loader-js-extension-resolutionb652f4b71773995765)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=33698Summary by CodeRabbit