-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Core: Fix external-globals-plugin handle undefined cache dir
#32579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Fix external-globals-plugin handle undefined cache dir
#32579
Conversation
WalkthroughThe externalGlobalsPlugin in builder-vite updates how it determines cachePath during serve. Instead of asserting a non-null value from pkg.cache(...), it now uses a nullish coalescing fallback to node_modules/.cache/sb-vite-plugin-externals when pkg.cache(...) returns nullish. This change prevents crashes from null cache values and ensures a default cache directory is used. No other control flow or feature behavior is altered. Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Vite as Vite Dev Server
participant Plugin as externalGlobalsPlugin
participant Pkg as pkg utils
Dev->>Vite: start serve
Vite->>Plugin: initialize plugin
Plugin->>Pkg: cache("sb-vite-plugin-externals")
alt cache path available
Pkg-->>Plugin: return path
note right of Plugin: Use pkg-provided cachePath
else cache path nullish
Pkg-->>Plugin: null/undefined
Plugin-->>Plugin: fallback to "node_modules/.cache/sb-vite-plugin-externals"
note right of Plugin: Changed: added safe fallback
end
Plugin-->>Vite: proceed with resolved cachePath
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/builders/builder-vite/src/plugins/external-globals-plugin.ts (2)
55-57: Good fallback for undefined cache; consider adding a last‑resort tmpdir fallback for unwritable projectsThe nullish fallback prevents the hard crash. In locked or non‑writable environments (e.g., some CI or PnP setups),
process.cwd()/node_modules/.cachemight also be unwritable. Consider a final fallback toos.tmpdir()(namespaced by project path hash) to keep dev server resilient.Example diff within this block:
- const cachePath = - pkg.cache('sb-vite-plugin-externals', { create: true }) ?? - join(process.cwd(), 'node_modules', '.cache', 'sb-vite-plugin-externals'); + const cachePath = + pkg.cache('sb-vite-plugin-externals', { create: true }) ?? + join(process.cwd(), 'node_modules', '.cache', 'sb-vite-plugin-externals');And outside this block, you could add:
import { tmpdir } from 'node:os'; import crypto from 'node:crypto'; const safeCachePath = (p: string | undefined) => p ?? join(tmpdir(), 'sb-vite-plugin-externals', crypto.createHash('md5').update(process.cwd()).digest('hex'));Then use
safeCachePath(cachePath)where you deriveexternalCachePath.
60-63: EscapeexternalKeywhen building the alias RegExpRaw package names can contain regex metacharacters (e.g.,
.). Using them unescaped innew RegExp(\^${externalKey}$`)can lead to accidental over‑matching. You already haveescapeKeys`; reuse it here.- newAlias.push({ find: new RegExp(`^${externalKey}$`), replacement: externalCachePath }); + newAlias.push({ find: new RegExp(`^${escapeKeys(externalKey)}$`), replacement: externalCachePath });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/builders/builder-vite/src/plugins/external-globals-plugin.ts(1 hunks)
🧰 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/builders/builder-vite/src/plugins/external-globals-plugin.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/builders/builder-vite/src/plugins/external-globals-plugin.ts
storybook:external-globals-plugin to handle undefined cache directoryexternal-globals-plugin handle undefined cache dir
|
View your CI Pipeline Execution ↗ for commit f7e6ce3
☁️ Nx Cloud last updated this comment at |
What I did
Updated the
storybook:external-globals-pluginVite plugin to handle the case wherepkg.cache('sb-vite-plugin-externals', { create: true })returnsundefined. This can apparently happen when the directoryempathic/packagefinds is not writeable:This causes a hard failure like this:
The change proposed in this PR is based on how this is handled in
code/core/src/common/utils/resolve-path-in-sb-cache.ts(here).Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
I tested this manually in our repository by patching
@storybook/builder-vite. Not sure it's worth the effort to write an automated test, but happy to try if necessary.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