Core: Fix Manifest Resolution For JS Imports Backed By TSX Files#34983
Core: Fix Manifest Resolution For JS Imports Backed By TSX Files#34983SSDWGG wants to merge 1 commit into
Conversation
|
I updated the title and added the required |
📝 WalkthroughWalkthrough
ChangesModule Resolution Fallback Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/common/utils/interpret-files.resolve.test.ts (1)
19-27: 💤 Low valueConsider adding test cases for fallback priority and other extension mappings.
The test correctly validates the
.js→.tsxfallback. For completeness, consider adding tests for:
.js→.tswhen a.tsfile exists (priority over.tsx).mjs→.mtsand.cjs→.ctsmappings.jsx→.tsxmappingThis would ensure the full fallback chain is covered and prevent regressions.
🧪 Example additional test case for priority
it('resolves a .js import to a .ts file when both .ts and .tsx exist', () => { fixtureDir = mkdtempSync(join(tmpdir(), 'storybook-resolve-import-')); mkdirSync(join(fixtureDir, 'src')); writeFileSync(join(fixtureDir, 'src', 'Chip.ts'), 'export const Chip = () => null;'); writeFileSync(join(fixtureDir, 'src', 'Chip.tsx'), 'export const Chip = () => null;'); const resolved = resolveImport('./src/Chip.js', { basedir: fixtureDir }); expect(resolved).toBe(realpathSync(join(fixtureDir, 'src', 'Chip.ts'))); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/utils/interpret-files.resolve.test.ts` around lines 19 - 27, Add additional unit tests in interpret-files.resolve.test.ts covering the full fallback chain and priority rules for resolveImport: add a test that when both Chip.ts and Chip.tsx exist resolving './src/Chip.js' returns Chip.ts (verifies .js→.ts priority over .tsx), and add tests verifying .mjs→.mts, .cjs→.cts, and .jsx→.tsx mappings by creating appropriate fixture files and asserting resolveImport('./src/YourModule.<ext>') returns the realpath of the expected fallback file; use the same fixture setup pattern (mkdtempSync, mkdirSync, writeFileSync, realpathSync) as existing tests and call resolveImport with { basedir: fixtureDir } to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/core/src/common/utils/interpret-files.resolve.test.ts`:
- Around line 19-27: Add additional unit tests in
interpret-files.resolve.test.ts covering the full fallback chain and priority
rules for resolveImport: add a test that when both Chip.ts and Chip.tsx exist
resolving './src/Chip.js' returns Chip.ts (verifies .js→.ts priority over .tsx),
and add tests verifying .mjs→.mts, .cjs→.cts, and .jsx→.tsx mappings by creating
appropriate fixture files and asserting resolveImport('./src/YourModule.<ext>')
returns the realpath of the expected fallback file; use the same fixture setup
pattern (mkdtempSync, mkdirSync, writeFileSync, realpathSync) as existing tests
and call resolveImport with { basedir: fixtureDir } to locate the code under
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8b8e00f-ae79-44e7-a580-2f367289c25e
📒 Files selected for processing (2)
code/core/src/common/utils/interpret-files.resolve.test.tscode/core/src/common/utils/interpret-files.ts
Summary
Fixes #34812.
When
resolveImport()falls back from an ESM-style.jsimport to TypeScript sources, it currently only checks the sibling.tsfile. That misses common React component files such asChip.tsx, so the component manifest can reportNo component file foundeven though the story renders correctly through the bundler.This PR keeps the existing
.js -> .tsbehavior and adds a second.js -> .tsxfallback when no.tsfile resolves. It also adds a regression test using a real temporary fixture directory so the resolver path matches production behavior.Testing
yarn install --immutableyarn vitest run --config code/core/vitest.config.ts code/core/src/common/utils/interpret-files.resolve.test.ts code/core/src/common/utils/__tests__/interpret-files.test.tsNotes
I attempted the repository lint command locally, but this shallow fork checkout cannot resolve the workspace-built
eslint-plugin-storybookpackage fromcode/.eslintrc.js. The focused Vitest regression passes locally; CI should run the full workspace pipeline with generated packages available.Manual testing
Not run locally; this change is covered by the added automated regression test.
Summary by CodeRabbit
Tests
Bug Fixes