-
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 6 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,131 @@ | ||||||
| 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'; | ||||||
| const message = | ||||||
| status === 'pass' | ||||||
| ? 'Vendure config loaded and validated successfully' | ||||||
| : status === 'warn' | ||||||
| ? 'Config loaded with warnings' | ||||||
| : 'Plugin compatibility issues detected'; | ||||||
|
|
||||||
| 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; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * 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; | ||||||
|
|
||||||
| if (!config.plugins || config.plugins.length === 0) { | ||||||
| details.push('No plugins configured'); | ||||||
| return { details, hasIncompatible, hasNoCompat }; | ||||||
| } | ||||||
|
|
||||||
| details.push(`${config.plugins.length} plugin(s) loaded`); | ||||||
|
|
||||||
| for (const plugin of config.plugins) { | ||||||
| const pluginName = (plugin as any).name as string; | ||||||
| const compatibility = getCompatibility(plugin); | ||||||
|
|
||||||
| if (!compatibility) { | ||||||
| hasNoCompat = true; | ||||||
| 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 }; | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import { RuntimeVendureConfig } from '@vendure/core'; | ||
| import { DataSource } from 'typeorm'; | ||
|
|
||
| import { CheckResult } from '../types'; | ||
|
|
||
| /** | ||
| * Checks database connectivity by attempting to connect using the | ||
| * dbConnectionOptions from the loaded Vendure config. | ||
| * | ||
| * Currently only verifies that the database is reachable and accepts connections. | ||
| * This is a read-only check -- it uses safe overrides to ensure | ||
| * no schema changes, migrations, or data modifications occur. | ||
| * | ||
| * TODO: Future enhancements: | ||
| * - Detect pending migrations (compare migration table vs registered migrations) | ||
| * - Detect schema drift (use connection.driver.createSchemaBuilder().log() | ||
| * from the pattern in core/src/migrate.ts) | ||
| * Both would require passing entities and using the createConnection pattern | ||
| * from core/src/migrate.ts instead of an empty entity list. | ||
| */ | ||
| export async function runDatabaseCheck(config: RuntimeVendureConfig): Promise<CheckResult> { | ||
| const details: string[] = []; | ||
| const dbOptions = config.dbConnectionOptions; | ||
| const dbType = (dbOptions as any).type || 'unknown'; | ||
| let worstStatus: 'pass' | 'warn' | 'fail' = 'pass'; | ||
|
|
||
| // Check for synchronize: true (risky regardless of environment) | ||
| if ((dbOptions as any).synchronize) { | ||
| worstStatus = 'warn'; | ||
| details.push('Warning: synchronize is enabled (use migrations instead)'); | ||
| } | ||
|
|
||
| let dataSource: DataSource | undefined; | ||
| try { | ||
| // Connectivity check only -- entities are emptied to avoid TypeORM | ||
| // metadata validation errors (e.g. plugin entities that require | ||
| // NestJS module initialization to register their primary columns). | ||
| dataSource = new DataSource( | ||
| Object.assign({}, dbOptions, { | ||
| entities: [], | ||
| subscribers: [], | ||
| synchronize: false, | ||
| migrationsRun: false, | ||
| dropSchema: false, | ||
| logging: false, | ||
| }) as any, | ||
| ); | ||
|
|
||
| await dataSource.initialize(); | ||
|
|
||
| details.push(`Database type: ${dbType}`); | ||
| if ((dbOptions as any).host) { | ||
| details.push(`Host: ${(dbOptions as any).host}`); | ||
| } | ||
| if ((dbOptions as any).database) { | ||
| details.push(`Database: ${(dbOptions as any).database}`); | ||
| } | ||
|
|
||
| return { | ||
| name: 'Database', | ||
| status: worstStatus, | ||
| message: `Successfully connected to ${dbType} database`, | ||
| details, | ||
| }; | ||
| } catch (e: any) { | ||
| const errorMessage = e instanceof Error ? e.message : String(e); | ||
| details.push(`Database type: ${dbType}`); | ||
| details.push(`Error: ${errorMessage}`); | ||
|
|
||
| return { | ||
| name: 'Database', | ||
| status: 'warn', | ||
| message: `Could not connect to ${dbType} database`, | ||
| details, | ||
| }; | ||
| } finally { | ||
| if (dataSource?.isInitialized) { | ||
| await dataSource.destroy(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { runDependencyCheck } from './dependency-check'; | ||
|
|
||
| // Mock fs-extra | ||
| vi.mock('fs-extra', () => ({ | ||
| default: { | ||
| existsSync: vi.fn(), | ||
| readJsonSync: vi.fn(), | ||
| readFileSync: vi.fn(), | ||
| readdirSync: vi.fn(() => []), | ||
| }, | ||
| })); | ||
|
|
||
| import fs from 'fs-extra'; | ||
|
|
||
| describe('dependency-check', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('returns fail when node_modules does not exist', async () => { | ||
| vi.mocked(fs.existsSync).mockReturnValue(false); | ||
|
|
||
| const result = await runDependencyCheck('/fake/node_modules'); | ||
|
|
||
| expect(result.status).toBe('fail'); | ||
| expect(result.message).toContain('node_modules not found'); | ||
| }); | ||
|
|
||
| it('returns pass when all @vendure/* packages are same version', async () => { | ||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| vi.mocked(fs.readJsonSync).mockReturnValue({ version: '3.6.3' }); | ||
| vi.mocked(fs.readdirSync).mockReturnValue([]); | ||
| vi.mocked(fs.readFileSync).mockReturnValue(''); | ||
|
|
||
| const result = await runDependencyCheck('/fake/node_modules'); | ||
|
|
||
| expect(result.status).toBe('pass'); | ||
| expect(result.details?.some(d => d.includes('All @vendure/* packages at 3.6.3'))).toBe(true); | ||
| }); | ||
|
|
||
| it('returns fail when @vendure/* versions are mismatched', async () => { | ||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| vi.mocked(fs.readdirSync).mockReturnValue([]); | ||
| vi.mocked(fs.readFileSync).mockReturnValue(''); | ||
|
|
||
| let callCount = 0; | ||
| vi.mocked(fs.readJsonSync).mockImplementation(() => { | ||
| callCount++; | ||
| // Return different version for one package | ||
| if (callCount === 3) { | ||
| return { version: '3.6.2' }; | ||
| } | ||
| return { version: '3.6.3' }; | ||
| }); | ||
|
|
||
| const result = await runDependencyCheck('/fake/node_modules'); | ||
|
|
||
| expect(result.status).toBe('fail'); | ||
| expect(result.details?.some(d => d.includes('Mismatched'))).toBe(true); | ||
| }); | ||
|
|
||
| it('detects duplicate singleton packages', async () => { | ||
| vi.mocked(fs.existsSync).mockImplementation((p: any) => { | ||
| const pathStr = String(p); | ||
| // node_modules exists | ||
| if (pathStr === '/fake/node_modules') return true; | ||
| // @vendure dir exists | ||
| if (pathStr.includes('@vendure')) return true; | ||
| // root graphql exists | ||
| if (pathStr === '/fake/node_modules/graphql/package.json') return true; | ||
| // nested graphql exists | ||
| if (pathStr.includes('msw/node_modules/graphql/package.json')) return true; | ||
| return false; | ||
| }); | ||
| vi.mocked(fs.readdirSync).mockImplementation((p: any) => { | ||
| const pathStr = String(p); | ||
| if (pathStr === '/fake/node_modules') return ['msw'] as any; | ||
| return []; | ||
| }); | ||
| vi.mocked(fs.readJsonSync).mockImplementation((p: any) => { | ||
| const pathStr = String(p); | ||
| if (pathStr === '/fake/node_modules/graphql/package.json') { | ||
| return { version: '16.11.0' }; | ||
| } | ||
| if (pathStr.includes('msw/node_modules/graphql/package.json')) { | ||
| return { version: '16.14.0' }; | ||
| } | ||
| return { version: '3.6.3' }; | ||
| }); | ||
| vi.mocked(fs.readFileSync).mockReturnValue(''); | ||
|
|
||
| const result = await runDependencyCheck('/fake/node_modules'); | ||
|
|
||
| expect(result.status).toBe('fail'); | ||
| expect(result.details?.some(d => d.includes('Multiple graphql versions'))).toBe(true); | ||
| }); | ||
|
|
||
| it('returns pass with no duplicate singleton dependencies', async () => { | ||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| vi.mocked(fs.readJsonSync).mockReturnValue({ version: '3.6.3' }); | ||
| vi.mocked(fs.readdirSync).mockReturnValue([]); | ||
| vi.mocked(fs.readFileSync).mockReturnValue(''); | ||
|
|
||
| const result = await runDependencyCheck('/fake/node_modules'); | ||
|
|
||
| expect(result.details?.some(d => d.includes('No duplicate singleton dependencies'))).toBe(true); | ||
| }); | ||
| }); |
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