Core: Resolve builder preset path correctly in pnpm strict mode#34032
Conversation
Use resolvePackageDir instead of dirname when resolving builder preset paths.
This fixes preset resolution in pnpm monorepos with shamefully-hoist=false.
The issue occurs because dirname('@storybook/builder-vite') returns '@storybook',
which is not a valid package path. Using resolvePackageDir properly resolves
the full package directory path, allowing the preset.js file to be found.
Fixes storybookjs#33748
- Add require.resolve fallback in resolvePackageDir for strict pnpm environments where import.meta.resolve may fail - Remove unused dirname import from load.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add support for file URLs and absolute paths in builder name resolution - Check if builder name is already resolved before calling resolvePackageDir - Use dirname for resolved paths, resolvePackageDir for bare package names - Import dirname and isAbsolute from pathe for path detection
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDistinguishes resolved (absolute/file URL) vs bare builder package names when assembling presets, and adds a Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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/shared/utils/module.ts (1)
35-42: Therequire.resolvefallback ignores theparentparameter.The function signature accepts an optional
parentparameter for resolution context, but the new fallback on line 40-41 creates arequirefromimport.meta.url(this module's location) rather than fromparent. If a caller passesparentexpecting it to affect resolution, the fallback won't honor it.For the immediate fix (builder preset resolution in
load.ts),parentis not passed, so this works. However, for consistency and correctness, consider using theparentparameter when available:Proposed fix to respect parent in fallback
} catch { // Fallback using require.resolve for strict pnpm environments where import.meta.resolve may fail - const require = createRequire(import.meta.url); + const require = createRequire(parent ?? import.meta.url); return dirname(require.resolve(join(pkg, 'package.json'))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/shared/utils/module.ts` around lines 35 - 42, The fallback branch in module resolution ignores the optional parent parameter: update the catch block in the function that uses dirname/fileURLToPath/importMetaResolve to create the Require from parent when provided (use createRequire(parent) if parent is a file URL/string), and only fall back to createRequire(import.meta.url) when parent is not passed; then call require.resolve(join(pkg, 'package.json')) via that parent-scoped require so the resolution honors the parent context (refer to the catch block where createRequire and require.resolve are used).
🤖 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/shared/utils/module.ts`:
- Around line 35-42: The fallback branch in module resolution ignores the
optional parent parameter: update the catch block in the function that uses
dirname/fileURLToPath/importMetaResolve to create the Require from parent when
provided (use createRequire(parent) if parent is a file URL/string), and only
fall back to createRequire(import.meta.url) when parent is not passed; then call
require.resolve(join(pkg, 'package.json')) via that parent-scoped require so the
resolution honors the parent context (refer to the catch block where
createRequire and require.resolve are used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 083c6ed3-2eb7-4485-9130-de945105eb83
📒 Files selected for processing (2)
code/core/src/core-server/load.tscode/core/src/shared/utils/module.ts
…vironments - Use `parent` parameter in `createRequire()` to maintain correct module resolution context - Rename `require` to `req` to avoid shadowing the global require function
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 76 KB | 76 KB | 0 B |
| Dependency size | 32.19 MB | 32.17 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 139 KB | 139 KB | 🎉 -48 B 🎉 |
| Dependency size | 30.40 MB | 30.38 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 189 | 189 | 0 |
| Self size | 15 KB | 15 KB | 🚨 +18 B 🚨 |
| Dependency size | 28.90 MB | 28.88 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 534 | 0 |
| Self size | 648 KB | 648 KB | 🚨 +87 B 🚨 |
| Dependency size | 59.86 MB | 59.84 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🚨 +262 B 🚨 |
| Dependency size | 22.47 MB | 22.45 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +18 B 🚨 |
| Dependency size | 23.76 MB | 23.74 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 20.25 MB | 20.24 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 271 | 271 | 0 |
| Self size | 24 KB | 24 KB | 🚨 +12 B 🚨 |
| Dependency size | 44.53 MB | 44.51 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 16 KB | 16 KB | 🎉 -12 B 🎉 |
| Dependency size | 33.44 MB | 33.42 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 779 KB | 0 B |
| Dependency size | 67.35 MB | 67.33 MB | 🎉 -18 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 65.87 MB | 65.86 MB | 🎉 -18 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 167 | 167 | 0 |
| Self size | 18 KB | 18 KB | 0 B |
| Dependency size | 31.40 MB | 31.38 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 58 | 58 | 0 |
| Self size | 1.19 MB | 1.19 MB | 🚨 +215 B 🚨 |
| Dependency size | 13.21 MB | 13.19 MB | 🎉 -19 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
Core: Resolve builder preset path correctly in pnpm strict mode (cherry picked from commit 5c8e0ea)
What I did
Fixed builder preset path resolution in pnpm monorepos with strict module resolution (
shamefully-hoist=false).Supersedes #33750 (closed due to inactivity). This PR addresses the CI failures that were flagged by the reviewer on the original PR.
Closes #33748
Problem
The current code uses
dirname(builderName)to construct the path to the builder's preset file. WhenbuilderNameis a bare package name like@storybook/builder-vite,dirname()performs string manipulation and returns@storybook, resulting in an invalid path@storybook/preset.jsthat causesERR_MODULE_NOT_FOUNDin pnpm's strict resolution mode.Why the original PR (#33750) had CI failures
The original one-line fix (
dirname(builderName)→resolvePackageDir(builderName)) broke the*--vitestsandbox integration tests. The root cause:builderNameis not always a bare package name. Framework presets resolve it viaimport.meta.resolve(), producing either:file:///path/to/node_modules/@storybook/builder-vite/dist/index.js/path/to/node_modules/@storybook/builder-vite/dist/index.jsPassing a file URL or absolute path to
resolvePackageDir()(which expects a bare package name) produced incorrect results, breaking preset loading in the CI sandbox tests.The bare package name case only occurs when users specify the builder directly in their config (e.g.
core: { builder: '@storybook/builder-vite' }), which is the scenario that fails under pnpm strict mode.Solution
code/core/src/core-server/load.ts: Detect whetherbuilderNameis already resolved (file URL or absolute path) vs a bare package name, and use the appropriate resolution strategy for each:dirname()as beforeresolvePackageDir()to properly resolve the package directorycode/core/src/shared/utils/module.ts: Added arequire.resolvefallback inresolvePackageDirfor strict pnpm environments where bothimport.meta.resolvecalls may fail. UsescreateRequire(already imported) as a final fallback.Testing
shamefully-hoist=false@storybook/builder-viteand vitest browser modecore-servertest suite (255 tests pass, 0 regressions)resolvePackageDir(30 tests pass)Summary by CodeRabbit