From ffc8b44ed9bb3b7b322cb061942e829c8fdb4713 Mon Sep 17 00:00:00 2001 From: DrJKL Date: Fri, 17 Jul 2026 23:16:11 -0700 Subject: [PATCH] refactor: harden output.links deprecation, migrate last readers - setOutputData/setOutputDataType and repairPrimitive read the link store via helpers instead of the warning getter, keeping first-party code out of the extension deprecation telemetry - freeze the getter's returned array so extension push/splice throws instead of silently discarding; narrow the declare to drop undefined - add behavioral tests for the deprecation getter - docs: pseudocode and floating-link claim match shipped buildOutputIndex, migration map gains enumerate-links entry, Decision 5 precedes 6, ECS plan Phase 3c/4b status marks output-side work done --- docs/architecture/ecs-migration-plan.md | 14 +++- docs/architecture/output-slot-connectivity.md | 51 ++++++------ .../migration/proxyWidgetMigration.ts | 21 +++-- src/lib/litegraph/src/LGraphNode.ts | 18 +---- .../litegraph/src/node/NodeOutputSlot.test.ts | 81 +++++++++++++++++++ src/lib/litegraph/src/node/NodeOutputSlot.ts | 6 +- 6 files changed, 134 insertions(+), 57 deletions(-) create mode 100644 src/lib/litegraph/src/node/NodeOutputSlot.test.ts diff --git a/docs/architecture/ecs-migration-plan.md b/docs/architecture/ecs-migration-plan.md index 8d6c701bf8..c718872707 100644 --- a/docs/architecture/ecs-migration-plan.md +++ b/docs/architecture/ecs-migration-plan.md @@ -277,9 +277,14 @@ data is complete and consistent with the class-based graph. > `rerouteStore` (PRs 13436, 13449): "what links pass through this reroute" is > derived per root graph by a cached reverse index over the links' `parentId` > chains, and input-side connectivity is one lookup via -> `linkStore.isInputSlotConnected()` / `getInputSlotLink()`. Remaining: -> slot mirrors (`input.link` / `output.links`), output-side queries, and -> execution order. +> `linkStore.isInputSlotConnected()` / `getInputSlotLink()`. +> +> **Status (2026-07-17):** Output-side queries shipped and the `output.links` +> mirror is deleted (PR 13479): `linkStore.isOutputSlotConnected()` / +> `getOutputSlotLinks()`, litegraph internals reading through +> `node/slotLinks.ts` (see +> [output slot connectivity](output-slot-connectivity.md)). Remaining: the +> `input.link` slot mirror and execution order. **Risk:** Low. Read-only system with equivalence tests. @@ -336,7 +341,8 @@ the system knowing about the callback API. > through canonical `LGraph` mutation chokepoints: `_addLink`/`_removeLink` and > `_addReroute`/`_removeReroute` pair every map mutation with store > (un)registration, and `clear()` / subgraph-definition GC unregister whole -> graphs. The callback contract above and slot-mirror extraction remain. +> graphs. The callback contract above and `input.link` slot-mirror extraction +> remain (`output.links` was deleted in PR 13479). **Risk:** High. Extensions depend on callback ordering and timing. Must be validated against real-world extensions. diff --git a/docs/architecture/output-slot-connectivity.md b/docs/architecture/output-slot-connectivity.md index 73f42e63b7..6b9216c9fa 100644 --- a/docs/architecture/output-slot-connectivity.md +++ b/docs/architecture/output-slot-connectivity.md @@ -69,7 +69,7 @@ function outputIndex(graphId: UUID): ComputedRef { const next = computed(() => { const index: OriginIndex = new Map() for (const t of graphTopologies(graphId)) { - if (t.originNodeId === UNASSIGNED_NODE_ID) continue + if (isFloatingTopology(t)) continue const key = originKey(t.originNodeId, t.originSlot) const links = index.get(key) ?? new Set() links.add(t) @@ -83,9 +83,10 @@ function outputIndex(graphId: UUID): ComputedRef { ``` The index spans both collections that `graphTopologies` yields, the keyed -targets and the unkeyed side set, so a floating link with an assigned -origin (one endpoint only) still reports its output as connected. A -`SUBGRAPH_INPUT_ID` origin indexes like any other id. +targets and the unkeyed side set. Floating links are skipped +(`isFloatingTopology`): the queries see fully-assigned links only, +matching the mirror they replace (Decision 6). A `SUBGRAPH_INPUT_ID` +origin indexes like any other id. ## Decision 3: Two public queries, mirroring the input pair @@ -123,6 +124,24 @@ a mirror read plus a `slotFloatingLinks` scan), widget value propagation (`widgetValuePropagation`), and matchType link revalidation (`dynamicWidgets.changeOutputType`). +## Decision 5: Wire connected state into the dots (the payoff) + +`NodeSlots.vue` passes `connected` to each slot: + +- input: `linkStore.isInputSlotConnected(rootGraphId, nodeId, index)` + (already available) +- output: `linkStore.isOutputSlotConnected(rootGraphId, nodeId, index)` + (Decision 3) + +`InputSlot` and `OutputSlot` already forward `connected` to the +`lg-slot--connected` class. `SlotConnectionDot` needs no prop of its own: +the wrapper class is an ancestor styling hook +(`.lg-slot--connected .slot-dot`), so the visual is one CSS rule away once +the design-standards check (open question 3) picks it. Threading a +`connected` prop into the dot before that would be plumbing with no +consumer. `compatible` stays driven by the existing drag state +(`useSlotLinkDragUIState`), which this phase leaves alone. + ## Decision 6: Delete the mirror (implemented) The runtime `output.links[]` field and all nine of its write sites are @@ -160,26 +179,10 @@ the pure helpers in `node/slotLinks.ts` (`outputHasLinks`, Extension migration map: presence → `node.isOutputConnected(slot)` / `slot.isConnected`; enumerate targets → `node.getOutputNodes(slot)`; -mutate → `node.connect(...)` / `node.disconnectOutput(slot, target?)`. -App/Vue code uses `useLinkStore().getOutputSlotLinks(...)` (reactive). - -## Decision 5: Wire connected state into the dots (the payoff) - -`NodeSlots.vue` passes `connected` to each slot: - -- input: `linkStore.isInputSlotConnected(rootGraphId, nodeId, index)` - (already available) -- output: `linkStore.isOutputSlotConnected(rootGraphId, nodeId, index)` - (Decision 3) - -`InputSlot` and `OutputSlot` already forward `connected` to the -`lg-slot--connected` class. `SlotConnectionDot` needs no prop of its own: -the wrapper class is an ancestor styling hook -(`.lg-slot--connected .slot-dot`), so the visual is one CSS rule away once -the design-standards check (open question 3) picks it. Threading a -`connected` prop into the dot before that would be plumbing with no -consumer. `compatible` stays driven by the existing drag state -(`useSlotLinkDragUIState`), which this phase leaves alone. +enumerate links → `outputLinks(graph, node.id, slot)` / `outputLinkIds` +(`node/slotLinks.ts`); mutate → `node.connect(...)` / +`node.disconnectOutput(slot, target?)`. App/Vue code uses +`useLinkStore().getOutputSlotLinks(...)` (reactive). ## Scope diff --git a/src/core/graph/subgraph/migration/proxyWidgetMigration.ts b/src/core/graph/subgraph/migration/proxyWidgetMigration.ts index ec52eec5f8..4a20a9d043 100644 --- a/src/core/graph/subgraph/migration/proxyWidgetMigration.ts +++ b/src/core/graph/subgraph/migration/proxyWidgetMigration.ts @@ -31,7 +31,7 @@ import { useLinkStore } from '@/stores/linkStore' import { usePreviewExposureStore } from '@/stores/previewExposureStore' import { useWidgetValueStore } from '@/stores/widgetValueStore' import type { LinkTopology } from '@/types/linkTopology' -import { UNASSIGNED_NODE_ID, toNodeId } from '@/types/nodeId' +import { toNodeId } from '@/types/nodeId' import type { NodeId, SerializedNodeId } from '@/types/nodeId' interface LegacyProxyEntrySource extends PromotedWidgetSource { @@ -624,17 +624,14 @@ function repairPrimitive( } const baseName = userRenamedTitle(primitiveNode) ?? validated.sourceWidgetName - const snapshot: SnapshotLink[] = (primitiveOutput.links ?? []) - .map((id) => subgraph.links.get(id)) - .filter( - (l): l is NonNullable => - l !== undefined && l.target_id !== UNASSIGNED_NODE_ID - ) - .map((l) => ({ - primitiveSlot: l.origin_slot, - targetNodeId: l.target_id, - targetSlot: l.target_slot - })) + const snapshot: SnapshotLink[] = primitiveOutputTopologies( + hostNode, + primitiveNode + ).map((topology) => ({ + primitiveSlot: topology.originSlot, + targetNodeId: topology.targetNodeId, + targetSlot: topology.targetSlot + })) let newSubgraphInput: SubgraphInput | undefined try { diff --git a/src/lib/litegraph/src/LGraphNode.ts b/src/lib/litegraph/src/LGraphNode.ts index 128c6effcc..0635a0b031 100644 --- a/src/lib/litegraph/src/LGraphNode.ts +++ b/src/lib/litegraph/src/LGraphNode.ts @@ -1128,13 +1128,8 @@ export class LGraphNode if (!this.graph) throw new NullGraphError() - // if there are connections, pass the data to the connections - const { links } = outputs[slot] - if (links) { - for (const id of links) { - const link = this.graph._links.get(id) - if (link) link.data = data - } + for (const link of outputLinks(this.graph, this.id, slot)) { + link.data = data } } @@ -1152,13 +1147,8 @@ export class LGraphNode if (!this.graph) throw new NullGraphError() - // if there are connections, pass the data to the connections - const { links } = outputs[slot] - if (links) { - for (const id of links) { - const link = this.graph._links.get(id) - if (link) link.type = type - } + for (const link of outputLinks(this.graph, this.id, slot)) { + link.type = type } } diff --git a/src/lib/litegraph/src/node/NodeOutputSlot.test.ts b/src/lib/litegraph/src/node/NodeOutputSlot.test.ts new file mode 100644 index 0000000000..bd8af0d3ce --- /dev/null +++ b/src/lib/litegraph/src/node/NodeOutputSlot.test.ts @@ -0,0 +1,81 @@ +import { createTestingPinia } from '@pinia/testing' +import { setActivePinia } from 'pinia' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import type { LinkId } from '@/lib/litegraph/src/LLink' +import { LGraph, LGraphNode, LiteGraph } from '@/lib/litegraph/src/litegraph' + +function createConnectedGraph() { + const graph = new LGraph() + const source = new LGraphNode('Source') + source.addOutput('out', 'INT') + graph.add(source) + + const target = new LGraphNode('Target') + target.addInput('in', 'INT') + graph.add(target) + + return { graph, source, target } +} + +describe('NodeOutputSlot deprecated links getter', () => { + const onWarning = vi.fn() + + beforeEach(() => { + setActivePinia(createTestingPinia({ stubActions: false })) + onWarning.mockClear() + LiteGraph.onDeprecationWarning = [onWarning] + LiteGraph.alwaysRepeatWarnings = true + }) + + afterEach(() => { + LiteGraph.alwaysRepeatWarnings = false + }) + + it('fires a deprecation warning on read', () => { + const { source } = createConnectedGraph() + + void source.outputs[0].links + + expect(onWarning).toHaveBeenCalledWith( + expect.stringContaining('output.links is deprecated'), + undefined + ) + }) + + it('reflects live link store data across connect and disconnect', () => { + const { source, target } = createConnectedGraph() + + const link = source.connect(0, target, 0) + expect(source.outputs[0].links).toEqual([link!.id]) + + source.disconnectOutput(0) + expect(source.outputs[0].links).toBeNull() + }) + + it('returns null for an unconnected slot and for a graphless node', () => { + const { source } = createConnectedGraph() + expect(source.outputs[0].links).toBeNull() + + const orphan = new LGraphNode('Orphan') + orphan.addOutput('out', 'INT') + expect(orphan.outputs[0].links).toBeNull() + }) + + it('throws on assignment', () => { + const { source } = createConnectedGraph() + const slot: { links?: unknown } = source.outputs[0] + + expect(() => { + slot.links = [] + }).toThrow(TypeError) + }) + + it('throws on mutation of the returned array', () => { + const { source, target } = createConnectedGraph() + source.connect(0, target, 0) + + const links = source.outputs[0].links as LinkId[] + expect(() => links.push(links[0])).toThrow(TypeError) + }) +}) diff --git a/src/lib/litegraph/src/node/NodeOutputSlot.ts b/src/lib/litegraph/src/node/NodeOutputSlot.ts index 6331e0a7a8..dea285e33c 100644 --- a/src/lib/litegraph/src/node/NodeOutputSlot.ts +++ b/src/lib/litegraph/src/node/NodeOutputSlot.ts @@ -21,7 +21,7 @@ import { warnDeprecated } from '@/lib/litegraph/src/utils/feedback' export class NodeOutputSlot extends NodeSlot implements INodeOutputSlot { /** @deprecated Derived from the link store via a warning prototype getter; never written. */ - declare readonly links?: readonly LinkId[] | null + declare readonly links: readonly LinkId[] | null _data?: unknown slot_index?: number @@ -110,7 +110,7 @@ export class NodeOutputSlot extends NodeSlot implements INodeOutputSlot { * writes throw in strict mode. First-party code uses the slotLinks helpers. */ Object.defineProperty(NodeOutputSlot.prototype, 'links', { - get(this: NodeOutputSlot): LinkId[] | null { + get(this: NodeOutputSlot): readonly LinkId[] | null { warnDeprecated( 'output.links is deprecated. Read connectivity via node.isOutputConnected(slot) / node.getOutputNodes(slot); mutate via node.connect() / node.disconnectOutput().' ) @@ -121,7 +121,7 @@ Object.defineProperty(NodeOutputSlot.prototype, 'links', { this._node.id, this._node.outputs.indexOf(this) ) - return ids.length ? ids : null + return ids.length ? Object.freeze(ids) : null }, configurable: true, enumerable: false