CLI: Fix CSF factories addon syncing in storybook add command #32728
Conversation
📝 WalkthroughWalkthroughAdds a skipCache option to module importing and propagates it through loadMainConfig; importModule can bypass cache via a timestamped dynamic import or by clearing require.cache. loadMainConfig gains a temporary-file ESM fallback for "require is not defined" errors. CLI add now loads the main config with skipCache true before syncing addons. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI add
participant LMC as loadMainConfig
participant IM as importModule
participant ESM as Dynamic Import
participant CJS as CommonJS (require)
participant TMP as Temp-file ESM fallback
CLI->>LMC: loadMainConfig({ configDir, skipCache: true })
LMC->>IM: importModule(mainPath, { skipCache: true })
alt Dynamic import (ESM)
IM->>ESM: import(mainPath + "?ts=now")
ESM-->>IM: module
else CommonJS fallback
IM->>CJS: if skipCache delete require.cache[resolvedPath]
IM->>CJS: require(resolvedPath)
CJS-->>IM: module or throws
alt "require is not defined" error in LMC
LMC->>TMP: write temp .tmp file with CJS shim
LMC->>IM: import(tempPath)
IM-->>LMC: module
LMC->>TMP: remove temp file
end
end
IM-->>LMC: mainConfig module
LMC-->>CLI: mainConfig
CLI->>CLI: syncStorybookAddons(mainConfig)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧬 Code graph analysis (1)code/core/src/common/utils/load-main-config.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/shared/utils/module.ts (1)
54-57: Update JSDoc to document the new parameter.The function signature now accepts a
skipCacheoption, but the JSDoc comments above (lines 37-53) don't mention this parameter. Please update the documentation to explain when and why to useskipCache.Example addition to the JSDoc:
/** * Dynamically imports a module with TypeScript support, falling back to require if necessary. * + * @param path - The module path to import + * @param options - Import options + * @param options.skipCache - When true, bypasses module cache to ensure a fresh import + * * @example Import a TypeScript presetcode/core/src/common/utils/load-main-config.ts (1)
56-56: Consider usingskipCache: truefor consistency.While the temporary file at
modifiedMainPathis unlikely to be cached, usingskipCache: truehere would maintain consistency with line 26 and prevent any theoretical caching issues.Apply this diff:
- out = await importModule(modifiedMainPath); + out = await importModule(modifiedMainPath, { skipCache: true });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/common/utils/load-main-config.ts(1 hunks)code/core/src/shared/utils/module.ts(2 hunks)code/lib/cli-storybook/src/add.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/common/utils/load-main-config.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/common/utils/load-main-config.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/common/utils/load-main-config.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/common/utils/load-main-config.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.ts
🧬 Code graph analysis (2)
code/core/src/common/utils/load-main-config.ts (1)
code/core/src/shared/utils/module.ts (1)
importModule(54-89)
code/lib/cli-storybook/src/add.ts (2)
code/core/src/common/utils/load-main-config.ts (1)
loadMainConfig(14-69)code/core/src/common/utils/sync-main-preview-addons.ts (1)
syncStorybookAddons(16-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/shared/utils/module.ts (1)
67-69: LGTM! Cache-busting implementation is correct.The dual approach for bypassing cache is well-designed:
- ESM path: Query parameter with timestamp prevents module cache hits
- CJS fallback: Explicit cache deletion before requiring
Both mechanisms correctly target their respective module systems.
Also applies to: 75-77
code/core/src/common/utils/load-main-config.ts (1)
26-26: LGTM! Core fix correctly implemented.Passing
skipCache: trueensures the main config is always loaded fresh, which is essential for the addon synchronization fix described in the PR objectives.code/lib/cli-storybook/src/add.ts (1)
183-184: LGTM! Critical fix correctly implemented.This change directly addresses the PR objective by loading a fresh main config after the addon has been written to
main.js(line 178). The previous implementation passed the stalemainConfigfrom line 92, which was loaded before the addon was added, causing the sync logic to miss the newly added addon.The try/catch wrapper with the TODO comment appropriately handles potential issues while CSF factories is still stabilizing.
|
View your CI Pipeline Execution ↗ for commit 59e030b
☁️ Nx Cloud last updated this comment at |
| // TODO: remove try/catch once CSF factories is shipped, for now gracefully handle any error | ||
| try { | ||
| await syncStorybookAddons(mainConfig, previewConfigPath!, configDir); | ||
| const newMainConfig = await loadMainConfig({ configDir, skipCache: true }); |
There was a problem hiding this comment.
So this is where I was worried about: that loadMainConfig would have a module-cache in NodeJS.
| export async function importModule( | ||
| path: string, | ||
| { skipCache = false }: { skipCache?: boolean } = {} | ||
| ) { | ||
| if (!isTypescriptLoaderRegistered) { | ||
| const typescriptLoaderUrl = importMetaResolve('storybook/internal/bin/loader'); | ||
| register(typescriptLoaderUrl, import.meta.url); | ||
| isTypescriptLoaderRegistered = true; | ||
| } | ||
|
|
||
| let mod; | ||
| try { | ||
| const resolvedPath = win32.isAbsolute(path) ? pathToFileURL(path).href : path; | ||
| mod = await import(resolvedPath); | ||
| // When applicable, add a hash to the import URL to bypass cache | ||
| const importUrl = skipCache ? `${resolvedPath}?${Date.now()}` : resolvedPath; | ||
| mod = await import(importUrl); | ||
| } catch (importError) { | ||
| try { | ||
| // fallback to require to support older behavior | ||
| // this is relevant for presets that are only available with the "require" condition in a package's export map | ||
| const require = createRequire(import.meta.url); | ||
| if (skipCache) { | ||
| delete require.cache[require.resolve(path)]; | ||
| } | ||
| mod = require(path); | ||
| } catch (requireError) { |
There was a problem hiding this comment.
okay, I assume you've tested this, ✅
There was a problem hiding this comment.
For sure, all cases are tested
Closes #32647, #32626
What I did
The CSF factories syncing in the add command was not working because the main config passed was actually the "old" one, where the new addon wasn't present. This PR fixes that and adds a way to bypass cache when importing the main config file as a module, otherwise the main config would always be resolving with the old values.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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 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