-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(cli): add vendure doctor command with project check #4777
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
base: minor
Are you sure you want to change the base?
Changes from all commits
4f5a4fd
e7b5368
fa20165
e773ffb
ac5c70b
d438056
67ab71f
2bc96c1
e087a04
487e5d0
2685ed4
0ed51a1
bc4b390
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| import { afterEach, describe, expect, it } from 'vitest'; | ||
|
|
||
| import { CliTestProject, createTestProject } from './cli-test-utils'; | ||
|
|
||
| describe('CLI Doctor Command E2E', () => { | ||
| let testProject: CliTestProject; | ||
|
|
||
| afterEach(() => { | ||
| if (testProject) { | ||
| testProject.cleanup(); | ||
| } | ||
| }); | ||
|
|
||
| describe('project check', () => { | ||
| it('should pass project check in a valid Vendure project', async () => { | ||
| testProject = createTestProject('doctor-project-pass'); | ||
|
|
||
| const result = await testProject.runCliCommand([ | ||
| 'doctor', | ||
| '--check', | ||
| 'project', | ||
| '--format', | ||
| 'json', | ||
| ]); | ||
|
|
||
| expect(result.exitCode).toBe(0); | ||
| const report = JSON.parse(result.stdout); | ||
| expect(report.overallStatus).toBe('passed'); | ||
| expect(report.checks).toHaveLength(1); | ||
| expect(report.checks[0].name).toBe('Project'); | ||
| expect(report.checks[0].status).toBe('pass'); | ||
| }); | ||
|
|
||
| it('should fail project check in a non-Vendure directory', async () => { | ||
| testProject = createTestProject('doctor-project-fail'); | ||
|
|
||
| // Overwrite package.json with no @vendure/* deps | ||
| testProject.writeFile( | ||
| 'package.json', | ||
| JSON.stringify({ | ||
| name: 'not-vendure', | ||
| version: '1.0.0', | ||
| dependencies: { express: '4.0.0' }, | ||
| }), | ||
| ); | ||
|
|
||
| const result = await testProject.runCliCommand( | ||
| ['doctor', '--check', 'project', '--format', 'json'], | ||
| { expectError: true }, | ||
| ); | ||
|
|
||
| expect(result.exitCode).toBe(1); | ||
| const report = JSON.parse(result.stdout); | ||
| expect(report.overallStatus).toBe('failed'); | ||
| expect(report.checks[0].status).toBe('fail'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('--format json', () => { | ||
| it('should output valid JSON with all expected fields', async () => { | ||
| testProject = createTestProject('doctor-json-output'); | ||
|
|
||
| const result = await testProject.runCliCommand([ | ||
| 'doctor', | ||
| '--check', | ||
| 'project', | ||
| '--format', | ||
| 'json', | ||
| ]); | ||
|
|
||
| expect(result.exitCode).toBe(0); | ||
| const report = JSON.parse(result.stdout); | ||
| expect(report).toHaveProperty('nodeVersion'); | ||
| expect(report).toHaveProperty('checks'); | ||
| expect(report).toHaveProperty('overallStatus'); | ||
| expect(report.nodeVersion).toMatch(/^v\d+\.\d+\.\d+$/); | ||
| }); | ||
| }); | ||
|
|
||
| describe('--strict mode', () => { | ||
| it('should treat warnings as failures with --strict', async () => { | ||
| testProject = createTestProject('doctor-strict'); | ||
|
|
||
| // Create a project with multiple lockfiles to trigger a warning | ||
| testProject.writeFile('yarn.lock', ''); | ||
| testProject.writeFile('package-lock.json', '{}'); | ||
|
|
||
| const result = await testProject.runCliCommand( | ||
| ['doctor', '--check', 'project', '--format', 'json', '--strict'], | ||
| { expectError: true }, | ||
| ); | ||
|
|
||
| // The project check itself passes but the multiple lockfiles | ||
| // don't cause a warn status on the project check -- they're just | ||
| // informational details. So strict mode won't change the outcome here. | ||
| // This test verifies the --strict flag is accepted without error. | ||
| const report = JSON.parse(result.stdout); | ||
| expect(report).toHaveProperty('overallStatus'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('--help', () => { | ||
| it('should show doctor help with all options', async () => { | ||
| testProject = createTestProject('doctor-help'); | ||
|
|
||
| const result = await testProject.runCliCommand(['doctor', '--help']); | ||
|
|
||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout).toContain('--config'); | ||
| expect(result.stdout).toContain('--check'); | ||
| expect(result.stdout).toContain('--profile'); | ||
| expect(result.stdout).toContain('--format'); | ||
| expect(result.stdout).toContain('--strict'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('dependency check', () => { | ||
| it('should report when node_modules is missing', async () => { | ||
| testProject = createTestProject('doctor-no-modules'); | ||
|
|
||
| // The default test project doesn't run npm install, | ||
| // so node_modules won't exist | ||
| const result = await testProject.runCliCommand( | ||
| ['doctor', '--check', 'dependencies', '--format', 'json'], | ||
| { expectError: true }, | ||
| ); | ||
|
|
||
| expect(result.exitCode).toBe(1); | ||
| const report = JSON.parse(result.stdout); | ||
| expect(report.checks[0].status).toBe('fail'); | ||
| expect(report.checks[0].message).toContain('node_modules not found'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('cascading skips', () => { | ||
| it('should skip config-dependent checks when project check fails', async () => { | ||
| testProject = createTestProject('doctor-cascade-skip'); | ||
|
|
||
| // Overwrite package.json with no vendure deps | ||
| testProject.writeFile( | ||
| 'package.json', | ||
| JSON.stringify({ | ||
| name: 'not-vendure', | ||
| version: '1.0.0', | ||
| dependencies: { express: '4.0.0' }, | ||
| }), | ||
| ); | ||
|
|
||
| const result = await testProject.runCliCommand( | ||
| ['doctor', '--format', 'json'], | ||
| { expectError: true }, | ||
| ); | ||
|
|
||
| expect(result.exitCode).toBe(1); | ||
| const report = JSON.parse(result.stdout); | ||
|
|
||
| // Project should fail | ||
| expect(report.checks[0].name).toBe('Project'); | ||
| expect(report.checks[0].status).toBe('fail'); | ||
|
|
||
| // All other checks should be skipped | ||
| const skippedChecks = report.checks.filter((c: any) => c.status === 'skip'); | ||
| expect(skippedChecks.length).toBeGreaterThanOrEqual(4); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,157 @@ | ||||||
| import { | ||||||
| getCompatibility, | ||||||
| preBootstrapConfig, | ||||||
| resetConfig, | ||||||
| RuntimeVendureConfig, | ||||||
| VENDURE_VERSION, | ||||||
| } from '@vendure/core'; | ||||||
| import { satisfies } from 'semver'; | ||||||
|
|
||||||
| import { loadVendureConfigFile } from '../../../shared/load-vendure-config-file'; | ||||||
| import { analyzeProject } from '../../../shared/shared-prompts'; | ||||||
| import { VendureConfigRef } from '../../../shared/vendure-config-ref'; | ||||||
| import { CheckResult } from '../types'; | ||||||
|
|
||||||
| export interface ConfigCheckResult { | ||||||
| check: CheckResult; | ||||||
| /** The loaded runtime config, available for subsequent checks (schema, db, production). */ | ||||||
| config?: RuntimeVendureConfig; | ||||||
| /** The Vendure version detected from @vendure/core. */ | ||||||
| vendureVersion?: string; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Loads the Vendure config, runs preBootstrapConfig() to validate custom fields, | ||||||
| * register entities, and run plugin configuration hooks. Then checks plugin | ||||||
| * compatibility with the current Vendure version. | ||||||
| */ | ||||||
| export async function runConfigCheck(configFlag?: string): Promise<ConfigCheckResult> { | ||||||
| const details: string[] = []; | ||||||
| let runtimeConfig: RuntimeVendureConfig | undefined; | ||||||
|
|
||||||
| try { | ||||||
| resetConfig(); | ||||||
| process.env.VENDURE_RUNNING_IN_CLI = 'true'; | ||||||
|
|
||||||
| // 1. Analyze the project (finds tsconfig, creates ts-morph Project) | ||||||
| const { project, vendureTsConfig } = await analyzeProject({ | ||||||
| cancelledMessage: '', | ||||||
| config: configFlag, | ||||||
| }); | ||||||
|
|
||||||
| // 2. Find the VendureConfig source file | ||||||
| const vendureConfigRef = new VendureConfigRef(project, configFlag); | ||||||
| const configPath = vendureConfigRef.getPathRelativeToProjectRoot(); | ||||||
| details.push(`Config loaded from ${configPath}`); | ||||||
|
|
||||||
| // 3. Load the config at runtime (ts-node, path mappings, dotenv) | ||||||
| const config = await loadVendureConfigFile(vendureConfigRef, vendureTsConfig); | ||||||
|
|
||||||
| // 4. Run preBootstrapConfig() -- validates custom fields, registers entities, | ||||||
| // runs plugin configuration() hooks, sets strategies | ||||||
| runtimeConfig = await preBootstrapConfig(config); | ||||||
| details.push('Custom fields validated'); | ||||||
| details.push('Plugin configuration completed'); | ||||||
|
|
||||||
| // 5. Check plugin compatibility | ||||||
| const pluginResults = checkPlugins(runtimeConfig); | ||||||
| details.push(...pluginResults.details); | ||||||
|
|
||||||
| const status = pluginResults.hasIncompatible ? 'fail' : pluginResults.hasNoCompat ? 'warn' : 'pass'; | ||||||
| let message: string; | ||||||
| if (status === 'pass') { | ||||||
| message = 'Vendure config loaded and validated successfully'; | ||||||
| } else if (status === 'fail') { | ||||||
| message = 'Plugin compatibility issues detected'; | ||||||
| } else { | ||||||
| // warn -- explain why | ||||||
| const parts: string[] = []; | ||||||
| if (pluginResults.noCompatCount > 0) { | ||||||
| parts.push(`${pluginResults.noCompatCount} plugin(s) without compatibility range`); | ||||||
| } | ||||||
| message = `Config loaded (${parts.join(', ')})`; | ||||||
| } | ||||||
|
|
||||||
| return { | ||||||
| check: { name: 'Config', status, message, details }, | ||||||
| config: runtimeConfig, | ||||||
| vendureVersion: VENDURE_VERSION, | ||||||
| }; | ||||||
| } catch (e: any) { | ||||||
| const errorMessage = e instanceof Error ? e.message : String(e); | ||||||
| details.push(`Error: ${errorMessage}`); | ||||||
| return { | ||||||
| check: { | ||||||
| name: 'Config', | ||||||
| status: 'fail', | ||||||
| message: 'Failed to load Vendure config', | ||||||
| details, | ||||||
| }, | ||||||
| config: runtimeConfig, | ||||||
| vendureVersion: VENDURE_VERSION, | ||||||
| }; | ||||||
| } finally { | ||||||
| process.env.VENDURE_RUNNING_IN_CLI = undefined; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify downstream impact by locating all reads/writes of this env variable.
rg -n -C2 'VENDURE_RUNNING_IN_CLI' packages/cli/srcRepository: vendurehq/vendure Length of output: 4114 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find where the env flag is interpreted
rg -n "isRunningFromVendureCli|VENDURE_RUNNING_IN_CLI" packages | head -n 200
# Show the implementation around isRunningFromVendureCli (if present)
rg -n "function isRunningFromVendureCli|isRunningFromVendureCli\s*=" packages -SRepository: vendurehq/vendure Length of output: 1664 🏁 Script executed: #!/bin/bash
node - <<'NODE'
const key = 'VENDURE_RUNNING_IN_CLI';
console.log('initial:', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
process.env[key] = 'true';
console.log('after "true":', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
process.env[key] = undefined;
console.log('after = undefined:', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
delete process.env[key];
console.log('after delete:', Object.prototype.hasOwnProperty.call(process.env, key), JSON.stringify(process.env[key]));
NODERepository: vendurehq/vendure Length of output: 178 Use On Line 87, setting Proposed fix- process.env.VENDURE_RUNNING_IN_CLI = undefined;
+ delete process.env.VENDURE_RUNNING_IN_CLI;Apply the same cleanup anywhere else this env var is cleared with 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing schema.ts and migrate.ts both use = undefined across 4 occurrences. Changing only our code would be inconsistent. The behavior is equivalent here since the check is === 'true'. Happy to address this repo-wide in a separate PR if desired.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The consistency argument also holds; introducing A repo-wide cleanup PR would be the right place to standardize this. Would you like me to open a follow-up issue to track that? 🧠 Learnings used
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I double-checked and isRunningFromVendureCli() in core/src/migrate.ts:256 uses != null (loose inequality), not === 'true' as I initially stated. So = undefined does coerce to the string "undefined" which satisfies != null, meaning the flag technically leaks. In practice it doesn't cause a bug since the doctor command exits after running and no migration code shares the same process.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You're right that it's benign here because the Would you like me to open a follow-up issue to track a repo-wide cleanup (replacing ✏️ Learnings added
🧠 Learnings used |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| interface PluginCheckResult { | ||||||
| details: string[]; | ||||||
| hasIncompatible: boolean; | ||||||
| hasNoCompat: boolean; | ||||||
| noCompatCount: number; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Checks each plugin's compatibility range against the current Vendure version. | ||||||
| * Reports results per-plugin instead of throwing on the first incompatible one. | ||||||
| */ | ||||||
| function checkPlugins(config: RuntimeVendureConfig): PluginCheckResult { | ||||||
| const details: string[] = []; | ||||||
| let hasIncompatible = false; | ||||||
| let hasNoCompat = false; | ||||||
| let noCompatCount = 0; | ||||||
|
|
||||||
| if (!config.plugins || config.plugins.length === 0) { | ||||||
| details.push('No plugins configured'); | ||||||
| return { details, hasIncompatible, hasNoCompat, noCompatCount }; | ||||||
| } | ||||||
|
|
||||||
| details.push(`${config.plugins.length} plugin(s) loaded`); | ||||||
|
|
||||||
| for (const plugin of config.plugins) { | ||||||
| // DynamicModule plugins (e.g. SomePlugin.init()) have the class on .module | ||||||
| const pluginName = getPluginName(plugin); | ||||||
| const compatibility = getCompatibility(plugin); | ||||||
|
|
||||||
| if (!compatibility) { | ||||||
| hasNoCompat = true; | ||||||
| noCompatCount++; | ||||||
| details.push(`Plugin "${pluginName}": no compatibility range specified`); | ||||||
| } else if ( | ||||||
| !satisfies(VENDURE_VERSION, compatibility, { loose: true, includePrerelease: true }) | ||||||
| ) { | ||||||
| hasIncompatible = true; | ||||||
| details.push( | ||||||
| `Plugin "${pluginName}": incompatible (requires ${compatibility}, running ${VENDURE_VERSION})`, | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return { details, hasIncompatible, hasNoCompat, noCompatCount }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Extracts the plugin name, handling both plain classes and DynamicModule objects. | ||||||
| */ | ||||||
| function getPluginName(plugin: any): string { | ||||||
| // DynamicModule has { module: Type<any>, ... } | ||||||
| if (plugin && plugin.module && plugin.module.name) { | ||||||
| return plugin.module.name; | ||||||
| } | ||||||
| // Plain class | ||||||
| if (plugin && plugin.name) { | ||||||
| return plugin.name; | ||||||
| } | ||||||
| return 'Unknown'; | ||||||
| } | ||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
Avoid forcing success exit code for
doctorcommand.Hard-coding
process.exit(0)can suppress diagnostic failure signaling (notably--strictCI use-cases) when the command intends to report non-zero outcomes.Suggested change
action: async options => { const { doctorCommand } = await import('./doctor/doctor'); await doctorCommand(options); - process.exit(0); },📝 Committable suggestion
🤖 Prompt for AI Agents
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.
This is the standard pattern across ALL 4 existing commands (add, migrate, codemod, schema). doctorCommand() calls process.exit(1) internally before this line is reached on failure
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.
@Ryrahul, you're right — I missed thatdoctorCommandalready callsprocess.exit(1)internally whenoverallStatusis'failed', soprocess.exit(0)here is only ever reached on success. This is indeed consistent with theadd,migrate,codemod, andschemacommands. My earlier comment was mistaken. No change needed here.🧠 Learnings used