mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-02 14:27:40 +00:00
fix: improve type safety in litegraph library layer - No Explicit Any mission (PR 2) (#7401)
## Summary Part 2 of the type safety remediation plan. This PR focuses on the Litegraph Library Layer as part of the **No Explicit Any** mission. ### Changes **LiteGraphGlobal.ts:** - `DEFAULT_GROUP_FONT_SIZE`: Changed from `any` (with no value) to `number = 24`. The internal fallback was already 24, so the constant was effectively useless without an assigned value. - `getParameterNames`: Replace `any` with `unknown` in function signature - `extendClass`: Replace deprecated `__lookupGetter__`/`__defineGetter__` with modern `Object.getOwnPropertyDescriptor`/`defineProperty` and add proper Record types **LGraphNodeProperties.ts:** - Replace `any` with `unknown` for property values - Use `Record<string, unknown>` with proper type assertions for dynamic property access **types/widgets.ts & BaseWidget.ts:** - Change `callback` value parameter from `any` to properly typed (`unknown` in interface, `TWidget['value']` in implementation) **Consuming code fixes:** - `previewAny.ts`: Add explicit `boolean` type annotation for callback value - `ButtonWidget.ts`: Pass widget value instead of widget instance to callback (matching the interface signature) ## Breaking Change Analysis (Sourcegraph Verified) ### ButtonWidget callback fix (`this` → `this.value`) This PR fixes the ButtonWidget callback to pass `value` instead of `this`, matching the interface definition. **Verification via Sourcegraph** - all external usages are safe: - [comfyui-ollama](https://cs.comfy.org/github.com/stavsap/comfyui-ollama/-/blob/web/js/OllamaNode.js?L84) - doesn't use callback args - [ComfyLab-Pack](https://cs.comfy.org/github.com/bugltd/ComfyLab-Pack/-/blob/dist/js/nodes/list.js?L8) - doesn't use callback args - [ComfyUI_PaintingCoderUtils](https://cs.comfy.org/github.com/jammyfu/ComfyUI_PaintingCoderUtils/-/blob/web/js/click_popup.js?L18) - doesn't use callback args - [ComfyUI-ShaderNoiseKSampler](https://cs.comfy.org/github.com/AEmotionStudio/ComfyUI-ShaderNoiseKSampler/-/blob/web/matrix_button.js?L3055-3056) - was already working around this bug --------- Co-authored-by: GitHub Action <action@github.com>
This commit is contained in:
committed by
GitHub
parent
886fe07de9
commit
a6ca2bcd42
@@ -41,7 +41,7 @@ useExtensionService().registerExtension({
|
||||
app
|
||||
)
|
||||
|
||||
showAsPlaintextWidget.widget.callback = (value) => {
|
||||
showAsPlaintextWidget.widget.callback = (value: boolean) => {
|
||||
showValueWidget.hidden = !value
|
||||
showValueWidget.options.hidden = !value
|
||||
showValueWidgetPlain.hidden = value
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
import type { LGraphNode } from './LGraphNode'
|
||||
|
||||
function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === 'object' && value !== null
|
||||
}
|
||||
|
||||
/**
|
||||
* Default properties to track
|
||||
*/
|
||||
@@ -11,7 +15,6 @@ const DEFAULT_TRACKED_PROPERTIES: string[] = [
|
||||
'color',
|
||||
'bgcolor'
|
||||
]
|
||||
|
||||
/**
|
||||
* Manages node properties with optional change tracking and instrumentation.
|
||||
*/
|
||||
@@ -37,6 +40,34 @@ export class LGraphNodeProperties {
|
||||
}
|
||||
}
|
||||
|
||||
#resolveTargetObject(parts: string[]): {
|
||||
targetObject: Record<string, unknown>
|
||||
propertyName: string
|
||||
} {
|
||||
// LGraphNode supports dynamic property access at runtime
|
||||
let targetObject: Record<string, unknown> = this.node as unknown as Record<
|
||||
string,
|
||||
unknown
|
||||
>
|
||||
|
||||
if (parts.length === 1) {
|
||||
return { targetObject, propertyName: parts[0] }
|
||||
}
|
||||
|
||||
for (let i = 0; i < parts.length - 1; i++) {
|
||||
const key = parts[i]
|
||||
const next = targetObject[key]
|
||||
if (isRecord(next)) {
|
||||
targetObject = next
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
targetObject,
|
||||
propertyName: parts[parts.length - 1]
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Instruments a single property to track changes
|
||||
*/
|
||||
@@ -47,15 +78,7 @@ export class LGraphNodeProperties {
|
||||
this.#ensureNestedPath(path)
|
||||
}
|
||||
|
||||
let targetObject: any = this.node
|
||||
let propertyName = parts[0]
|
||||
|
||||
if (parts.length > 1) {
|
||||
for (let i = 0; i < parts.length - 1; i++) {
|
||||
targetObject = targetObject[parts[i]]
|
||||
}
|
||||
propertyName = parts.at(-1)!
|
||||
}
|
||||
const { targetObject, propertyName } = this.#resolveTargetObject(parts)
|
||||
|
||||
const hasProperty = Object.prototype.hasOwnProperty.call(
|
||||
targetObject,
|
||||
@@ -64,11 +87,11 @@ export class LGraphNodeProperties {
|
||||
const currentValue = targetObject[propertyName]
|
||||
|
||||
if (!hasProperty) {
|
||||
let value: any = undefined
|
||||
let value: unknown = undefined
|
||||
|
||||
Object.defineProperty(targetObject, propertyName, {
|
||||
get: () => value,
|
||||
set: (newValue: any) => {
|
||||
set: (newValue: unknown) => {
|
||||
const oldValue = value
|
||||
value = newValue
|
||||
this.#emitPropertyChange(path, oldValue, newValue)
|
||||
@@ -108,13 +131,20 @@ export class LGraphNodeProperties {
|
||||
*/
|
||||
#createInstrumentedDescriptor(
|
||||
propertyPath: string,
|
||||
initialValue: any
|
||||
initialValue: unknown
|
||||
): PropertyDescriptor {
|
||||
let value = initialValue
|
||||
return this.#createInstrumentedDescriptorTyped(propertyPath, initialValue)
|
||||
}
|
||||
|
||||
#createInstrumentedDescriptorTyped<TValue>(
|
||||
propertyPath: string,
|
||||
initialValue: TValue
|
||||
): PropertyDescriptor {
|
||||
let value: TValue = initialValue
|
||||
|
||||
return {
|
||||
get: () => value,
|
||||
set: (newValue: any) => {
|
||||
set: (newValue: TValue) => {
|
||||
const oldValue = value
|
||||
value = newValue
|
||||
this.#emitPropertyChange(propertyPath, oldValue, newValue)
|
||||
@@ -129,8 +159,16 @@ export class LGraphNodeProperties {
|
||||
*/
|
||||
#emitPropertyChange(
|
||||
propertyPath: string,
|
||||
oldValue: any,
|
||||
newValue: any
|
||||
oldValue: unknown,
|
||||
newValue: unknown
|
||||
): void {
|
||||
this.#emitPropertyChangeTyped(propertyPath, oldValue, newValue)
|
||||
}
|
||||
|
||||
#emitPropertyChangeTyped<TValue>(
|
||||
propertyPath: string,
|
||||
oldValue: TValue,
|
||||
newValue: TValue
|
||||
): void {
|
||||
this.node.graph?.trigger('node:property:changed', {
|
||||
nodeId: this.node.id,
|
||||
@@ -145,7 +183,11 @@ export class LGraphNodeProperties {
|
||||
*/
|
||||
#ensureNestedPath(path: string): void {
|
||||
const parts = path.split('.')
|
||||
let current: any = this.node
|
||||
// LGraphNode supports dynamic property access at runtime
|
||||
let current: Record<string, unknown> = this.node as unknown as Record<
|
||||
string,
|
||||
unknown
|
||||
>
|
||||
|
||||
// Create all parent objects except the last property
|
||||
for (let i = 0; i < parts.length - 1; i++) {
|
||||
@@ -153,7 +195,10 @@ export class LGraphNodeProperties {
|
||||
if (!current[part]) {
|
||||
current[part] = {}
|
||||
}
|
||||
current = current[part]
|
||||
const next = current[part]
|
||||
if (isRecord(next)) {
|
||||
current = next
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -175,7 +220,7 @@ export class LGraphNodeProperties {
|
||||
* Custom toJSON method for JSON.stringify
|
||||
* Returns undefined to exclude from serialization since we only use defaults
|
||||
*/
|
||||
toJSON(): any {
|
||||
toJSON(): undefined {
|
||||
return undefined
|
||||
}
|
||||
}
|
||||
|
||||
@@ -67,7 +67,7 @@ export class LiteGraphGlobal {
|
||||
DEFAULT_SHADOW_COLOR = 'rgba(0,0,0,0.5)'
|
||||
|
||||
DEFAULT_GROUP_FONT = 24
|
||||
DEFAULT_GROUP_FONT_SIZE?: any
|
||||
DEFAULT_GROUP_FONT_SIZE = 24
|
||||
GROUP_FONT = 'Inter'
|
||||
|
||||
WIDGET_BGCOLOR = '#222'
|
||||
@@ -716,7 +716,7 @@ export class LiteGraphGlobal {
|
||||
}
|
||||
|
||||
// used to create nodes from wrapping functions
|
||||
getParameterNames(func: (...args: any) => any): string[] {
|
||||
getParameterNames(func: (...args: unknown[]) => unknown): string[] {
|
||||
return String(func)
|
||||
.replaceAll(/\/\/.*$/gm, '') // strip single-line comments
|
||||
.replaceAll(/\s+/g, '') // strip white space
|
||||
@@ -971,7 +971,10 @@ export class LiteGraphGlobal {
|
||||
}
|
||||
}
|
||||
|
||||
extendClass(target: any, origin: any): void {
|
||||
extendClass(
|
||||
target: Record<string, unknown> & { prototype?: object },
|
||||
origin: Record<string, unknown> & { prototype?: object }
|
||||
): void {
|
||||
for (const i in origin) {
|
||||
// copy class properties
|
||||
// eslint-disable-next-line no-prototype-builtins
|
||||
@@ -979,33 +982,24 @@ export class LiteGraphGlobal {
|
||||
target[i] = origin[i]
|
||||
}
|
||||
|
||||
if (origin.prototype) {
|
||||
if (origin.prototype && target.prototype) {
|
||||
const originProto = origin.prototype as Record<string, unknown>
|
||||
const targetProto = target.prototype as Record<string, unknown>
|
||||
|
||||
// copy prototype properties
|
||||
for (const i in origin.prototype) {
|
||||
for (const i in originProto) {
|
||||
// only enumerable
|
||||
// eslint-disable-next-line no-prototype-builtins
|
||||
if (!origin.prototype.hasOwnProperty(i)) continue
|
||||
if (!originProto.hasOwnProperty(i)) continue
|
||||
|
||||
// avoid overwriting existing ones
|
||||
// eslint-disable-next-line no-prototype-builtins
|
||||
if (target.prototype.hasOwnProperty(i)) continue
|
||||
if (targetProto.hasOwnProperty(i)) continue
|
||||
|
||||
// copy getters
|
||||
if (origin.prototype.__lookupGetter__(i)) {
|
||||
target.prototype.__defineGetter__(
|
||||
i,
|
||||
origin.prototype.__lookupGetter__(i)
|
||||
)
|
||||
} else {
|
||||
target.prototype[i] = origin.prototype[i]
|
||||
}
|
||||
|
||||
// and setters
|
||||
if (origin.prototype.__lookupSetter__(i)) {
|
||||
target.prototype.__defineSetter__(
|
||||
i,
|
||||
origin.prototype.__lookupSetter__(i)
|
||||
)
|
||||
// Use Object.getOwnPropertyDescriptor to copy getters/setters properly
|
||||
const descriptor = Object.getOwnPropertyDescriptor(originProto, i)
|
||||
if (descriptor) {
|
||||
Object.defineProperty(targetProto, i, descriptor)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -30,7 +30,7 @@ export class MapProxyHandler<V> implements ProxyHandler<
|
||||
return [...target.keys()].map(String)
|
||||
}
|
||||
|
||||
get(target: Map<number | string, V>, p: string | symbol): any {
|
||||
get(target: Map<number | string, V>, p: string | symbol): V | undefined {
|
||||
// Workaround does not support link IDs of "values", "entries", "constructor", etc.
|
||||
if (p in target) return Reflect.get(target, p, target)
|
||||
if (typeof p === 'symbol') return
|
||||
@@ -42,7 +42,7 @@ export class MapProxyHandler<V> implements ProxyHandler<
|
||||
set(
|
||||
target: Map<number | string, V>,
|
||||
p: string | symbol,
|
||||
newValue: any
|
||||
newValue: V
|
||||
): boolean {
|
||||
if (typeof p === 'symbol') return false
|
||||
|
||||
@@ -55,7 +55,7 @@ export class MapProxyHandler<V> implements ProxyHandler<
|
||||
return target.delete(p as number | string)
|
||||
}
|
||||
|
||||
static bindAllMethods(map: Map<any, any>): void {
|
||||
static bindAllMethods(map: Map<unknown, unknown>): void {
|
||||
map.clear = map.clear.bind(map)
|
||||
map.delete = map.delete.bind(map)
|
||||
map.forEach = map.forEach.bind(map)
|
||||
|
||||
@@ -22,7 +22,7 @@ LiteGraphGlobal {
|
||||
"CurveEditor": [Function],
|
||||
"DEFAULT_FONT": "Inter",
|
||||
"DEFAULT_GROUP_FONT": 24,
|
||||
"DEFAULT_GROUP_FONT_SIZE": undefined,
|
||||
"DEFAULT_GROUP_FONT_SIZE": 24,
|
||||
"DEFAULT_POSITION": [
|
||||
100,
|
||||
100,
|
||||
|
||||
@@ -62,7 +62,7 @@ export interface LGraphNodeConstructor<T extends LGraphNode = LGraphNode> {
|
||||
size?: Size
|
||||
min_height?: number
|
||||
slot_start_y?: number
|
||||
widgets_info?: any
|
||||
widgets_info?: Record<string, unknown>
|
||||
collapsable?: boolean
|
||||
color?: string
|
||||
bgcolor?: string
|
||||
|
||||
@@ -343,7 +343,7 @@ export interface IBaseWidget<
|
||||
|
||||
// TODO: Confirm this format
|
||||
callback?(
|
||||
value: any,
|
||||
value: unknown,
|
||||
canvas?: LGraphCanvas,
|
||||
node?: LGraphNode,
|
||||
pos?: Point,
|
||||
|
||||
@@ -78,7 +78,7 @@ export abstract class BaseWidget<
|
||||
tooltip?: string
|
||||
element?: HTMLElement
|
||||
callback?(
|
||||
value: any,
|
||||
value: TWidget['value'],
|
||||
canvas?: LGraphCanvas,
|
||||
node?: LGraphNode,
|
||||
pos?: Point,
|
||||
|
||||
@@ -65,7 +65,7 @@ export class ButtonWidget
|
||||
this.clicked = true
|
||||
canvas.setDirty(true)
|
||||
|
||||
// Call the callback with widget instance and other context
|
||||
this.callback?.(this, canvas, node, pos, e)
|
||||
// Call the callback with widget value
|
||||
this.callback?.(this.value, canvas, node, pos, e)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user