mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-02 22:37:32 +00:00
docs: expand type-safety guidance with TypeScript handbook and Google style patterns
This commit is contained in:
@@ -14,12 +14,14 @@ Type assertions bypass TypeScript's type checking. Instead, use proper type guar
|
||||
### DOM Elements
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
const el = e.target as HTMLInputElement
|
||||
el.value
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
if (e.target instanceof HTMLInputElement) {
|
||||
e.target.value
|
||||
@@ -29,12 +31,14 @@ if (e.target instanceof HTMLInputElement) {
|
||||
### Optional Properties on Objects
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
const obj = value as { prop?: string }
|
||||
if (obj.prop) { ... }
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
if ('prop' in value && typeof value.prop === 'string') {
|
||||
value.prop
|
||||
@@ -44,12 +48,14 @@ if ('prop' in value && typeof value.prop === 'string') {
|
||||
### Constructor/Static Properties
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
const ctor = node.constructor as { type?: string }
|
||||
const type = ctor.type
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
const ctor = node.constructor
|
||||
const type = 'type' in ctor && typeof ctor.type === 'string' ? ctor.type : undefined
|
||||
@@ -64,12 +70,14 @@ When you find yourself needing a cast, ask: "Can I fix the type definition inste
|
||||
If a property exists at runtime but not in the type, add it to the interface:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
const box = this.search_box as { close?: () => void }
|
||||
box?.close?.()
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
// Update the type definition
|
||||
search_box?: HTMLDivElement & ICloseable
|
||||
@@ -83,6 +91,7 @@ this.search_box?.close()
|
||||
If a callback receives a different type than declared, fix the callback signature:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
onDrawTooltip?: (link: LLink) => void
|
||||
// Later...
|
||||
@@ -90,6 +99,7 @@ onDrawTooltip(link as LLink) // link is actually LinkSegment
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
onDrawTooltip?: (link: LinkSegment) => void
|
||||
// Later...
|
||||
@@ -117,33 +127,110 @@ for (const panel of panels) {
|
||||
}
|
||||
```
|
||||
|
||||
## When Casts Are Unavoidable
|
||||
## Never Use `@ts-ignore` or `@ts-expect-error`
|
||||
|
||||
Some casts are genuinely unavoidable. Document why:
|
||||
These directives suppress all errors on a line, making it easy to accidentally mask serious bugs. Instead:
|
||||
|
||||
1. Fix the underlying type issue
|
||||
2. Use a type guard to narrow the type
|
||||
3. If truly unavoidable, use a targeted cast with explanation
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
/**
|
||||
* @deprecated Prefer {@link structuredClone}
|
||||
* Note: JSON.parse returns `unknown`, so type assertions are unavoidable here.
|
||||
* This function is deprecated precisely because it cannot be made type-safe.
|
||||
*/
|
||||
cloneObject<T>(obj: T): T {
|
||||
const cloned: unknown = JSON.parse(JSON.stringify(obj))
|
||||
return cloned as T // Unavoidable - JSON.parse returns unknown
|
||||
// @ts-expect-error - doesn't work otherwise
|
||||
node.customProperty = value
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
interface ExtendedNode extends LGraphNode {
|
||||
customProperty?: string
|
||||
}
|
||||
|
||||
function isExtendedNode(node: LGraphNode): node is ExtendedNode {
|
||||
return 'customProperty' in node
|
||||
}
|
||||
|
||||
if (isExtendedNode(node)) {
|
||||
node.customProperty = value
|
||||
}
|
||||
```
|
||||
|
||||
### Acceptable Cast Scenarios
|
||||
## Use `unknown` Instead of `any`
|
||||
|
||||
1. **`JSON.parse` results** - Returns `unknown`, must be cast
|
||||
2. **Conditional types in return positions** - TypeScript can't always narrow these
|
||||
3. **Third-party library limitations** - When you can't fix the source types
|
||||
When you don't know the type, use `unknown` and narrow with type guards:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
function process(data: any) {
|
||||
return data.value // No type checking
|
||||
}
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
function process(data: unknown) {
|
||||
if (typeof data === 'object' && data !== null && 'value' in data) {
|
||||
return data.value
|
||||
}
|
||||
return undefined
|
||||
}
|
||||
```
|
||||
|
||||
## Use Type Annotations for Object Literals
|
||||
|
||||
Prefer type annotations over assertions for object literals - this catches refactoring bugs:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
const config = {
|
||||
naem: 'test' // Typo not caught
|
||||
} as Config
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
const config: Config = {
|
||||
naem: 'test' // Error: 'naem' does not exist on type 'Config'
|
||||
}
|
||||
```
|
||||
|
||||
## Prefer Interfaces Over Type Aliases for Objects
|
||||
|
||||
Use interfaces for object types - they have better error messages and performance:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
type NodeConfig = {
|
||||
id: string
|
||||
name: string
|
||||
}
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
interface NodeConfig {
|
||||
id: string
|
||||
name: string
|
||||
}
|
||||
```
|
||||
|
||||
Use type aliases for unions, primitives, and tuples where interfaces don't apply.
|
||||
|
||||
## Prefer Specific Over General
|
||||
|
||||
Use the most specific type possible:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
const value = obj as any
|
||||
const value = obj as unknown
|
||||
@@ -151,6 +238,7 @@ const value = obj as Record<string, unknown>
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
// Use the actual expected type
|
||||
const value: ISerialisedNode = obj
|
||||
@@ -163,24 +251,144 @@ if (isSerialisedNode(obj)) { ... }
|
||||
For union types, use proper narrowing instead of casting:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
// NodeId = number | string
|
||||
(node.id as number) - (other.id as number)
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
if (typeof node.id === 'number' && typeof other.id === 'number') {
|
||||
node.id - other.id
|
||||
}
|
||||
```
|
||||
|
||||
## Primitive Types
|
||||
|
||||
Use lowercase primitive types, never boxed object types:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
function greet(name: String): String
|
||||
function count(items: Object): Number
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
function greet(name: string): string
|
||||
function count(items: object): number
|
||||
```
|
||||
|
||||
## Callback Types
|
||||
|
||||
### Return Types
|
||||
|
||||
Use `void` for callbacks whose return value is ignored:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
interface Options {
|
||||
onComplete?: () => any
|
||||
}
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
interface Options {
|
||||
onComplete?: () => void
|
||||
}
|
||||
```
|
||||
|
||||
### Optional Parameters
|
||||
|
||||
Don't make callback parameters optional unless you intend to invoke the callback with varying argument counts:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
interface Callbacks {
|
||||
onProgress?: (current: number, total?: number) => void
|
||||
}
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
interface Callbacks {
|
||||
onProgress?: (current: number, total: number) => void
|
||||
}
|
||||
```
|
||||
|
||||
Callbacks can always ignore parameters they don't need.
|
||||
|
||||
## Function Overloads
|
||||
|
||||
### Ordering
|
||||
|
||||
Put specific overloads before general ones (TypeScript uses first match):
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
declare function fn(x: unknown): unknown
|
||||
declare function fn(x: HTMLElement): HTMLElement
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
declare function fn(x: HTMLElement): HTMLElement
|
||||
declare function fn(x: unknown): unknown
|
||||
```
|
||||
|
||||
### Prefer Optional Parameters Over Overloads
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
interface Example {
|
||||
diff(one: string): number
|
||||
diff(one: string, two: string): number
|
||||
diff(one: string, two: string, three: string): number
|
||||
}
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
interface Example {
|
||||
diff(one: string, two?: string, three?: string): number
|
||||
}
|
||||
```
|
||||
|
||||
## Generics
|
||||
|
||||
Never create a generic type that doesn't use its type parameter:
|
||||
|
||||
❌ **Wrong:**
|
||||
|
||||
```typescript
|
||||
declare function fn<T>(): void
|
||||
```
|
||||
|
||||
✅ **Correct:**
|
||||
|
||||
```typescript
|
||||
declare function fn<T>(arg: T): T
|
||||
```
|
||||
|
||||
## Summary
|
||||
|
||||
1. **Never use `as`** unless absolutely unavoidable
|
||||
1. **Never use `as`** outside of custom type guards
|
||||
2. **Use `instanceof`** for class/DOM element checks
|
||||
3. **Use `'prop' in obj`** for property existence checks
|
||||
4. **Use `typeof`** for primitive type checks
|
||||
5. **Fix source types** instead of casting at usage sites
|
||||
6. **Create type guards** for repeated patterns
|
||||
7. **Document unavoidable casts** with clear explanations
|
||||
7. **Use `void`** for ignored callback returns
|
||||
|
||||
Reference in New Issue
Block a user