diff --git a/packages/app/src/cli/commands/app/config/validate.ts b/packages/app/src/cli/commands/app/config/validate.ts index c597c5228e..b6ddd0c9ed 100644 --- a/packages/app/src/cli/commands/app/config/validate.ts +++ b/packages/app/src/cli/commands/app/config/validate.ts @@ -3,6 +3,8 @@ import {validateApp} from '../../../services/validate.js' import AppLinkedCommand, {AppLinkedCommandOutput} from '../../../utilities/app-linked-command.js' import {linkedAppContext} from '../../../services/app-context.js' import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' +import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' +import {outputResult, stringifyMessage, unstyled} from '@shopify/cli-kit/node/output' export default class Validate extends AppLinkedCommand { static summary = 'Validate your app configuration and extensions.' @@ -20,13 +22,24 @@ export default class Validate extends AppLinkedCommand { public async run(): Promise { const {flags} = await this.parse(Validate) - const {app} = await linkedAppContext({ - directory: flags.path, - clientId: flags['client-id'], - forceRelink: flags.reset, - userProvidedConfigName: flags.config, - unsafeTolerateErrors: true, - }) + let app + try { + const context = await linkedAppContext({ + directory: flags.path, + clientId: flags['client-id'], + forceRelink: flags.reset, + userProvidedConfigName: flags.config, + unsafeTolerateErrors: true, + }) + app = context.app + } catch (err) { + if (err instanceof AbortError && flags.json) { + const message = unstyled(stringifyMessage(err.message)).trim() + outputResult(JSON.stringify({valid: false, errors: [{message}]}, null, 2)) + throw new AbortSilentError() + } + throw err + } await validateApp(app, {json: flags.json}) diff --git a/packages/app/src/cli/models/app/error-parsing.test.ts b/packages/app/src/cli/models/app/error-parsing.test.ts index 85986f5088..410adf0b5e 100644 --- a/packages/app/src/cli/models/app/error-parsing.test.ts +++ b/packages/app/src/cli/models/app/error-parsing.test.ts @@ -1,137 +1,27 @@ -import {parseHumanReadableError} from './error-parsing.js' +import {parseStructuredErrors} from './error-parsing.js' import {describe, expect, test} from 'vitest' -describe('parseHumanReadableError', () => { - test('formats union errors with smart variant detection', () => { - const unionErrorObject = [ - { - code: 'invalid_union', - unionErrors: [ - { - issues: [ - { - code: 'invalid_type', - expected: 'array', - received: 'number', - path: ['web', 'roles'], - message: 'Expected array, received number', - }, - { - code: 'invalid_type', - expected: 'string', - received: 'undefined', - path: ['web', 'commands', 'build'], - message: 'Required', - }, - ], - name: 'ZodError', - }, - { - issues: [ - { - code: 'invalid_literal', - expected: "'frontend'", - received: 'number', - path: ['web', 'type'], - message: "Invalid literal value, expected 'frontend'", - }, - ], - name: 'ZodError', - }, - ], - path: ['web'], - message: 'Invalid input', - }, +describe('parseStructuredErrors', () => { + test('converts regular issues to structured format', () => { + const issues = [ + {path: ['name'], message: 'Required', code: 'invalid_type'}, + {path: ['version'], message: 'Must be a string', code: 'invalid_type'}, ] - const result = parseHumanReadableError(unionErrorObject) - - // Verify the enhanced format shows only the best matching variant's errors - // (Option 1 has both missing field + type error, so it's likely the intended one) - expect(result).toContain('[web.roles]: Expected array, received number') - expect(result).toContain('[web.commands.build]: Required') - - // Should NOT show confusing union variant breakdown - expect(result).not.toContain('Union validation failed') - expect(result).not.toContain('Option 1:') - expect(result).not.toContain('Option 2:') + const result = parseStructuredErrors(issues) - // Should NOT show errors from the less likely option 2 - expect(result).not.toContain("[web.type]: Invalid literal value, expected 'frontend'") + expect(result).toEqual([ + {path: ['name'], message: 'Required', code: 'invalid_type'}, + {path: ['version'], message: 'Must be a string', code: 'invalid_type'}, + ]) }) - test('handles regular non-union errors', () => { - const regularErrorObject = [ - { - path: ['name'], - message: 'Required field is missing', - }, - { - path: ['version'], - message: 'Must be a valid semver string', - }, - ] - - const result = parseHumanReadableError(regularErrorObject) - - // Verify regular errors still work as expected - expect(result).toBe('• [name]: Required field is missing\n• [version]: Must be a valid semver string\n') - expect(result).not.toContain('Union validation failed') - }) - - test('handles edge cases for union error detection', () => { - // Test case 1: Union error with no unionErrors array - const noUnionErrors = [ - { - code: 'invalid_union', - path: ['root'], - message: 'Invalid input', - }, - ] - - const result1 = parseHumanReadableError(noUnionErrors) - expect(result1).toBe('• [root]: Invalid input\n') - - // Test case 2: Union error with empty unionErrors array - should fall back to showing full union error - const emptyUnionErrors = [ - { - code: 'invalid_union', - unionErrors: [], - path: ['root'], - message: 'Invalid input', - }, - ] - - const result2 = parseHumanReadableError(emptyUnionErrors) - expect(result2).toContain("Configuration doesn't match any expected format:") - - // Test case 3: Union errors with variants that have no issues - results in empty string - const noIssuesVariants = [ - { - code: 'invalid_union', - unionErrors: [ - {issues: [], name: 'ZodError'}, - {issues: [], name: 'ZodError'}, - ], - path: ['root'], - message: 'Invalid input', - }, - ] - - const result3 = parseHumanReadableError(noIssuesVariants) - // When all variants have no issues, the best variant selection returns null issues - // resulting in no output, which falls back to the union error display - expect(result3).toContain("Configuration doesn't match any expected format:") - }) - - test('findBestMatchingVariant scoring logic works correctly', () => { - // Test various scoring scenarios by creating mock union errors - const scenarioWithMissingFields = [ + test('selects best union variant based on scoring', () => { + const issues = [ { code: 'invalid_union', unionErrors: [ { - // Variant with missing fields - should score highest issues: [ {path: ['supports_moto'], message: 'Required'}, {path: ['merchant_label'], message: 'Required'}, @@ -139,99 +29,63 @@ describe('parseHumanReadableError', () => { name: 'ZodError', }, { - // Variant with only type errors - should score lower issues: [ {path: ['type'], message: 'Expected string, received number'}, {path: ['id'], message: 'Expected number, received string'}, ], name: 'ZodError', }, - { - // Variant with other errors - should score lowest - issues: [{path: ['url'], message: 'Invalid URL format'}], - name: 'ZodError', - }, ], path: [], message: 'Invalid input', }, ] - const result = parseHumanReadableError(scenarioWithMissingFields) + const result = parseStructuredErrors(issues) - // Should show only the variant with missing fields (highest score) - expect(result).toContain('[supports_moto]: Required') - expect(result).toContain('[merchant_label]: Required') - - // Should NOT show errors from other variants - expect(result).not.toContain('Expected string, received number') - expect(result).not.toContain('Invalid URL format') - expect(result).not.toContain('Union validation failed') + expect(result).toEqual([ + {path: ['supports_moto'], message: 'Required', code: 'invalid_union'}, + {path: ['merchant_label'], message: 'Required', code: 'invalid_union'}, + ]) }) - test('handles undefined messages gracefully', () => { - const undefinedMessageError = [ - { - path: ['field'], - message: undefined, - }, + test('falls back when all union variants have empty issues', () => { + const issues = [ { - path: [], - // message is completely missing + code: 'invalid_union', + unionErrors: [ + {issues: [], name: 'ZodError'}, + {issues: [], name: 'ZodError'}, + ], + path: ['root'], + message: 'Invalid input', }, ] - const result = parseHumanReadableError(undefinedMessageError) + const result = parseStructuredErrors(issues) - expect(result).toBe('• [field]: Unknown error\n• [root]: Unknown error\n') + expect(result).toEqual([{path: ['root'], message: 'Invalid input', code: 'invalid_union'}]) }) - test('handles mixed scoring scenarios', () => { - // Test scenario where we need to pick between variants with different error combinations - const mixedScenario = [ + test('falls back when union has no unionErrors array', () => { + const issues = [ { code: 'invalid_union', - unionErrors: [ - { - // Mix of missing fields and type errors - this should win due to missing fields - issues: [ - {path: ['required_field'], message: 'Required'}, - {path: ['wrong_type'], message: 'Expected string, received number'}, - ], - name: 'ZodError', - }, - { - // Only type errors - should lose - issues: [ - {path: ['field1'], message: 'Expected boolean, received string'}, - {path: ['field2'], message: 'Expected array, received object'}, - {path: ['field3'], message: 'Expected number, received string'}, - ], - name: 'ZodError', - }, - { - // Only other validation errors - should score lowest - issues: [ - {path: ['url'], message: 'Must be valid URL'}, - {path: ['email'], message: 'Must be valid email'}, - ], - name: 'ZodError', - }, - ], - path: [], + path: ['field'], message: 'Invalid input', }, ] - const result = parseHumanReadableError(mixedScenario) + const result = parseStructuredErrors(issues) + + expect(result).toEqual([{path: ['field'], message: 'Invalid input', code: 'invalid_union'}]) + }) + + test('handles missing message with fallback', () => { + const issues = [{path: ['x'], code: 'custom'}] - // Should pick the variant with missing field (even though it has fewer total errors) - expect(result).toContain('[required_field]: Required') - expect(result).toContain('[wrong_type]: Expected string, received number') + const result = parseStructuredErrors(issues) - // Should not show errors from other variants - expect(result).not.toContain('Expected boolean, received string') - expect(result).not.toContain('Must be valid URL') - expect(result).not.toContain('Union validation failed') + expect(result).toEqual([{path: ['x'], message: 'Unknown error', code: 'custom'}]) }) }) diff --git a/packages/app/src/cli/models/app/error-parsing.ts b/packages/app/src/cli/models/app/error-parsing.ts index 96b3c81e28..125a6f4938 100644 --- a/packages/app/src/cli/models/app/error-parsing.ts +++ b/packages/app/src/cli/models/app/error-parsing.ts @@ -1,3 +1,9 @@ +interface ParsedIssue { + path?: (string | number)[] + message: string + code?: string +} + interface UnionError { issues?: {path?: (string | number)[]; message: string}[] name: string @@ -56,44 +62,25 @@ function findBestMatchingVariant(unionErrors: UnionError[]): UnionError | null { return bestVariant } -/** - * Formats an issue into a human-readable error line - */ -function formatErrorLine(issue: {path?: (string | number)[]; message?: string}, indent = '') { - const path = issue.path && issue.path.length > 0 ? issue.path.map(String).join('.') : 'root' - const message = issue.message ?? 'Unknown error' - return `${indent}• [${path}]: ${message}\n` -} - -export function parseHumanReadableError(issues: ExtendedZodIssue[]) { - let humanReadableError = '' - - issues.forEach((issue) => { - // Handle union errors with smart variant detection +export function parseStructuredErrors(issues: ExtendedZodIssue[]): ParsedIssue[] { + const result: ParsedIssue[] = [] + for (const issue of issues) { if (issue.code === 'invalid_union' && issue.unionErrors) { - // Find the variant that's most likely the intended one const bestVariant = findBestMatchingVariant(issue.unionErrors) - if (bestVariant?.issues?.length) { - // Show errors only for the best matching variant - bestVariant.issues.forEach((nestedIssue) => { - humanReadableError += formatErrorLine(nestedIssue) - }) + for (const nested of bestVariant.issues) { + result.push({path: nested.path, message: nested.message, code: issue.code}) + } } else { - // Fallback: show all variants if we can't determine the best one - humanReadableError += `• Configuration doesn't match any expected format:\n` - issue.unionErrors.forEach((unionError, index: number) => { - humanReadableError += ` Option ${index + 1}:\n` - unionError.issues?.forEach((nestedIssue) => { - humanReadableError += formatErrorLine(nestedIssue, ' ') - }) + result.push({ + path: issue.path, + message: issue.message ?? "Configuration doesn't match any expected format", + code: issue.code, }) } } else { - // Handle regular issues - humanReadableError += formatErrorLine(issue) + result.push({path: issue.path, message: issue.message ?? 'Unknown error', code: issue.code}) } - }) - - return humanReadableError + } + return result } diff --git a/packages/app/src/cli/models/app/loader.test.ts b/packages/app/src/cli/models/app/loader.test.ts index 455f73c608..4f35ca534b 100644 --- a/packages/app/src/cli/models/app/loader.test.ts +++ b/packages/app/src/cli/models/app/loader.test.ts @@ -8,7 +8,6 @@ import { getAppConfigurationContext, loadConfigForAppCreation, reloadApp, - ConfigurationError, } from './loader.js' import {App, AppInterface, AppLinkedInterface, AppSchema, WebConfigurationSchema} from './app.js' import {DEFAULT_CONFIG, buildVersionedAppSchema, getWebhookConfig} from './app.test-data.js' @@ -2767,45 +2766,38 @@ describe('parseConfigurationObject', () => { }, ] const {schema} = await buildVersionedAppSchema() - expect(() => parseConfigurationObject(schema, 'tmp', configurationObject)).toThrow(ConfigurationError) + const result = parseConfigurationObject(schema, 'tmp', configurationObject) + expect(result.errors).toBeDefined() }) - test('throws an error when client_id is missing in app schema TOML file', async () => { + test('returns errors when client_id is missing in app schema TOML file', async () => { const configurationObject = { scopes: [], } - expect(() => parseConfigurationObject(AppSchema, 'tmp', configurationObject)).toThrow(ConfigurationError) - expect(() => parseConfigurationObject(AppSchema, 'tmp', configurationObject)).toThrow('[client_id]: Required') + const result = parseConfigurationObject(AppSchema, 'tmp', configurationObject) + expect(result.errors).toBeDefined() + expect(result.errors!.some((err) => err.message === 'Required' && err.path?.includes('client_id'))).toBe(true) }) - test('throws an error if fields are missing in a frontend config web TOML file', async () => { + test('returns errors if fields are missing in a frontend config web TOML file', async () => { const configurationObject = { type: 11, commands: {dev: ''}, roles: 1, } - let thrown: ConfigurationError | undefined - try { - parseConfigurationObject(WebConfigurationSchema, 'tmp', configurationObject) - expect.fail('Expected ConfigurationError to be thrown') - } catch (err) { - if (err instanceof ConfigurationError) { - thrown = err - } else { - throw err - } - } + const result = parseConfigurationObject(WebConfigurationSchema, 'tmp', configurationObject) + expect(result.errors).toBeDefined() // The enhanced union handling should show only the most relevant errors - // instead of showing all variants, making it much more user-friendly - expect(thrown.message).toContain('[roles]: Expected array, received number') + const messages = result.errors!.map((err) => err.message) + expect(messages.some((msg) => msg.includes('Expected array, received number'))).toBe(true) - // Should NOT show the confusing union variant breakdown - expect(thrown.message).not.toContain('Union validation failed') - expect(thrown.message).not.toContain('Option 1:') - expect(thrown.message).not.toContain('Option 2:') + // Should NOT show confusing union variant breakdown + expect(messages.join('\n')).not.toContain('Union validation failed') + expect(messages.join('\n')).not.toContain('Option 1:') + expect(messages.join('\n')).not.toContain('Option 2:') }) }) @@ -3327,13 +3319,11 @@ describe('WebhooksSchema', () => { async function setupParsing(errorObj: zod.ZodIssue | {}, webhookConfigOverrides: WebhooksConfig) { const toParse = getWebhookConfig(webhookConfigOverrides) - try { - const parsedConfiguration = parseConfigurationObject(WebhooksSchema, 'tmp', toParse) - return {threw: false as const, parsedConfiguration, error: undefined} - } catch (err) { - if (!(err instanceof ConfigurationError)) throw err - return {threw: true as const, parsedConfiguration: undefined as any, error: err} + const result = parseConfigurationObject(WebhooksSchema, 'tmp', toParse) + if (result.errors) { + return {threw: true as const, parsedConfiguration: undefined as any, error: result.errors} } + return {threw: false as const, parsedConfiguration: result.data, error: undefined} } }) diff --git a/packages/app/src/cli/models/app/loader.ts b/packages/app/src/cli/models/app/loader.ts index 381cf75404..cf7ed881ce 100644 --- a/packages/app/src/cli/models/app/loader.ts +++ b/packages/app/src/cli/models/app/loader.ts @@ -12,7 +12,7 @@ import { SchemaForConfig, AppLinkedInterface, } from './app.js' -import {parseHumanReadableError} from './error-parsing.js' +import {parseStructuredErrors} from './error-parsing.js' import { getAppConfigurationFileName, getAppConfigurationShorthand, @@ -46,7 +46,7 @@ import {hashString} from '@shopify/cli-kit/node/crypto' import {JsonMapType} from '@shopify/cli-kit/node/toml' import {joinPath, dirname, basename, relativePath, relativizePath} from '@shopify/cli-kit/node/path' import {AbortError} from '@shopify/cli-kit/node/error' -import {outputContent, outputDebug, OutputMessage, outputToken} from '@shopify/cli-kit/node/output' +import {outputContent, outputDebug, outputToken, stringifyMessage} from '@shopify/cli-kit/node/output' import {joinWithAnd} from '@shopify/cli-kit/common/string' import {getArrayRejectingUndefined} from '@shopify/cli-kit/common/array' import {showNotificationsIfNeeded} from '@shopify/cli-kit/node/notifications-system' @@ -66,25 +66,30 @@ interface ReloadState { previousDevURLs?: ApplicationURLs } -export class ConfigurationError extends AbortError { - constructor( - message: OutputMessage, - public readonly configurationPath: string, - ) { - super(message) +export interface ConfigurationError { + file: string + message: string + path?: (string | number)[] + code?: string +} + +export function formatConfigurationError(error: ConfigurationError): string { + if (error.path?.length) { + return `[${error.path.join('.')}]: ${error.message}` } + return error.message } +type ConfigurationResult = {data: T; errors?: never} | {data?: never; errors: ConfigurationError[]} + /** - * Loads a configuration file, validates it against a schema, and returns the parsed object. - * - * Throws `ConfigurationError` if the file is missing or invalid. + * Loads a configuration file, validates it against a schema, and returns a result. */ export async function parseConfigurationFile( schema: TSchema, filepath: string, preloadedContent?: JsonMapType, -): Promise> { +): Promise>> { let content = preloadedContent if (!content) { try { @@ -92,13 +97,10 @@ export async function parseConfigurationFile( content = file.content } catch (err) { if (err instanceof TomlFileNotFoundError) { - throw new ConfigurationError( - outputContent`Couldn't find the configuration file at ${outputToken.path(filepath)}`, - filepath, - ) + return {errors: [{file: filepath, message: `Couldn't find the configuration file at ${filepath}`}]} } if (err instanceof TomlParseError) { - throw new ConfigurationError(outputContent`${err.message}`, filepath) + return {errors: [{file: filepath, message: err.message}]} } throw err } @@ -107,72 +109,70 @@ export async function parseConfigurationFile( } /** - * Parses a configuration object using a schema, and returns the parsed object. - * - * Throws `ConfigurationError` if the object is invalid. + * Parses a configuration object using a schema, and returns a result. */ export function parseConfigurationObject( schema: TSchema, filepath: string, configurationObject: unknown, -): zod.TypeOf { +): ConfigurationResult> { const parseResult = schema.safeParse(configurationObject) if (!parseResult.success) { - throw new ConfigurationError( - outputContent`\n${outputToken.errorText('Validation errors')} in ${outputToken.path( - filepath, - )}:\n\n${parseHumanReadableError(parseResult.error.issues)}`, - filepath, - ) + return { + errors: parseStructuredErrors(parseResult.error.issues).map((issue) => ({ + file: filepath, + message: issue.message, + path: issue.path, + code: issue.code, + })), + } } - return parseResult.data + return {data: parseResult.data} } /** - * Parses a configuration object using a specification's schema, and returns the parsed object. - * - * Throws `ConfigurationError` if the object is invalid. + * Parses a configuration object using a specification's schema, and returns a result. */ export function parseConfigurationObjectAgainstSpecification( spec: ExtensionSpecification, filepath: string, configurationObject: object, -): zod.TypeOf { +): ConfigurationResult> { const parsed = spec.parseConfigurationObject(configurationObject) switch (parsed.state) { case 'ok': { - return parsed.data + return {data: parsed.data} } case 'error': { - throw new ConfigurationError( - outputContent`App configuration is not valid\nValidation errors in ${outputToken.path( - filepath, - )}:\n\n${parseHumanReadableError(parsed.errors)}`, - filepath, - ) + return { + errors: parsed.errors.map((err) => ({ + file: filepath, + message: err.message ?? 'Unknown error', + path: err.path, + })), + } } } } export class AppErrors { - private errors: { - [key: string]: OutputMessage - } = {} + private readonly errors: ConfigurationError[] = [] - addError(path: string, message: OutputMessage): void { - this.errors[path] = message + addError(error: ConfigurationError): void { + this.errors.push(error) } - getError(path: string) { - return this.errors[path] + addErrors(errors: ConfigurationError[]): void { + this.errors.push(...errors) } - isEmpty() { - return Object.keys(this.errors).length === 0 + getErrors(file?: string): ConfigurationError[] { + if (file) return this.errors.filter((err) => err.file === file) + return [...this.errors] } - toJSON(): OutputMessage[] { - return Object.values(this.errors) + isEmpty(): boolean { + return this.errors.length === 0 } } @@ -200,7 +200,12 @@ export async function loadConfigForAppCreation(directory: string, name: string): const {project, activeConfig} = await getAppConfigurationContext(directory) const rawConfig = activeConfig.file.content const webFiles = webFilesForConfig(project, activeConfig.file) - const webs = await Promise.all(webFiles.map((wf) => loadSingleWeb(wf.path, wf.content))) + const webResults = await Promise.all(webFiles.map((wf) => loadSingleWeb(wf.path, wf.content))) + const webErrors = webResults.flatMap((result) => result.errors ?? []) + if (webErrors.length > 0) { + throw new AbortError(webErrors.map(formatConfigurationError).join('\n')) + } + const webs = webResults.filter((result): result is {web: Web} => 'web' in result).map((result) => result.web) const isLaunchable = webs.some((web) => isWebType(web, WebType.Frontend) || isWebType(web, WebType.Backend)) const scopesArray = getAppScopesArray(rawConfig as CurrentAppConfiguration) @@ -215,15 +220,22 @@ export async function loadConfigForAppCreation(directory: string, name: string): } } -async function loadSingleWeb(webConfigPath: string, preloadedContent?: JsonMapType): Promise { - const config = await parseConfigurationFile(WebConfigurationSchema, webConfigPath, preloadedContent) +async function loadSingleWeb( + webConfigPath: string, + preloadedContent?: JsonMapType, +): Promise<{web: Web; errors?: never} | {web?: never; errors: ConfigurationError[]}> { + const result = await parseConfigurationFile(WebConfigurationSchema, webConfigPath, preloadedContent) + if (result.errors) return {errors: result.errors} + const config = result.data const roles = new Set('roles' in config ? config.roles : []) if ('type' in config) roles.add(config.type) const {type, ...processedWebConfiguration} = {...config, roles: Array.from(roles), type: undefined} return { - directory: dirname(webConfigPath), - configuration: processedWebConfiguration, - framework: await resolveFramework(dirname(webConfigPath)), + web: { + directory: dirname(webConfigPath), + configuration: processedWebConfiguration, + framework: await resolveFramework(dirname(webConfigPath)), + }, } } @@ -283,7 +295,12 @@ export async function loadAppFromContext file.path) - const webResults = await Promise.all( - webFiles.map(async (webFile) => { - try { - return await loadSingleWeb(webFile.path, webFile.content) - } catch (err) { - if (err instanceof ConfigurationError) { - this.errors.addError(err.configurationPath, err.message) - return undefined - } - throw err - } - }), - ) - const webs = getArrayRejectingUndefined(webResults) + const webResults = await Promise.all(webFiles.map((webFile) => loadSingleWeb(webFile.path, webFile.content))) + const webs: Web[] = [] + for (const result of webResults) { + if (result.errors) { + this.errors.addErrors(result.errors) + } else { + webs.push(result.web) + } + } this.validateWebs(webs) const allWebsUnderStandardDir = webTomlPaths.every((webPath) => { @@ -561,12 +576,10 @@ class AppLoader ` ${path}`).join('\n') const lastConflictingPath = conflictingPaths[conflictingPaths.length - 1]! - this.errors.addError( - lastConflictingPath, - outputContent`You can only have one "web" configuration file with the ${outputToken.yellow( - webType, - )} role in your app.\n\nConflicting configurations found at:\n${pathsList}`, - ) + this.errors.addError({ + file: lastConflictingPath, + message: `You can only have one "web" configuration file with the ${webType} role in your app.\n\nConflicting configurations found at:\n${pathsList}`, + }) } }) } @@ -577,63 +590,59 @@ class AppLoader { - try { - const specification = this.findSpecificationForType(type) - let entryPath - let usedKnownSpecification = false + const specification = this.findSpecificationForType(type) + let entryPath + let usedKnownSpecification = false - if (specification) { - usedKnownSpecification = true - } else if (this.ignoreUnknownExtensions) { - return undefined - } else { - this.errors.addError( - configurationPath, - outputContent`Invalid extension type "${type}" in "${relativizePath(configurationPath)}"`, - ) - return undefined - } + if (specification) { + usedKnownSpecification = true + } else if (this.ignoreUnknownExtensions) { + return undefined + } else { + this.errors.addError({ + file: configurationPath, + message: `Invalid extension type "${type}" in "${relativizePath(configurationPath)}"`, + }) + return undefined + } - const configuration = parseConfigurationObjectAgainstSpecification( - specification, - configurationPath, - configurationObject, - ) + const specResult = parseConfigurationObjectAgainstSpecification( + specification, + configurationPath, + configurationObject, + ) + if (specResult.errors) { + this.errors.addErrors(specResult.errors) + return undefined + } + const configuration = specResult.data - if (usedKnownSpecification) { - entryPath = await this.findEntryPath(directory, specification) - } + if (usedKnownSpecification) { + entryPath = await this.findEntryPath(directory, specification) + } - const extensionInstance = new ExtensionInstance({ - configuration, - configurationPath, - entryPath, - directory, - specification, - }) + const extensionInstance = new ExtensionInstance({ + configuration, + configurationPath, + entryPath, + directory, + specification, + }) - if (this.reloadState && configuration.handle) { - const previousDevUUID = this.reloadState.extensionDevUUIDs.get(configuration.handle) - if (previousDevUUID) { - // Keep the existing devUUID for consistency with the dev-console across reloads - extensionInstance.devUUID = previousDevUUID - } + if (this.reloadState && configuration.handle) { + const previousDevUUID = this.reloadState.extensionDevUUIDs.get(configuration.handle) + if (previousDevUUID) { + extensionInstance.devUUID = previousDevUUID } + } - if (usedKnownSpecification) { - const validateResult = await extensionInstance.validate() - if (validateResult.isErr()) { - this.errors.addError(configurationPath, outputContent`\n${validateResult.error}`) - } + if (usedKnownSpecification) { + const validateResult = await extensionInstance.validate() + if (validateResult.isErr()) { + this.errors.addError({file: configurationPath, message: stringifyMessage(validateResult.error).trim()}) } - return extensionInstance - } catch (err) { - if (err instanceof ConfigurationError) { - this.errors.addError(err.configurationPath, err.message) - return undefined - } - throw err } + return extensionInstance } private async loadExtensions(appDirectory: string, appConfiguration: TConfig): Promise { @@ -654,12 +663,10 @@ class AppLoader ext.handle === extension.handle) const result = joinWithAnd(matchingExtensions.map((ext) => ext.name)) - const handle = outputToken.cyan(extension.handle) - - this.errors.addError( - extension.configurationPath, - outputContent`Duplicated handle "${handle}" in extensions ${result}. Handle needs to be unique per extension.`, - ) + this.errors.addError({ + file: extension.configurationPath, + message: `Duplicated handle "${extension.handle}" in extensions ${result}. Handle needs to be unique per extension.`, + }) } else if (extension.handle) { handles.add(extension.handle) } @@ -681,10 +688,10 @@ class AppLoader { const mergedConfig = {...configuration, ...extensionConfig} - // Remove `extensions` and `path`, they are injected automatically but not needed nor expected by the contract if (!mergedConfig.handle) { - // Handle is required for unified config extensions. - this.errors.addError( - configurationPath, - outputContent`Missing handle for extension "${mergedConfig.name}" at ${relativePath( - appDirectory, - configurationPath, - )}`, - ) + this.errors.addError({ + file: configurationPath, + message: `Missing handle for extension "${mergedConfig.name}" at ${relativePath(appDirectory, configurationPath)}`, + }) mergedConfig.handle = 'unknown-handle' } return this.createExtensionInstance(mergedConfig.type, mergedConfig, configurationPath, directory) }) return Promise.all(extensionsInstancesPromises) } else if (type) { - // Legacy toml file with a single extension. return this.createExtensionInstance(type, obj, configurationPath, directory) } else { - this.errors.addError( - configurationPath, - outputContent`Invalid extension type at "${outputToken.path( - relativePath(appDirectory, configurationPath), - )}". Please specify a type.`, - ) + this.errors.addError({ + file: configurationPath, + message: `Invalid extension type at "${relativePath(appDirectory, configurationPath)}". Please specify a type.`, + }) return undefined } }) @@ -739,18 +734,13 @@ class AppLoader { @@ -777,20 +767,12 @@ class AppLoader isAppConfigSpecification(specification)) .filter((specification) => specification.identifier !== WebhookSubscriptionSpecIdentifier) .map(async (specification) => { - let specConfiguration - try { - specConfiguration = parseConfigurationObjectAgainstSpecification( - specification, - configPath, - appConfiguration, - ) - } catch (err) { - if (err instanceof ConfigurationError) { - this.errors.addError(err.configurationPath, err.message) - return [null, [] as string[]] as const - } - throw err + const specResult = parseConfigurationObjectAgainstSpecification(specification, configPath, appConfiguration) + if (specResult.errors) { + this.errors.addErrors(specResult.errors) + return [null, [] as string[]] as const } + const specConfiguration = specResult.data if (Object.keys(specConfiguration).length === 0) return [null, Object.keys(specConfiguration)] as const @@ -820,10 +802,10 @@ class AppLoader 0 && !this.ignoreUnknownExtensions) { - this.errors.addError( - configPath, - outputContent`Unsupported section(s) in app configuration: ${unusedKeys.sort().join(', ')}`, - ) + this.errors.addError({ + file: configPath, + message: `Unsupported section(s) in app configuration: ${unusedKeys.sort().join(', ')}`, + }) } return extensionInstancesWithKeys .filter(([instance]) => instance) @@ -854,12 +836,10 @@ class AppLoader sourcePath !== undefined) if (!entryPath) { - this.errors.addError( - directory, - outputContent`Couldn't find an index.{js,jsx,ts,tsx} file in the directories ${outputToken.path( - directory, - )} or ${outputToken.path(joinPath(directory, 'src'))}`, - ) + this.errors.addError({ + file: directory, + message: `Couldn't find an index.{js,jsx,ts,tsx} file in the directories ${directory} or ${joinPath(directory, 'src')}`, + }) } } else if (specification.identifier === 'function') { entryPath = ( diff --git a/packages/app/src/cli/services/app-context.test.ts b/packages/app/src/cli/services/app-context.test.ts index 68e3019452..460ab4095c 100644 --- a/packages/app/src/cli/services/app-context.test.ts +++ b/packages/app/src/cli/services/app-context.test.ts @@ -237,7 +237,7 @@ client_id="test-api-key"` const loadSpy = vi.spyOn(loader, 'loadAppFromContext') const {AppErrors} = await import('../models/app/loader.js') const errors = new AppErrors() - errors.addError('test', 'some error') + errors.addError({file: 'test', message: 'some error'}) loadSpy.mockResolvedValue({errors} as any) // When/Then diff --git a/packages/app/src/cli/services/app-context.ts b/packages/app/src/cli/services/app-context.ts index 6a449f1770..ddd25de060 100644 --- a/packages/app/src/cli/services/app-context.ts +++ b/packages/app/src/cli/services/app-context.ts @@ -7,16 +7,27 @@ import {addUidToTomlsIfNecessary} from './app/add-uid-to-extension-toml.js' import {loadLocalExtensionsSpecifications} from '../models/extensions/load-specifications.js' import {Organization, OrganizationApp, OrganizationSource} from '../models/organization.js' import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' -import {getAppConfigurationContext, loadAppFromContext} from '../models/app/loader.js' +import { + getAppConfigurationContext, + loadAppFromContext, + formatConfigurationError, + type ConfigurationError, +} from '../models/app/loader.js' import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js' import {AppLinkedInterface, AppInterface} from '../models/app/app.js' import {Project} from '../models/project/project.js' import metadata from '../metadata.js' import {tryParseInt} from '@shopify/cli-kit/common/string' import {AbortError, BugError} from '@shopify/cli-kit/node/error' +import {outputContent, outputToken} from '@shopify/cli-kit/node/output' import {basename} from '@shopify/cli-kit/node/path' import type {ActiveConfig} from '../models/project/active-config.js' +function styledConfigurationError(error: ConfigurationError) { + const formatted = formatConfigurationError(error) + return outputContent`${outputToken.errorText('Validation error')} in ${outputToken.path(error.file)}:\n\n${formatted}` +} + export interface LoadedAppContextOutput { app: AppLinkedInterface remoteApp: OrganizationApp @@ -110,7 +121,7 @@ export async function linkedAppContext({ }) if (!unsafeTolerateErrors && !localApp.errors.isEmpty()) { - throw new AbortError(localApp.errors.toJSON()[0]!) + throw new AbortError(styledConfigurationError(localApp.errors.getErrors()[0]!)) } // If the remoteApp is the same as the linked one, update the cached info. @@ -166,7 +177,7 @@ export async function localAppContext({ const app = await loadAppFromContext({project, activeConfig, specifications, ignoreUnknownExtensions: true}) if (!app.errors.isEmpty()) { - throw new AbortError(app.errors.toJSON()[0]!) + throw new AbortError(styledConfigurationError(app.errors.getErrors()[0]!)) } return {app, project} diff --git a/packages/app/src/cli/services/app/config-pipeline-snapshot.test.ts b/packages/app/src/cli/services/app/config-pipeline-snapshot.test.ts index 309876880b..ab03312d32 100644 --- a/packages/app/src/cli/services/app/config-pipeline-snapshot.test.ts +++ b/packages/app/src/cli/services/app/config-pipeline-snapshot.test.ts @@ -8,7 +8,9 @@ import {describe, expect, test} from 'vitest' import type {zod} from '@shopify/cli-kit/node/schema' async function parseConfigAsCurrentApp(schema: zod.ZodTypeAny, filePath: string): Promise { - return parseConfigurationFile(schema, filePath) as Promise + const result = await parseConfigurationFile(schema, filePath) + if (result.errors) throw new Error(result.errors.map((err) => err.message).join('\n')) + return result.data as CurrentAppConfiguration } /** diff --git a/packages/app/src/cli/services/info.test.ts b/packages/app/src/cli/services/info.test.ts index 00c9680a08..5a34f9a79c 100644 --- a/packages/app/src/cli/services/info.test.ts +++ b/packages/app/src/cli/services/info.test.ts @@ -150,8 +150,8 @@ describe('info', () => { }) const errors = new AppErrors() - errors.addError(uiExtension1.configurationPath, 'Mock error with ui_extension') - errors.addError(uiExtension2.configurationPath, 'Mock error with checkout_ui_extension') + errors.addError({file: uiExtension1.configurationPath, message: 'Mock error with ui_extension'}) + errors.addError({file: uiExtension2.configurationPath, message: 'Mock error with checkout_ui_extension'}) const app = mockApp({ directory: tmp, diff --git a/packages/app/src/cli/services/info.ts b/packages/app/src/cli/services/info.ts index d6bd74a77e..3230ea2ae2 100644 --- a/packages/app/src/cli/services/info.ts +++ b/packages/app/src/cli/services/info.ts @@ -13,7 +13,6 @@ import { formatPackageManagerCommand, outputContent, shouldDisplayColors, - stringifyMessage, } from '@shopify/cli-kit/node/output' import {AlertCustomSection, InlineToken} from '@shopify/cli-kit/node/ui' import {CLI_KIT_VERSION} from '@shopify/cli-kit/common/version' @@ -196,7 +195,7 @@ class AppInfo { } webComponentsSection(): AlertCustomSection | undefined { - const errors: OutputMessage[] = [] + const errors: string[] = [] const sublevels: InlineToken[][] = [] if (!this.app.webs[0]) return this.app.webs.forEach((web) => { @@ -217,8 +216,8 @@ class AppInfo { sublevels.push([{subdued: ` 📂 ${UNKNOWN_TEXT}`}, {filePath: relativePath(this.app.directory, web.directory)}]) } if (!this.app.errors.isEmpty()) { - const error = this.app.errors.getError(`${web.directory}/${configurationFileNames.web}`) - if (error) errors.push(error) + const fileErrors = this.app.errors.getErrors(`${web.directory}/${configurationFileNames.web}`) + errors.push(...fileErrors.map((err) => err.message)) } }) @@ -254,19 +253,18 @@ class AppInfo { if (config && 'metafields' in config && Array.isArray(config.metafields) && config.metafields.length > 0) { details.push([' metafields', `${config.metafields.length}`]) } - const error = this.app.errors.getError(extension.configurationPath) - if (error) { - details.push([{error: ' error'}, {error: this.formattedError(error)}]) + const fileErrors = this.app.errors.getErrors(extension.configurationPath) + for (const error of fileErrors) { + details.push([{error: ' error'}, {error: this.formattedError(error.message)}]) } return details } - formattedError(str: OutputMessage): string { - // Some errors have newlines at the beginning for no apparent reason - const rawErrorMessage = stringifyMessage(str).trim() + formattedError(str: string): string { + const rawErrorMessage = str.trim() if (shouldDisplayColors()) return rawErrorMessage - const [errorFirstLine, ...errorRemainingLines] = stringifyMessage(str).trim().split('\n') + const [errorFirstLine, ...errorRemainingLines] = rawErrorMessage.split('\n') return [`! ${errorFirstLine}`, ...errorRemainingLines.map((line) => ` ${line}`)].join('\n') } diff --git a/packages/app/src/cli/services/validate.test.ts b/packages/app/src/cli/services/validate.test.ts index c475c4399d..3f390aa032 100644 --- a/packages/app/src/cli/services/validate.test.ts +++ b/packages/app/src/cli/services/validate.test.ts @@ -1,6 +1,6 @@ import {validateApp} from './validate.js' import {testAppLinked} from '../models/app/app.test-data.js' -import {AppErrors} from '../models/app/loader.js' +import {AppErrors, formatConfigurationError} from '../models/app/loader.js' import {describe, expect, test, vi} from 'vitest' import {outputResult} from '@shopify/cli-kit/node/output' import {renderError, renderSuccess} from '@shopify/cli-kit/node/ui' @@ -15,42 +15,42 @@ vi.mock('@shopify/cli-kit/node/output', async (importOriginal) => { }) vi.mock('@shopify/cli-kit/node/ui') +describe('formatConfigurationError', () => { + test('returns plain message when no path', () => { + expect(formatConfigurationError({file: 'foo.toml', message: 'something broke'})).toBe('something broke') + }) + + test('includes field path when present', () => { + expect(formatConfigurationError({file: 'foo.toml', path: ['access', 'admin'], message: 'Required'})).toBe( + '[access.admin]: Required', + ) + }) +}) + describe('validateApp', () => { test('renders success when there are no errors', async () => { - // Given const app = testAppLinked() - - // When await validateApp(app) - - // Then expect(renderSuccess).toHaveBeenCalledWith({headline: 'App configuration is valid.'}) expect(renderError).not.toHaveBeenCalled() expect(outputResult).not.toHaveBeenCalled() }) test('outputs json success when --json is enabled and there are no errors', async () => { - // Given const app = testAppLinked() - - // When await validateApp(app, {json: true}) - - // Then expect(outputResult).toHaveBeenCalledWith(JSON.stringify({valid: true, errors: []}, null, 2)) expect(renderSuccess).not.toHaveBeenCalled() expect(renderError).not.toHaveBeenCalled() }) test('renders errors and throws when there are validation errors', async () => { - // Given const errors = new AppErrors() - errors.addError('/path/to/shopify.app.toml', 'client_id is required') - errors.addError('/path/to/extensions/my-ext/shopify.extension.toml', 'invalid type "unknown"') + errors.addError({file: '/path/to/shopify.app.toml', message: 'client_id is required'}) + errors.addError({file: '/path/to/extensions/my-ext/shopify.extension.toml', message: 'invalid type "unknown"'}) const app = testAppLinked() app.errors = errors - // When / Then await expect(validateApp(app)).rejects.toThrow(AbortSilentError) expect(renderError).toHaveBeenCalledWith({ headline: 'Validation errors found.', @@ -61,40 +61,36 @@ describe('validateApp', () => { }) test('outputs json errors and throws when --json is enabled and there are validation errors', async () => { - // Given const errors = new AppErrors() - errors.addError('/path/to/shopify.app.toml', 'client_id is required') - errors.addError('/path/to/extensions/my-ext/shopify.extension.toml', 'invalid type "unknown"') + errors.addError({file: '/path/to/shopify.app.toml', message: 'client_id is required'}) + errors.addError({file: '/path/to/extensions/my-ext/shopify.extension.toml', message: 'invalid type "unknown"'}) const app = testAppLinked() app.errors = errors - // When / Then await expect(validateApp(app, {json: true})).rejects.toThrow(AbortSilentError) expect(outputResult).toHaveBeenCalledWith( - JSON.stringify( - { - valid: false, - errors: ['client_id is required', 'invalid type "unknown"'], - }, - null, - 2, - ), + JSON.stringify({valid: false, errors: ['client_id is required', 'invalid type "unknown"']}, null, 2), ) expect(renderError).not.toHaveBeenCalled() expect(renderSuccess).not.toHaveBeenCalled() }) test('renders success when errors object exists but is empty', async () => { - // Given const errors = new AppErrors() const app = testAppLinked() app.errors = errors - - // When await validateApp(app) - - // Then expect(renderSuccess).toHaveBeenCalledWith({headline: 'App configuration is valid.'}) expect(outputResult).not.toHaveBeenCalled() }) + + test('formats structured errors with paths for JSON output', async () => { + const errors = new AppErrors() + errors.addError({file: '/path/to/shopify.app.toml', path: ['name'], message: 'Required', code: 'invalid_type'}) + const app = testAppLinked() + app.errors = errors + + await expect(validateApp(app, {json: true})).rejects.toThrow(AbortSilentError) + expect(outputResult).toHaveBeenCalledWith(JSON.stringify({valid: false, errors: ['[name]: Required']}, null, 2)) + }) }) diff --git a/packages/app/src/cli/services/validate.ts b/packages/app/src/cli/services/validate.ts index d27efebeea..ddca3fe685 100644 --- a/packages/app/src/cli/services/validate.ts +++ b/packages/app/src/cli/services/validate.ts @@ -1,5 +1,6 @@ import {AppLinkedInterface} from '../models/app/app.js' -import {outputResult, stringifyMessage} from '@shopify/cli-kit/node/output' +import {formatConfigurationError} from '../models/app/loader.js' +import {outputResult} from '@shopify/cli-kit/node/output' import {renderError, renderSuccess} from '@shopify/cli-kit/node/ui' import {AbortSilentError} from '@shopify/cli-kit/node/error' @@ -8,9 +9,9 @@ interface ValidateAppOptions { } export async function validateApp(app: AppLinkedInterface, options: ValidateAppOptions = {json: false}): Promise { - const errors = app.errors + const appErrors = app.errors - if (!errors || errors.isEmpty()) { + if (!appErrors || appErrors.isEmpty()) { if (options.json) { outputResult(JSON.stringify({valid: true, errors: []}, null, 2)) return @@ -20,16 +21,17 @@ export async function validateApp(app: AppLinkedInterface, options: ValidateAppO return } - const errorMessages = errors.toJSON().map((error) => stringifyMessage(error).trim()) + const errors = appErrors.getErrors() if (options.json) { + const errorMessages = errors.map(formatConfigurationError) outputResult(JSON.stringify({valid: false, errors: errorMessages}, null, 2)) throw new AbortSilentError() } renderError({ headline: 'Validation errors found.', - body: errorMessages.join('\n\n'), + body: errors.map((err) => `• ${formatConfigurationError(err)}`).join('\n'), }) throw new AbortSilentError()