Maintenance: Replace resolve and resolve.exports with oxc-resolver#34692
Conversation
Now that `oxc-resolver` is a runtime dependency of `code/core` (added in
the change-detection PR), the two remaining call sites of the legacy
`resolve` and `resolve.exports` packages can be migrated:
- `common/utils/interpret-files.ts`: `resolve.sync` + `packageFilter`
(prefer `module` over `main`) → `ResolverFactory.sync` with
`mainFields: ['module', 'main']`.
- `mocking-utils/resolve.ts`: `resolve.exports({ browser: true })` on a
manually-fetched `package.json` → `ResolverFactory.sync` with
`conditionNames: ['browser', 'import', 'module', 'default']`,
`mainFields: ['browser', 'module', 'main']`, and
`aliasFields: [['browser']]`. The `require.resolve` fallback for
legacy package layouts is preserved.
`resolve` and `resolve.exports` are removed from `code/core`'s
dependencies.
The public `resolveImport(id, options)` signature changes from
`resolve.SyncOpts` to `{ basedir: string }`. Both internal callers
(`StoryIndexGenerator.resolveComponentPath`, `cachedResolveImport` in
the React renderer's component manifest) only pass `basedir`, so this
is a safe narrowing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughReplaced ChangesDependency & Resolver Migration to oxc-resolver
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/common/utils/interpret-files.ts`:
- Around line 30-33: The ResolverFactory instantiated as importResolver lacks
conditionNames so packages using conditional exports won't resolve; update the
ResolverFactory call that creates importResolver (the ResolverFactory
instantiation near supportedExtensions/mainFields) to include conditionNames set
for server-side ESM (e.g., ['node','import']) so ESM conditional exports are
honored when resolving imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dabe5719-4118-433f-a4a6-b6a25956f9fc
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
code/core/package.jsoncode/core/src/common/utils/interpret-files.tscode/core/src/mocking-utils/resolve.ts
💤 Files with no reviewable changes (1)
- code/core/package.json
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 20.30 MB | 20.25 MB | 🎉 -53 KB 🎉 |
| Dependency size | 36.17 MB | 36.17 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 203 | 0 |
| Self size | 908 KB | 908 KB | 0 B |
| Dependency size | 87.59 MB | 87.54 MB | 🎉 -53 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 86.08 MB | 86.03 MB | 🎉 -53 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.08 MB | 0 B |
| Dependency size | 56.48 MB | 56.43 MB | 🎉 -53 KB 🎉 |
| Bundle Size Analyzer | node | node |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/common/utils/interpret-files.ts (1)
39-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTry all matching TypeScript source extensions in the fallback path.
This retry only remaps to one target (
.ts, or.tsxfor.jsx), so valid files likefoo.tsx,foo.mts, orfoo.ctsstill fail even though they are listed insupportedExtensions. That turns legitimate ESM-style imports into false negatives.Suggested fix
export function resolveImport(id: string, options: ResolveImportOptions): string { try { return resolveSync(id, options.basedir); } catch (error) { const ext = extname(id); @@ - const newId = ['.js', '.mjs', '.cjs'].includes(ext) - ? `${id.slice(0, -2)}ts` - : ext === '.jsx' - ? `${id.slice(0, -3)}tsx` - : null; - - if (!newId) { - throw error; - } - return resolveSync(newId, options.basedir); + const candidates = + ext === '.js' + ? [`${id.slice(0, -2)}ts`, `${id.slice(0, -2)}tsx`] + : ext === '.mjs' + ? [`${id.slice(0, -3)}mts`, `${id.slice(0, -3)}mtsx`] + : ext === '.cjs' + ? [`${id.slice(0, -3)}cts`, `${id.slice(0, -3)}ctsx`] + : ext === '.jsx' + ? [`${id.slice(0, -3)}tsx`] + : []; + + for (const candidate of candidates) { + try { + return resolveSync(candidate, options.basedir); + } catch { + // Try the next interpreted extension. + } + } + + throw error; } }🤖 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.ts` around lines 39 - 59, In resolveImport, replace the single remap (newId) logic with trying all TypeScript-related candidate extensions instead of one; when extname(id) is one of '.js', '.mjs', '.cjs' or '.jsx' build a list of candidate replacements (e.g. map '.js' family to ['.ts','.mts','.cts'] and '.jsx' to ['.tsx','.mts','.cts'] or use the module's supportedExtensions list) and iterate calling resolveSync(candidateId, options.basedir) until one succeeds, returning the resolved path; if none succeed rethrow the original caught error—use the existing symbols resolveImport, resolveSync, extname and preserve the original error behavior.
🤖 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.
Outside diff comments:
In `@code/core/src/common/utils/interpret-files.ts`:
- Around line 39-59: In resolveImport, replace the single remap (newId) logic
with trying all TypeScript-related candidate extensions instead of one; when
extname(id) is one of '.js', '.mjs', '.cjs' or '.jsx' build a list of candidate
replacements (e.g. map '.js' family to ['.ts','.mts','.cts'] and '.jsx' to
['.tsx','.mts','.cts'] or use the module's supportedExtensions list) and iterate
calling resolveSync(candidateId, options.basedir) until one succeeds, returning
the resolved path; if none succeed rethrow the original caught error—use the
existing symbols resolveImport, resolveSync, extname and preserve the original
error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82dc3e88-53cf-469f-8662-22291378004e
📒 Files selected for processing (2)
code/core/src/common/utils/interpret-files.tscode/core/src/mocking-utils/resolve.ts
Sidnioulz
left a comment
There was a problem hiding this comment.
LGTM, but check the comment on extensions. We might need to do more to support community frameworks.
Apply suggestion from @Sidnioulz — there's no naming conflict with the other ResolverFactory consumer (interpret-files.ts is a separate module), so the `as OxcResolverFactory` alias is unnecessary noise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address @Sidnioulz's review feedback. The two extension lists in resolve.ts both feed sb.mock() resolution but had drifted (`.json` and `.jsx` were missing from the require.resolve trial loop). Extract a single `moduleExtensions` constant used by both `externalResolver` and `resolveWithExtensions`. Side effect: `resolveWithExtensions` now also tries `.json` and `.jsx` — pure improvement, no previously successful resolution changes target. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address @Sidnioulz's review feedback (PR #34692). Inline extension lists were scattered across code/core with subtle drift between similar tasks. Introduces `code/core/src/shared/constants/extensions.ts` with four task-named constants: - `jsModuleExtensions` — .mjs/.js/.cjs only - `jsTsSourceExtensions` — JS/TS family incl. JSX/TSX - `storybookConfigExtensions` — main/preview/manager config files - `userModuleExtensions` — user code incl. JSON, .vue, .svelte Migrates five consumers: - common/utils/interpret-files.ts → storybookConfigExtensions (keeps the public `supportedExtensions` re-export used by the react renderer's component manifest) - mocking-utils/resolve.ts → userModuleExtensions - common/utils/validate-config.ts → jsModuleExtensions - shared/utils/module.ts → jsModuleExtensions - core-server/change-detection/parser-registry/builtins.ts → jsTsSourceExtensions Left intentionally inline (different task / subtle behavior): - bin/loader.ts — runtime ESM/CJS loader, narrower set - builder-manager/index.ts — esbuild bundler config - telemetry/get-application-file-count.ts — git ls-files glob (no dots) No behavioral changes; pure deduplication. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #
What I did
Stacked on top of #34625 (which introduces oxc-resolver and oxc-parser as runtime deps of code/core).
Now that oxc-resolver is available, the two remaining call sites of the legacy resolve and resolve.exports npm packages in code/core can be migrated. This PR removes both packages.
Why now
The change-detection PR (#34625) added oxc-resolver to code/core's runtime deps for its dependency-graph builder, but did not migrate the two pre-existing call sites that were still on the older resolvers. This PR closes that loop and lets us drop two transitive deps.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
The existing
GitDiffProvider.test.tscontinues to pass (13/13). No new tests are added because the assertion would be "execa was called with a specific env" which over-specifies an implementation detail; the user-visible behavior is "concurrent commits no longer fail," which is verified manually.Manual testing
cd code && yarn storybook:ui(or run any sandbox dev server in a repo).touch code/core/src/foo.ts && git add code/core/src/foo.ts && git commit -m wipwhile the dev server is running.fatal: Unable to create '.git/index.lock': File exists.Documentation
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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit