-
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
Closed
Closed
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
3858914
[feat] Add property instrumentation for node changes
benceruleanlu d982a40
Update test snapshots (?)
benceruleanlu 4671ea3
Add basic unit tests
benceruleanlu 838a185
Change to generic approach
benceruleanlu ae7577e
Improve property instrumentation to preserve existing object state
benceruleanlu ddd95da
Move instr to constr
benceruleanlu c0fe0fa
Refactor property instrumentation: consolidate into LGraphNodePropert…
benceruleanlu 727e926
Fix type errors
benceruleanlu 3524e66
nit
benceruleanlu 8108ecc
Remove default property
benceruleanlu f35fbb3
nit
benceruleanlu b59bf7e
Move test file
benceruleanlu caee207
Rename propertyManager to changeTracker
benceruleanlu baa4c83
Update test path
benceruleanlu c9b4861
Revert shotgun tsc changes
benceruleanlu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| import type { LGraphNode } from "./LGraphNode" | ||
|
|
||
| /** | ||
| * Default properties to track | ||
| */ | ||
| const DEFAULT_TRACKED_PROPERTIES: string[] = [ | ||
| "title", | ||
| "flags.collapsed", | ||
| ] | ||
|
|
||
| /** | ||
| * Manages node properties with optional change tracking and instrumentation. | ||
| */ | ||
| export class LGraphNodeProperties { | ||
| /** The node this property manager belongs to */ | ||
| node: LGraphNode | ||
|
|
||
| /** Set of property paths that have been instrumented */ | ||
| #instrumentedPaths = new Set<string>() | ||
|
|
||
| constructor(node: LGraphNode) { | ||
| this.node = node | ||
|
|
||
| this.#setupInstrumentation() | ||
| } | ||
|
|
||
| /** | ||
| * Sets up property instrumentation for all tracked properties | ||
| */ | ||
| #setupInstrumentation(): void { | ||
| for (const path of DEFAULT_TRACKED_PROPERTIES) { | ||
| this.#instrumentProperty(path) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Instruments a single property to track changes | ||
| */ | ||
| #instrumentProperty(path: string): void { | ||
| const parts = path.split(".") | ||
|
|
||
| if (parts.length > 1) { | ||
| 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 hasProperty = Object.prototype.hasOwnProperty.call(targetObject, propertyName) | ||
| const currentValue = targetObject[propertyName] | ||
|
|
||
| if (!hasProperty) { | ||
| let value: any = undefined | ||
|
|
||
| Object.defineProperty(targetObject, propertyName, { | ||
| get: () => value, | ||
benceruleanlu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| set: (newValue: any) => { | ||
| const oldValue = value | ||
| value = newValue | ||
| this.#emitPropertyChange(path, oldValue, newValue) | ||
|
|
||
| // Update enumerable: true for non-undefined values, false for undefined | ||
| const shouldBeEnumerable = newValue !== undefined | ||
| const currentDescriptor = Object.getOwnPropertyDescriptor(targetObject, propertyName) | ||
| if (currentDescriptor && currentDescriptor.enumerable !== shouldBeEnumerable) { | ||
| Object.defineProperty(targetObject, propertyName, { | ||
| ...currentDescriptor, | ||
| enumerable: shouldBeEnumerable, | ||
| }) | ||
| } | ||
| }, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }) | ||
| } else { | ||
| Object.defineProperty( | ||
| targetObject, | ||
| propertyName, | ||
| this.#createInstrumentedDescriptor(path, currentValue), | ||
| ) | ||
| } | ||
|
|
||
| this.#instrumentedPaths.add(path) | ||
| } | ||
|
|
||
| /** | ||
| * Creates a property descriptor that emits change events | ||
| */ | ||
| #createInstrumentedDescriptor(propertyPath: string, initialValue: any): PropertyDescriptor { | ||
| let value = initialValue | ||
|
|
||
| return { | ||
| get: () => value, | ||
| set: (newValue: any) => { | ||
| const oldValue = value | ||
| value = newValue | ||
| this.#emitPropertyChange(propertyPath, oldValue, newValue) | ||
| }, | ||
| enumerable: true, | ||
| configurable: true, | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Emits a property change event if the node is connected to a graph | ||
| */ | ||
| #emitPropertyChange(propertyPath: string, oldValue: any, newValue: any): void { | ||
| if (oldValue !== newValue && this.node.graph) { | ||
| this.node.graph.trigger("node:property:changed", { | ||
| nodeId: this.node.id, | ||
| property: propertyPath, | ||
| oldValue, | ||
| newValue, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Ensures parent objects exist for nested properties | ||
| */ | ||
| #ensureNestedPath(path: string): void { | ||
| const parts = path.split(".") | ||
| let current: any = this.node | ||
|
|
||
| // Create all parent objects except the last property | ||
| for (let i = 0; i < parts.length - 1; i++) { | ||
| const part = parts[i] | ||
| if (!current[part]) { | ||
| current[part] = {} | ||
| } | ||
| current = current[part] | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a property is being tracked | ||
| */ | ||
| isTracked(path: string): boolean { | ||
| return this.#instrumentedPaths.has(path) | ||
| } | ||
|
|
||
| /** | ||
| * Gets the list of tracked properties | ||
| */ | ||
| getTrackedProperties(): string[] { | ||
| return [...DEFAULT_TRACKED_PROPERTIES] | ||
| } | ||
|
|
||
| /** | ||
| * Custom toJSON method for JSON.stringify | ||
| * Returns undefined to exclude from serialization since we only use defaults | ||
| */ | ||
| toJSON(): any { | ||
| return undefined | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -521,6 +521,7 @@ export class LiteGraphGlobal { | |
|
|
||
| // callback | ||
| node.onNodeCreated?.() | ||
|
|
||
| return node | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest" | ||
|
|
||
| import { LGraphNodeProperties } from "@/LGraphNodeProperties" | ||
|
|
||
| describe("LGraphNodeProperties", () => { | ||
| let mockNode: any | ||
| let mockGraph: any | ||
|
|
||
| beforeEach(() => { | ||
| mockGraph = { | ||
| trigger: vi.fn(), | ||
| } | ||
|
|
||
| mockNode = { | ||
| id: 123, | ||
| title: "Test Node", | ||
| flags: {}, | ||
| graph: mockGraph, | ||
| } | ||
| }) | ||
|
|
||
| describe("constructor", () => { | ||
| it("should initialize with default tracked properties", () => { | ||
| const propManager = new LGraphNodeProperties(mockNode) | ||
| const tracked = propManager.getTrackedProperties() | ||
|
|
||
| expect(tracked).toHaveLength(2) | ||
| expect(tracked).toContain("title") | ||
| expect(tracked).toContain("flags.collapsed") | ||
| }) | ||
| }) | ||
|
|
||
| describe("property tracking", () => { | ||
| it("should track changes to existing properties", () => { | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| mockNode.title = "New Title" | ||
|
|
||
| expect(mockGraph.trigger).toHaveBeenCalledWith("node:property:changed", { | ||
| nodeId: mockNode.id, | ||
| property: "title", | ||
| oldValue: "Test Node", | ||
| newValue: "New Title", | ||
| }) | ||
| }) | ||
|
|
||
| it("should track changes to nested properties", () => { | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| mockNode.flags.collapsed = true | ||
|
|
||
| expect(mockGraph.trigger).toHaveBeenCalledWith("node:property:changed", { | ||
| nodeId: mockNode.id, | ||
| property: "flags.collapsed", | ||
| oldValue: undefined, | ||
| newValue: true, | ||
| }) | ||
| }) | ||
|
|
||
| it("should not emit events when value doesn't change", () => { | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| mockNode.title = "Test Node" // Same value as original | ||
|
|
||
| expect(mockGraph.trigger).toHaveBeenCalledTimes(0) | ||
| }) | ||
|
|
||
| it("should not emit events when node has no graph", () => { | ||
| mockNode.graph = null | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| // Should not throw | ||
| expect(() => { | ||
| mockNode.title = "New Title" | ||
| }).not.toThrow() | ||
| }) | ||
| }) | ||
|
|
||
| describe("isTracked", () => { | ||
| it("should correctly identify tracked properties", () => { | ||
| const propManager = new LGraphNodeProperties(mockNode) | ||
|
|
||
| expect(propManager.isTracked("title")).toBe(true) | ||
| expect(propManager.isTracked("flags.collapsed")).toBe(true) | ||
| expect(propManager.isTracked("untracked")).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe("serialization behavior", () => { | ||
| it("should not make non-existent properties enumerable", () => { | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| // flags.collapsed doesn't exist initially | ||
| const descriptor = Object.getOwnPropertyDescriptor(mockNode.flags, "collapsed") | ||
| expect(descriptor?.enumerable).toBe(false) | ||
| }) | ||
|
|
||
| it("should make properties enumerable when set to non-default values", () => { | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| mockNode.flags.collapsed = true | ||
|
|
||
| const descriptor = Object.getOwnPropertyDescriptor(mockNode.flags, "collapsed") | ||
| expect(descriptor?.enumerable).toBe(true) | ||
| }) | ||
|
|
||
| it("should make properties non-enumerable when set back to undefined", () => { | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| mockNode.flags.collapsed = true | ||
| mockNode.flags.collapsed = undefined | ||
|
|
||
| const descriptor = Object.getOwnPropertyDescriptor(mockNode.flags, "collapsed") | ||
| expect(descriptor?.enumerable).toBe(false) | ||
| }) | ||
|
|
||
| it("should keep existing properties enumerable", () => { | ||
| // title exists initially | ||
| const initialDescriptor = Object.getOwnPropertyDescriptor(mockNode, "title") | ||
| expect(initialDescriptor?.enumerable).toBe(true) | ||
|
|
||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| const afterDescriptor = Object.getOwnPropertyDescriptor(mockNode, "title") | ||
| expect(afterDescriptor?.enumerable).toBe(true) | ||
| }) | ||
|
|
||
| it("should only include non-undefined values in JSON.stringify", () => { | ||
| new LGraphNodeProperties(mockNode) | ||
|
|
||
| // Initially, flags.collapsed shouldn't appear | ||
| let json = JSON.parse(JSON.stringify(mockNode)) | ||
| expect(json.flags.collapsed).toBeUndefined() | ||
|
|
||
| // After setting to true, it should appear | ||
| mockNode.flags.collapsed = true | ||
| json = JSON.parse(JSON.stringify(mockNode)) | ||
| expect(json.flags.collapsed).toBe(true) | ||
|
|
||
| // After setting to false, it should still appear (false is not undefined) | ||
| mockNode.flags.collapsed = false | ||
| json = JSON.parse(JSON.stringify(mockNode)) | ||
| expect(json.flags.collapsed).toBe(false) | ||
|
|
||
| // After setting back to undefined, it should disappear | ||
| mockNode.flags.collapsed = undefined | ||
| json = JSON.parse(JSON.stringify(mockNode)) | ||
| expect(json.flags.collapsed).toBeUndefined() | ||
| }) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.