-
Notifications
You must be signed in to change notification settings - Fork 66
[feat] Add property instrumentation for node changes #1136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
LMK if we don't want to have some things be defined and be false, like flags collapsable false as the new updated test snapshots instead of an empty flags. I can change it easily, it'll just look a touch ugly perhaps. |
Do not want this, sorry! Having it defined causes serialisation bloat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if these changes would be easier if implemented as a class (e.g. LGraphNodeProperties) that nodes instantiate on construction.
Similar to how functionality is split into child classes of LGraphCanvas. It seems to fit the existing OO pattern more closely. Thoughts?
Edit: Lacking context - I moved this from a code comment on the use of Object.defineProperty.
SG, I don't mind either way. |
|
there's gotta be a pre-push hook for tsc... edit: nvm no clue how tsc didn't catch it when I committed |
Oh this was from a live review conversation. I meant if switching to using a Proxy. |
Distinguish between existing and non-existing properties when instrumenting nodes. Properties that already exist maintain their enumerable status and values, while tracked properties that don't exist yet are created as non-enumerable with undefined initial values. This prevents unintended initialization side effects while maintaining proper change tracking. 🤖 Generated with [opencode](https://opencode.ai) Co-Authored-By: opencode <[email protected]>
…ies and remove legacy code
76c5cde to
baa4c83
Compare
|
Rebased, cleaned some things up. Think this can go in, we can always switch to a vue/proxy approach later. |
webfiltered
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can go into a testing branch for PoC / Vue immediately. There's a release dev version action, also.
If we want this in master we really have to consider the merits of a child class monkey patching the properties of its parent class, changing them from enumerable own props into non-enumerable own accessors.
There's no perfect hidden way to wrap an object, but using explicit, legible, and obvious code is significantly simpler to debug.
christian-byrne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vue Reactivity Integration Options for LiteGraph PR #1136
Here are 5 different approaches for integrating Vue reactivity with LiteGraph node properties, with varying levels of complexity and integration:
Option 1: Enhanced useLiteGraphNode Composable (Recommended)
import { shallowRef, triggerRef, effectScope } from '@vue/reactivity'
export function useLiteGraphNode(node: LGraphNode, watchedProps?: string[]) {
const nodeRef = shallowRef(node)
const scope = effectScope()
const defaultProps = ['title', 'pos', 'size', 'color', 'bgcolor', 'shape', 'mode']
const propsToWatch = new Set(watchedProps || defaultProps)
scope.run(() => {
propsToWatch.forEach(prop => {
const descriptor = Object.getOwnPropertyDescriptor(node, prop) ||
Object.getOwnPropertyDescriptor(node.constructor.prototype, prop)
Object.defineProperty(node, prop, {
get: descriptor?.get || function() { return this[`_${prop}`] },
set(value) {
const oldValue = this[prop]
if (descriptor?.set) {
descriptor.set.call(this, value)
} else {
this[`_${prop}`] = value
}
if (oldValue !== value) {
triggerRef(nodeRef) // Trigger Vue reactivity
}
},
configurable: true
})
})
})
return { nodeRef, dispose: () => scope.stop() }
}Explanation: Instruments specific node properties with Vue reactivity triggers. Uses shallowRef + triggerRef for optimal performance, only updating when properties actually change.
Option 2: Selective Property Binding (Low-level)
import { effectScope, ReactiveEffect } from '@vue/reactivity'
export function createSelectiveBinding<T extends object>(
target: T,
props: (keyof T)[]
) {
const scope = effectScope()
return scope.run(() => {
const bindings = {} as Record<keyof T, { value: T[keyof T] }>
props.forEach(prop => {
const effect = new ReactiveEffect(
() => target[prop],
() => {} // Trigger callback
)
bindings[prop] = {
get value() {
effect.track() // Track access for reactivity
return target[prop]
},
set value(newValue) {
target[prop] = newValue
effect.trigger() // Notify dependents
}
}
})
return { bindings, dispose: () => scope.stop() }
})
}Explanation: Uses Vue's low-level ReactiveEffect API to create individual reactive bindings per property. Gives fine-grained control over tracking and triggering.
Option 3: Widget Value Reactivity (For Private Fields)
import { ref, computed } from '@vue/reactivity'
export function useReactiveWidget(widget: BaseWidget) {
const valueRef = ref(widget.value)
// Hook into widget's callback system
const originalCallback = widget.callback
widget.callback = (value: unknown) => {
valueRef.value = value // Update Vue state
originalCallback?.call(widget, value) // Call original
}
return computed({
get: () => valueRef.value,
set: (value) => { widget.value = value }
})
}Explanation: Solves the private field problem by hooking into widget callbacks rather than trying to make private fields reactive. Creates two-way computed for Vue binding.
Option 4: Graph-Level Reactivity Integration
import { reactive, shallowReactive } from '@vue/reactivity'
export function useReactiveGraph(graph: LGraph) {
const graphState = reactive({
nodeCount: graph.nodes?.length || 0,
linkCount: graph.links?.length || 0,
version: graph._version || 0
})
const reactiveNodes = shallowReactive(new Map<string, any>())
// Hook into graph events
const originalOnNodeAdded = graph.onNodeAdded
graph.onNodeAdded = (node: LGraphNode) => {
const { nodeRef, dispose } = useLiteGraphNode(node)
reactiveNodes.set(String(node.id), { nodeRef, dispose })
graphState.nodeCount++
originalOnNodeAdded?.call(graph, node)
}
return { graphState, reactiveNodes }
}Explanation: Provides graph-level reactivity by hooking into LiteGraph's event system. Automatically makes new nodes reactive and tracks graph metadata changes.
Option 5: Reactive Property Proxy (Comprehensive)
import { reactive, isProxy } from '@vue/reactivity'
export function createReactiveNodeProxy(node: LGraphNode, trackedProps: string[]) {
// Create reactive data object
const reactiveData = reactive(
Object.fromEntries(
trackedProps.map(prop => [prop, node[prop]])
)
)
// Create proxy that bridges both objects
return new Proxy(node, {
get(target, prop) {
if (trackedProps.includes(prop as string)) {
return reactiveData[prop as string]
}
return Reflect.get(target, prop)
},
set(target, prop, value) {
if (trackedProps.includes(prop as string)) {
reactiveData[prop as string] = value
return Reflect.set(target, prop, value)
}
return Reflect.set(target, prop, value)
}
})
}Explanation: Creates a proxy that transparently makes specified properties reactive while keeping the rest of the node unchanged. Most comprehensive but highest overhead.
Recommendations
Primary Recommendation: Option 1 (Enhanced useLiteGraphNode)
- Clean Vue composable pattern
- Selective property tracking
- Automatic cleanup with effectScope
- Minimal performance overhead
Secondary: Option 3 (Widget Reactivity)
- Essential for handling private field limitations
- Solves the biggest technical blocker
- Works with existing widget architecture
Comparison Matrix
| Option | Vue Integration | Performance | Complexity | Private Field Support | Cleanup |
|---|---|---|---|---|---|
| 1. useLiteGraphNode | ⭐⭐⭐ Excellent | ⭐⭐⭐ Fast | ⭐⭐ Medium | ❌ No | ⭐⭐⭐ Auto |
| 2. Selective Binding | ⭐⭐⭐ Excellent | ⭐⭐ Moderate | ⭐ Complex | ❌ No | ⭐⭐⭐ Auto |
| 3. Widget Reactivity | ⭐⭐⭐ Excellent | ⭐⭐⭐ Fast | ⭐⭐⭐ Simple | ⭐⭐⭐ Yes | ⭐⭐ Manual |
| 4. Graph Integration | ⭐⭐ Good | ⭐⭐ Moderate | ⭐⭐ Medium | ❌ No | ⭐⭐⭐ Auto |
| 5. Reactive Proxy | ⭐⭐⭐ Excellent | ⭐ Slow | ⭐ Complex | ❌ No | ⭐ Manual |
Pros/Cons Breakdown
Option 1: useLiteGraphNode ✅ RECOMMENDED
Pros:
- Clean composable API familiar to Vue developers
- Selective property tracking reduces overhead
- Automatic cleanup with effectScope
- Works with existing LiteGraph architecture
Cons:
- Doesn't handle private fields (need Option 3 too)
- Requires Vue dependency
Option 2: Selective Binding
Pros:
- Fine-grained control over reactivity
- Uses Vue's low-level APIs efficiently
- Can create custom reactive patterns
Cons:
- More complex to implement and understand
- Steeper learning curve
- Still doesn't solve private fields
Option 3: Widget Reactivity ✅ ESSENTIAL
Pros:
- Solves the private field problem
- Works with existing widget callback system
- Simple and focused approach
Cons:
- Only handles widgets, not node properties
- Requires hooking into callback system
Option 4: Graph Integration
Pros:
- Comprehensive graph-level tracking
- Automatic node reactivity
- Good for large-scale applications
Cons:
- Higher complexity
- Potential memory overhead
- May be overkill for simple use cases
Option 5: Reactive Proxy
Pros:
- Most transparent to consumers
- Comprehensive property coverage
- Flexible property selection
Cons:
- Proxy overhead on every property access
- Complex to implement correctly
- Potential memory leaks if not cleaned up
Implementation Strategy
- Start with Option 1 for basic node property reactivity
- Add Option 3 to handle widget values and private fields
- Consider Option 4 for applications needing comprehensive graph tracking
- Use Options 2 & 5 for specialized use cases requiring custom behavior
This layered approach provides a solid foundation while allowing advanced users to opt into more sophisticated patterns as needed.
|
Closing due to being stale |
This PR instruments individual properties of LGraphNode with Object.defineProperty to emit events when node property changes.
Example implementation: Comfy-Org/ComfyUI_frontend#4387