CLI: Drop contextual dependencies in create-storybook#32134
Conversation
|
View your CI Pipeline Execution ↗ for commit 12ded7f
☁️ Nx Cloud last updated this comment at |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
valentinpalkovic
added
addon: vitest
cleanup
43081j
force-pushed
the
config-files-context
branch
from
2a0b22f to
520b09c
Compare
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
the bundle analysis says we somehow gained 29KB 🤔 but clicking through to the treeemap - both look identical (and should be) 😅 |
We've had this TODO for a while. Basically, we no longer pull dependencies from the context, so we don't need to mock it or populate it anymore.
43081j
force-pushed
the
config-files-context
branch
from
520b09c to
5d00cfd
Compare
|
have caught this up, let me know if we can land this its already dead code in that the condition is always truthy, so it should only be a patch change |
ndelangen
changed the title
create-storybook
|
@43081j Sorry for the disruption in the Things should be stable now. |
|
no worries, this should pass now 👍 it does locally but will keep an eye on CI |
…iles.test.ts` - Adjusted the formatting of the `coerce` function to enhance clarity and maintainability.
WalkthroughRefactors vitestConfigFiles tests to a fully mock-driven suite and consolidates the implementation’s validation flow. The condition now unconditionally checks workspace and config files, aggregates incompatibility reasons, and returns a single compatibility result. Tests mock file discovery and reads, expanding scenario coverage without live filesystem or dependency gating. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as StepRunner
participant Check as vitestConfigFiles.condition
participant Finder as find.any
participant FS as readFile
participant Babel as Babel Parser
Runner->>Check: invoke(state)
Note over Check: Start unified validation flow
Check->>Finder: locate vitest workspace file
alt Workspace file found
Check->>FS: read workspace file
Check->>Check: validate workspace syntax/options
else No workspace file
Note over Check: Skip workspace checks
end
Check->>Finder: locate vitest.config.* file
alt Config file found
Check->>FS: read config file
Check->>Babel: parse and inspect for safe DefineConfig
Check->>Check: validate config/testConfig usage
else No config file
Note over Check: Skip config checks
end
Check->>Check: Aggregate reasons (if any)
Check-->>Runner: COMPATIBLE or INCOMPATIBLE (+ reasons)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
91-99: Type the reasons array to avoid implicit any[]Prevents accidental non-string pushes and improves type safety.
- const reasons = []; + const reasons: string[] = [];code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (4)
7-9: Use spy: true and mock all required exports from empathic/findAligns with repo mocking rules and avoids accidental partial-module shape mismatches (e.g., find.up).
-vi.mock('empathic/find', () => ({ - any: vi.fn(), -})); +vi.mock( + 'empathic/find', + () => ({ + any: vi.fn(), + up: vi.fn(), + }), + { spy: true } +);
83-86: Make coerce return undefined when there is no matchBetter models find.any semantics and avoids accidental “found” files in unrelated checks.
-const coerce = - (from: string, to: string) => - ([name]: string[]) => - name.includes(from) ? to : name; +const coerce = + (from: string, to: string) => + (names: string[]) => + names.some((n) => n.includes(from)) ? to : undefined;
92-97: Reset mocks between testsPrevents cross-test leakage and adheres to our mocking rules.
describe('vitestConfigFiles', () => { + beforeEach(() => { + vi.mocked(find.any).mockReset(); + }); + afterEach(() => { + vi.resetAllMocks(); + }); it('should run properly with mock dependencies', async () => {
100-101: Inline mock implementations in it-blocksPrefer setting per-case implementations in a scoped beforeEach for each describe to comply with our rules; keeping as-is is functionally fine but deviates from the guideline.
Would you like me to push a small refactor that moves these mockImplementation calls into per-scope beforeEach blocks?
Also applies to: 118-119, 126-127, 137-138, 146-147, 155-156, 163-164, 172-173, 180-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts(3 hunks)code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursorrules:0-0
Timestamp: 2025-09-17T08:11:04.275Z
Learning: Applies to code/vitest.workspace.ts : Keep and use the Vitest workspace configuration at code/vitest.workspace.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:04.275Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursorrules:0-0
Timestamp: 2025-09-17T08:11:04.275Z
Learning: Applies to code/vitest.workspace.ts : Keep and use the Vitest workspace configuration at code/vitest.workspace.ts
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without vi.mocked()
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to type and access mocked functions
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with vi.mocked()
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
📚 Learning: 2025-09-17T08:11:47.184Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.184Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
🧬 Code graph analysis (2)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (1)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
vitestConfigFiles(90-138)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
code/core/src/common/utils/paths.ts (1)
getProjectRoot(11-57)
⏰ 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)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
134-137: Return path simplification LGTMClear, single decision point; reads well.
| vi.mock(import('node:fs/promises'), async (importOriginal) => { | ||
| const actual = await importOriginal(); | ||
| return { | ||
| ...actual, | ||
| readFile: vi | ||
| .fn() | ||
| .mockImplementation((filePath) => fileMocks[filePath as keyof typeof fileMocks]), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Invalid vi.mock target and non-async readFile mock
First arg must be the module id string. Also return a Promise for async APIs and use spy: true per rules.
-vi.mock(import('node:fs/promises'), async (importOriginal) => {
- const actual = await importOriginal();
- return {
- ...actual,
- readFile: vi
- .fn()
- .mockImplementation((filePath) => fileMocks[filePath as keyof typeof fileMocks]),
- };
-});
+vi.mock(
+ 'node:fs/promises',
+ async () => {
+ const actual = await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises');
+ return {
+ ...actual,
+ readFile: vi.fn(async (filePath: string) =>
+ Promise.resolve(fileMocks[filePath as keyof typeof fileMocks])
+ ),
+ };
+ },
+ { spy: true }
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.mock(import('node:fs/promises'), async (importOriginal) => { | |
| const actual = await importOriginal(); | |
| return { | |
| ...actual, | |
| readFile: vi | |
| .fn() | |
| .mockImplementation((filePath) => fileMocks[filePath as keyof typeof fileMocks]), | |
| }; | |
| }); | |
| vi.mock( | |
| 'node:fs/promises', | |
| async () => { | |
| const actual = await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises'); | |
| return { | |
| ...actual, | |
| readFile: vi.fn(async (filePath: string) => | |
| Promise.resolve(fileMocks[filePath as keyof typeof fileMocks]) | |
| ), | |
| }; | |
| }, | |
| { spy: true } | |
| ); |
🤖 Prompt for AI Agents
In code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
around lines 71-79, the vi.mock call is using import(...) as the first arg and
returning a non-async readFile mock; change it to mock the module by its string
id ('node:fs/promises'), use vi.importActual to get the real implementation,
return an object shaped like the module (exporting the actual members) and
ensure readFile is mocked as an async function (use
vi.fn().mockResolvedValue(...) or mockImplementation returning a Promise) and
pass { spy: true } in the vi.mock options so spies are allowed; ensure the mock
factory returns the module object (or Promise of it) as required by Vitest.
| const fileContents = await fs.readFile(vitestWorkspaceFile, 'utf8'); | ||
| if (!isValidWorkspaceConfigFile(fileContents, babel)) { | ||
| reasons.push(`Found an invalid workspace config file: ${vitestWorkspaceFile}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Harden workspace parsing with try/catch; drop unused babel arg
Avoids unhandled exceptions on unreadable/invalid files and removes the unused parameter.
- const fileContents = await fs.readFile(vitestWorkspaceFile, 'utf8');
- if (!isValidWorkspaceConfigFile(fileContents, babel)) {
- reasons.push(`Found an invalid workspace config file: ${vitestWorkspaceFile}`);
- }
+ try {
+ const fileContents = await fs.readFile(vitestWorkspaceFile, 'utf8');
+ if (!isValidWorkspaceConfigFile(fileContents)) {
+ reasons.push(`Found an invalid workspace config file: ${vitestWorkspaceFile}`);
+ }
+ } catch {
+ reasons.push(`Error reading/parsing workspace config file: ${vitestWorkspaceFile}`);
+ }Outside this hunk, update the helper’s signature to match usage:
-export const isValidWorkspaceConfigFile: (fileContents: string, babel: any) => boolean = (
- fileContents
-) => {
+export const isValidWorkspaceConfigFile = (fileContents: string): boolean => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fileContents = await fs.readFile(vitestWorkspaceFile, 'utf8'); | |
| if (!isValidWorkspaceConfigFile(fileContents, babel)) { | |
| reasons.push(`Found an invalid workspace config file: ${vitestWorkspaceFile}`); | |
| } | |
| } | |
| try { | |
| const fileContents = await fs.readFile(vitestWorkspaceFile, 'utf8'); | |
| if (!isValidWorkspaceConfigFile(fileContents)) { | |
| reasons.push(`Found an invalid workspace config file: ${vitestWorkspaceFile}`); | |
| } | |
| } catch { | |
| reasons.push(`Error reading/parsing workspace config file: ${vitestWorkspaceFile}`); | |
| } | |
| } |
| const fileContents = await fs.readFile(vitestWorkspaceFile, 'utf8'); | |
| if (!isValidWorkspaceConfigFile(fileContents, babel)) { | |
| reasons.push(`Found an invalid workspace config file: ${vitestWorkspaceFile}`); | |
| } | |
| } | |
| export const isValidWorkspaceConfigFile = (fileContents: string): boolean => { |
🤖 Prompt for AI Agents
In code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx around
lines 103 to 107, wrap the fs.readFile + isValidWorkspaceConfigFile call in a
try/catch so unreadable or invalid files don't throw (on error push a
descriptive reason like `Unable to read/parse workspace config:
${vitestWorkspaceFile}`); remove the unused `babel` argument from the
isValidWorkspaceConfigFile call here and update the helper function's signature
and all its call sites elsewhere to drop that parameter so signatures and usages
remain consistent.
| const vitestConfigFile = find.any( | ||
| ['ts', 'js', 'tsx', 'jsx', 'cts', 'cjs', 'mts', 'mjs'].map((ex) => `vitest.config.${ex}`), | ||
| { cwd: state.directory, last: projectRoot } | ||
| ); | ||
| if (vitestConfigFile?.endsWith('.cts') || vitestConfigFile?.endsWith('.cjs')) { | ||
| reasons.push(`Cannot auto-update CommonJS config file: ${vitestConfigFile}`); | ||
| } else if (vitestConfigFile) { | ||
| let isValidVitestConfig = false; | ||
| const configContent = await fs.readFile(vitestConfigFile, 'utf8'); | ||
| const parsedConfig = babel.babelParse(configContent); | ||
| babel.traverse(parsedConfig, { | ||
| ExportDefaultDeclaration(path) { | ||
| if ( | ||
| isDefineConfigExpression(path.node.declaration) && | ||
| isSafeToExtendWorkspace(path.node.declaration as CallExpression) | ||
| ) { | ||
| isValidVitestConfig = true; | ||
| } | ||
| }, | ||
| }); | ||
| if (!isValidVitestConfig) { | ||
| reasons.push(`Found an invalid Vitest config file: ${vitestConfigFile}`); | ||
| } | ||
|
|
||
| return reasons.length | ||
| ? { type: CompatibilityType.INCOMPATIBLE, reasons } | ||
| : { type: CompatibilityType.COMPATIBLE }; | ||
| } |
There was a problem hiding this comment.
Guard Vitest config read/parse with try/catch
Prevents crashes on IO/parse errors and surfaces actionable reasons.
- } else if (vitestConfigFile) {
- let isValidVitestConfig = false;
- const configContent = await fs.readFile(vitestConfigFile, 'utf8');
- const parsedConfig = babel.babelParse(configContent);
- babel.traverse(parsedConfig, {
- ExportDefaultDeclaration(path) {
- if (
- isDefineConfigExpression(path.node.declaration) &&
- isSafeToExtendWorkspace(path.node.declaration as CallExpression)
- ) {
- isValidVitestConfig = true;
- }
- },
- });
- if (!isValidVitestConfig) {
- reasons.push(`Found an invalid Vitest config file: ${vitestConfigFile}`);
- }
- }
+ } else if (vitestConfigFile) {
+ try {
+ let isValidVitestConfig = false;
+ const configContent = await fs.readFile(vitestConfigFile, 'utf8');
+ const parsedConfig = babel.babelParse(configContent);
+ babel.traverse(parsedConfig, {
+ ExportDefaultDeclaration(path) {
+ if (
+ isDefineConfigExpression(path.node.declaration) &&
+ isSafeToExtendWorkspace(path.node.declaration as CallExpression)
+ ) {
+ isValidVitestConfig = true;
+ }
+ },
+ });
+ if (!isValidVitestConfig) {
+ reasons.push(`Found an invalid Vitest config file: ${vitestConfigFile}`);
+ }
+ } catch {
+ reasons.push(`Error reading/parsing Vitest config file: ${vitestConfigFile}`);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const vitestConfigFile = find.any( | |
| ['ts', 'js', 'tsx', 'jsx', 'cts', 'cjs', 'mts', 'mjs'].map((ex) => `vitest.config.${ex}`), | |
| { cwd: state.directory, last: projectRoot } | |
| ); | |
| if (vitestConfigFile?.endsWith('.cts') || vitestConfigFile?.endsWith('.cjs')) { | |
| reasons.push(`Cannot auto-update CommonJS config file: ${vitestConfigFile}`); | |
| } else if (vitestConfigFile) { | |
| let isValidVitestConfig = false; | |
| const configContent = await fs.readFile(vitestConfigFile, 'utf8'); | |
| const parsedConfig = babel.babelParse(configContent); | |
| babel.traverse(parsedConfig, { | |
| ExportDefaultDeclaration(path) { | |
| if ( | |
| isDefineConfigExpression(path.node.declaration) && | |
| isSafeToExtendWorkspace(path.node.declaration as CallExpression) | |
| ) { | |
| isValidVitestConfig = true; | |
| } | |
| }, | |
| }); | |
| if (!isValidVitestConfig) { | |
| reasons.push(`Found an invalid Vitest config file: ${vitestConfigFile}`); | |
| } | |
| return reasons.length | |
| ? { type: CompatibilityType.INCOMPATIBLE, reasons } | |
| : { type: CompatibilityType.COMPATIBLE }; | |
| } | |
| const vitestConfigFile = find.any( | |
| ['ts', 'js', 'tsx', 'jsx', 'cts', 'cjs', 'mts', 'mjs'].map((ex) => `vitest.config.${ex}`), | |
| { cwd: state.directory, last: projectRoot } | |
| ); | |
| if (vitestConfigFile?.endsWith('.cts') || vitestConfigFile?.endsWith('.cjs')) { | |
| reasons.push(`Cannot auto-update CommonJS config file: ${vitestConfigFile}`); | |
| } else if (vitestConfigFile) { | |
| try { | |
| let isValidVitestConfig = false; | |
| const configContent = await fs.readFile(vitestConfigFile, 'utf8'); | |
| const parsedConfig = babel.babelParse(configContent); | |
| babel.traverse(parsedConfig, { | |
| ExportDefaultDeclaration(path) { | |
| if ( | |
| isDefineConfigExpression(path.node.declaration) && | |
| isSafeToExtendWorkspace(path.node.declaration as CallExpression) | |
| ) { | |
| isValidVitestConfig = true; | |
| } | |
| }, | |
| }); | |
| if (!isValidVitestConfig) { | |
| reasons.push(`Found an invalid Vitest config file: ${vitestConfigFile}`); | |
| } | |
| } catch { | |
| reasons.push(`Error reading/parsing Vitest config file: ${vitestConfigFile}`); | |
| } | |
| } |
5 participants
We've had this TODO for a while. Basically, we no longer pull
dependencies from the context, so we don't need to mock it or populate
it anymore.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Ensure the config resolution/loading still works in a created project.
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.Greptile Summary
This PR refactors the dependency management system in Storybook's create-storybook module by removing contextual dependencies. The main change is in the
vitestConfigFilescheck, where the code previously validated that required dependencies (babel,findUp,fs) were present in a runtime context object before proceeding with configuration validation.The refactored implementation directly imports and uses these dependencies at the module level, eliminating the need for runtime context validation. This simplifies the code by removing 13 lines of dependency validation logic and associated error handling. The core functionality for validating Vitest workspace and configuration files remains unchanged - the system still parses configuration files using Babel, searches for workspace files using
findUp, and reads file contents using thefsmodule.The corresponding test file has been updated to use proper Vitest mocking (
vi.mock()) instead of the previous context-based mocking approach. ThemockContextis now empty since dependencies are no longer passed through it. This change aligns with modern JavaScript/TypeScript practices where dependencies are resolved at import time rather than being injected through context objects.Confidence score: 2/5
This PR has a significant issue that could cause runtime failures in production
The babel dependency is imported in the implementation but may not be properly mocked in tests, potentially causing test failures
The test file removes babel from the context but doesn't ensure babel parsing functionality is properly tested
Files that need more attention:
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tsSummary by CodeRabbit
Refactor
Tests