Skip to content

Commit d29a9a2

Browse files
Copilotsynthable
Copilot
and
synthable
committed
refactor: Extract duplicated error handling and validation utilities
- Extract error message extraction utility in CLI file-operations - Extract NodeJS error code checking utility in SDK - Extract validation error formatting utility in SDK loaders - Add comprehensive tests for new utility functions - Consolidate file read/write operations in CLI Co-authored-by: synthable <374893+synthable@users.noreply.github.com>
1 parent 100d3bc commit d29a9a2

File tree

7 files changed

+214
-38
lines changed

7 files changed

+214
-38
lines changed

‎packages/ums-cli/src/utils/file-operations.ts‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,27 +12,38 @@ import { join } from 'path';
1212
import { glob } from 'glob';
1313

1414
/**
15-
* Reads a persona file and returns its content as a string
15+
* Extracts error message from unknown error type
1616
*/
17-
export async function readPersonaFile(path: string): Promise<string> {
17+
function getErrorMessage(error: unknown): string {
18+
return error instanceof Error ? error.message : String(error);
19+
}
20+
21+
/**
22+
* Generic file read operation with error handling
23+
*/
24+
async function readFileWithContext(
25+
path: string,
26+
context: string
27+
): Promise<string> {
1828
try {
1929
return await readFileAsync(path, 'utf-8');
2030
} catch (error) {
21-
const message = error instanceof Error ? error.message : String(error);
22-
throw new Error(`Failed to read persona file '${path}': ${message}`);
31+
throw new Error(`Failed to read ${context} '${path}': ${getErrorMessage(error)}`);
2332
}
2433
}
2534

35+
/**
36+
* Reads a persona file and returns its content as a string
37+
*/
38+
export async function readPersonaFile(path: string): Promise<string> {
39+
return readFileWithContext(path, 'persona file');
40+
}
41+
2642
/**
2743
* Reads a module file and returns its content as a string
2844
*/
2945
export async function readModuleFile(path: string): Promise<string> {
30-
try {
31-
return await readFileAsync(path, 'utf-8');
32-
} catch (error) {
33-
const message = error instanceof Error ? error.message : String(error);
34-
throw new Error(`Failed to read module file '${path}': ${message}`);
35-
}
46+
return readFileWithContext(path, 'module file');
3647
}
3748

3849
/**
@@ -45,8 +56,7 @@ export async function writeOutputFile(
4556
try {
4657
await writeFileAsync(path, content, 'utf-8');
4758
} catch (error) {
48-
const message = error instanceof Error ? error.message : String(error);
49-
throw new Error(`Failed to write output file '${path}': ${message}`);
59+
throw new Error(`Failed to write output file '${path}': ${getErrorMessage(error)}`);
5060
}
5161
}
5262

@@ -66,9 +76,8 @@ export async function discoverModuleFiles(paths: string[]): Promise<string[]> {
6676
allFiles.push(...files);
6777
}
6878
} catch (error) {
69-
const message = error instanceof Error ? error.message : String(error);
7079
throw new Error(
71-
`Failed to discover modules in path '${path}': ${message}`
80+
`Failed to discover modules in path '${path}': ${getErrorMessage(error)}`
7281
);
7382
}
7483
}

‎packages/ums-sdk/src/loaders/loader-utils.test.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/**
2+
* Tests for loader utilities
3+
*/
4+
5+
import { describe, it, expect } from 'vitest';
6+
import { filePathToUrl, formatValidationErrors } from './loader-utils.js';
7+
import type { ValidationResult } from 'ums-lib';
8+
9+
describe('loader-utils', () => {
10+
describe('filePathToUrl', () => {
11+
it('should convert file path to file URL', () => {
12+
const filePath = '/path/to/module.ts';
13+
const url = filePathToUrl(filePath);
14+
15+
expect(url).toContain('file://');
16+
expect(url).toContain('module.ts');
17+
});
18+
19+
it('should handle paths with special characters', () => {
20+
const filePath = '/path/to/my module.ts';
21+
const url = filePathToUrl(filePath);
22+
23+
expect(url).toContain('file://');
24+
expect(url).toContain('my%20module.ts');
25+
});
26+
});
27+
28+
describe('formatValidationErrors', () => {
29+
it('should format validation errors with paths', () => {
30+
const validation: ValidationResult = {
31+
valid: false,
32+
errors: [
33+
{ path: 'metadata.name', message: 'Name is required' },
34+
{ path: 'components[0]', message: 'Invalid component' }
35+
],
36+
warnings: []
37+
};
38+
39+
const result = formatValidationErrors(validation);
40+
expect(result).toBe('metadata.name: Name is required; components[0]: Invalid component');
41+
});
42+
43+
it('should use default path for errors without path', () => {
44+
const validation: ValidationResult = {
45+
valid: false,
46+
errors: [
47+
{ message: 'Invalid structure' }
48+
],
49+
warnings: []
50+
};
51+
52+
const result = formatValidationErrors(validation);
53+
expect(result).toBe('module: Invalid structure');
54+
});
55+
56+
it('should use custom default path', () => {
57+
const validation: ValidationResult = {
58+
valid: false,
59+
errors: [
60+
{ message: 'Invalid structure' }
61+
],
62+
warnings: []
63+
};
64+
65+
const result = formatValidationErrors(validation, 'persona');
66+
expect(result).toBe('persona: Invalid structure');
67+
});
68+
69+
it('should handle mixed errors with and without paths', () => {
70+
const validation: ValidationResult = {
71+
valid: false,
72+
errors: [
73+
{ path: 'id', message: 'ID is required' },
74+
{ message: 'Missing schema version' },
75+
{ path: 'version', message: 'Invalid version format' }
76+
],
77+
warnings: []
78+
};
79+
80+
const result = formatValidationErrors(validation);
81+
expect(result).toBe('id: ID is required; module: Missing schema version; version: Invalid version format');
82+
});
83+
84+
it('should handle single error', () => {
85+
const validation: ValidationResult = {
86+
valid: false,
87+
errors: [
88+
{ path: 'id', message: 'ID is required' }
89+
],
90+
warnings: []
91+
};
92+
93+
const result = formatValidationErrors(validation);
94+
expect(result).toBe('id: ID is required');
95+
});
96+
97+
it('should handle empty errors array', () => {
98+
const validation: ValidationResult = {
99+
valid: true,
100+
errors: [],
101+
warnings: []
102+
};
103+
104+
const result = formatValidationErrors(validation);
105+
expect(result).toBe('');
106+
});
107+
});
108+
});

‎packages/ums-sdk/src/loaders/loader-utils.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/**
2+
* Shared utilities for module and persona loaders
3+
*/
4+
5+
import { pathToFileURL } from 'node:url';
6+
import type { ValidationResult } from 'ums-lib';
7+
8+
/**
9+
* Convert file path to file URL for dynamic import
10+
* @param filePath - Absolute path to file
11+
* @returns File URL string suitable for dynamic import
12+
*/
13+
export function filePathToUrl(filePath: string): string {
14+
return pathToFileURL(filePath).href;
15+
}
16+
17+
/**
18+
* Format validation errors into a single error message
19+
* @param validation - Validation result from ums-lib
20+
* @param defaultPath - Default path to use if error has no path (e.g., 'module', 'persona')
21+
* @returns Formatted error message string
22+
*/
23+
export function formatValidationErrors(
24+
validation: ValidationResult,
25+
defaultPath = 'module'
26+
): string {
27+
return validation.errors
28+
.map(e => `${e.path ?? defaultPath}: ${e.message}`)
29+
.join('; ');
30+
}

‎packages/ums-sdk/src/loaders/module-loader.ts‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
*/
1414

1515
import { readFile } from 'node:fs/promises';
16-
import { pathToFileURL } from 'node:url';
1716
import {
1817
moduleIdToExportName,
1918
parseModule,
@@ -25,7 +24,8 @@ import {
2524
ModuleNotFoundError,
2625
InvalidExportError,
2726
} from '../errors/index.js';
28-
import { checkFileExists } from '../utils/file-utils.js';
27+
import { checkFileExists, isFileNotFoundError } from '../utils/file-utils.js';
28+
import { filePathToUrl, formatValidationErrors } from './loader-utils.js';
2929

3030
/**
3131
* Checks if an object looks like a UMS Module (duck typing).
@@ -66,7 +66,7 @@ export class ModuleLoader {
6666
await checkFileExists(filePath);
6767

6868
// Convert file path to file URL for dynamic import
69-
const fileUrl = pathToFileURL(filePath).href;
69+
const fileUrl = filePathToUrl(filePath);
7070

7171
// Dynamically import the TypeScript file (tsx handles compilation)
7272
const moduleExports = (await import(fileUrl)) as Record<string, unknown>;
@@ -120,11 +120,8 @@ export class ModuleLoader {
120120
// Delegate to ums-lib for full UMS v2.0 spec validation
121121
const validation = validateModule(parsedModule);
122122
if (!validation.valid) {
123-
const errorMessages = validation.errors
124-
.map(e => `${e.path ?? 'module'}: ${e.message}`)
125-
.join('; ');
126123
throw new ModuleLoadError(
127-
`Module validation failed: ${errorMessages}`,
124+
`Module validation failed: ${formatValidationErrors(validation)}`,
128125
filePath
129126
);
130127
}
@@ -162,11 +159,8 @@ export class ModuleLoader {
162159
try {
163160
return await readFile(filePath, 'utf-8');
164161
} catch (error) {
165-
if (error && typeof error === 'object' && 'code' in error) {
166-
const nodeError = error as NodeJS.ErrnoException;
167-
if (nodeError.code === 'ENOENT') {
168-
throw new ModuleNotFoundError(filePath);
169-
}
162+
if (isFileNotFoundError(error)) {
163+
throw new ModuleNotFoundError(filePath);
170164
}
171165
throw new ModuleLoadError(
172166
`Failed to read file: ${error instanceof Error ? error.message : String(error)}`,

‎packages/ums-sdk/src/loaders/persona-loader.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212
* - Validation (UMS v2.0 spec compliance)
1313
*/
1414

15-
import { pathToFileURL } from 'node:url';
1615
import { parsePersona, validatePersona, type Persona } from 'ums-lib';
1716
import { ModuleLoadError, ModuleNotFoundError } from '../errors/index.js';
1817
import { checkFileExists } from '../utils/file-utils.js';
18+
import { filePathToUrl, formatValidationErrors } from './loader-utils.js';
1919

2020
/**
2121
* PersonaLoader - Loads and validates TypeScript persona files
@@ -34,7 +34,7 @@ export class PersonaLoader {
3434
await checkFileExists(filePath);
3535

3636
// Convert file path to file URL for dynamic import
37-
const fileUrl = pathToFileURL(filePath).href;
37+
const fileUrl = filePathToUrl(filePath);
3838

3939
// Dynamically import the TypeScript file
4040
const personaExports = (await import(fileUrl)) as Record<string, unknown>;
@@ -69,11 +69,8 @@ export class PersonaLoader {
6969
// Delegate to ums-lib for full UMS v2.0 spec validation
7070
const validation = validatePersona(parsedPersona);
7171
if (!validation.valid) {
72-
const errorMessages = validation.errors
73-
.map(e => `${e.path ?? 'persona'}: ${e.message}`)
74-
.join('; ');
7572
throw new ModuleLoadError(
76-
`Persona validation failed: ${errorMessages}`,
73+
`Persona validation failed: ${formatValidationErrors(validation, 'persona')}`,
7774
filePath
7875
);
7976
}

‎packages/ums-sdk/src/utils/file-utils.test.ts‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
*/
44

55
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
6-
import { checkFileExists } from './file-utils.js';
6+
import { checkFileExists, isFileNotFoundError } from './file-utils.js';
77
import { ModuleNotFoundError } from '../errors/index.js';
88
import * as fs from 'node:fs/promises';
99

@@ -14,6 +14,33 @@ vi.mock('node:fs/promises', () => ({
1414
}));
1515

1616
describe('file-utils', () => {
17+
describe('isFileNotFoundError', () => {
18+
it('should return true for ENOENT error', () => {
19+
const error = Object.assign(new Error('ENOENT'), { code: 'ENOENT' });
20+
expect(isFileNotFoundError(error)).toBe(true);
21+
});
22+
23+
it('should return false for other error codes', () => {
24+
const error = Object.assign(new Error('EACCES'), { code: 'EACCES' });
25+
expect(isFileNotFoundError(error)).toBe(false);
26+
});
27+
28+
it('should return false for errors without code property', () => {
29+
const error = new Error('Some error');
30+
expect(isFileNotFoundError(error)).toBe(false);
31+
});
32+
33+
it('should return false for null', () => {
34+
expect(isFileNotFoundError(null)).toBe(false);
35+
});
36+
37+
it('should return false for non-object errors', () => {
38+
expect(isFileNotFoundError('string error')).toBe(false);
39+
expect(isFileNotFoundError(123)).toBe(false);
40+
expect(isFileNotFoundError(undefined)).toBe(false);
41+
});
42+
});
43+
1744
describe('checkFileExists', () => {
1845
beforeEach(() => {
1946
vi.clearAllMocks();

‎packages/ums-sdk/src/utils/file-utils.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,20 @@
55
import { access, constants } from 'node:fs/promises';
66
import { ModuleNotFoundError } from '../errors/index.js';
77

8+
/**
9+
* Type guard to check if an error is a NodeJS ErrnoException
10+
*/
11+
function isNodeError(error: unknown): error is NodeJS.ErrnoException {
12+
return error !== null && typeof error === 'object' && 'code' in error;
13+
}
14+
15+
/**
16+
* Check if an error is an ENOENT (file not found) error
17+
*/
18+
export function isFileNotFoundError(error: unknown): boolean {
19+
return isNodeError(error) && error.code === 'ENOENT';
20+
}
21+
822
/**
923
* Check if a file exists using access() for efficiency.
1024
* @param filePath - Absolute path to the file
@@ -14,11 +28,8 @@ export async function checkFileExists(filePath: string): Promise<void> {
1428
try {
1529
await access(filePath, constants.F_OK);
1630
} catch (error) {
17-
if (error && typeof error === 'object' && 'code' in error) {
18-
const nodeError = error as NodeJS.ErrnoException;
19-
if (nodeError.code === 'ENOENT') {
20-
throw new ModuleNotFoundError(filePath);
21-
}
31+
if (isFileNotFoundError(error)) {
32+
throw new ModuleNotFoundError(filePath);
2233
}
2334
throw error;
2435
}

0 commit comments

Comments
 (0)