Current Behavior
The TransportManager.closeAll() method uses console.error for logging errors during transport cleanup:
} catch (error) {
// Log but don't throw - we want to close all transports
console.error('Error closing transport:', error)
}
Location: packages/transport/src/manager.ts:132
Issues
- Library using console.error: Libraries should avoid direct console logging
- Missing context: Error doesn't indicate which transport failed
- No structured logging: Can't be captured by logging frameworks
Recommendations
Option 1: Silent Failure with Comment (Simplest)
} catch (error) {
// Silently ignore close errors during shutdown - transport may already be closed
// or connection may have been lost. We still want to close other transports.
}
Option 2: Include Transport Context
} catch (error) {
console.error(
`Error closing ${entry.isRTU ? 'RTU' : 'TCP'} transport:`,
entry.config,
error
)
}
Option 3: Aggregate Errors (Most Robust)
const errors: Error[] = []
for (const entry of this.transports.values()) {
try {
await entry.rawTransport.close()
} catch (error) {
errors.push(error as Error)
}
}
if (errors.length > 0) {
throw new AggregateError(errors, `Failed to close ${errors.length} transport(s)`)
}
Related
Priority
Low - Current implementation works correctly, this is a code quality improvement
From Claude Code Review of PR #125
Current Behavior
The
TransportManager.closeAll()method usesconsole.errorfor logging errors during transport cleanup:Location:
packages/transport/src/manager.ts:132Issues
Recommendations
Option 1: Silent Failure with Comment (Simplest)
Option 2: Include Transport Context
Option 3: Aggregate Errors (Most Robust)
Related
Priority
Low - Current implementation works correctly, this is a code quality improvement
From Claude Code Review of PR #125