Comfy-Org / Comfy-Org/ComfyUI_frontend

refactor(litegraph): add deprecation warning for direct graph._version writes

Open
#11,701 0 comments 0 reactions 1 assignee Claimed by @AustinMroz View on GitHub
developer experience Public API
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Summary

Follow-up to #11698 (ECS Migration Phase 0a). That PR centralized internal `LGraph._version` increments behind `incrementVersion()`. This issue tracks adding a deprecation warning for **external** writes to `graph._version` from the custom-node ecosystem, so authors migrate to `incrementVersion()` ahead of the future `VersionSystem` (ECS).

Per AGENTS.md ADR 0008, changes that affect 40+ custom node repos require migration guidance. A dedup'd deprecation warning is the right tool to deliver that guidance without breaking existing nodes.

## Proposed Implementation

Replace the public `_version: number` field on [`LGraph`](https://github.com/Comfy-Org/ComfyUI_frontend/blob/main/src/lib/litegraph/src/LGraph.ts) with a getter/setter pair plus a private backing field. The setter is **write-through** (zero behavior change) and emits a deduped warning via the existing [`warnDeprecated`](https://github.com/Comfy-Org/ComfyUI_frontend/blob/main/src/lib/litegraph/src/utils/feedback.ts) helper.

```ts
private __version = -1

get _version(): number {
return this.__version
}

set _version(v: number) {
warnDeprecated(
'Direct write to graph._version is deprecated. Use graph.incrementVersion() instead.',
this
)
this.__version = v // write-through: existing behavior preserved
}

incrementVersion(): void {
this.__version++
}

// Internal resets (e.g. clear()) write the backing field directly.
clear() {
// ...
this.__version = -1
}
```

### Key properties

- **Reads stay silent.** `LGraphCanvas.renderInfo()` and any custom node read paths don't trigger warnings.
- **Write-through, not redirect.** `graph._version++` and `graph._version = N` still do exactly what they did before — only the warning is new.
- **Dedup'd.** [`warnDeprecated`](https://github.com/Comfy-Org/ComfyUI_frontend/blob/main/src/lib/litegraph/src/utils/feedback.ts#L13-L27) emits each unique message at most once per session.
- **Internal call sites unaffected.** All in-tree writes already go through `incrementVersion()` after #11698; `clear()`'s `-1` reset uses the backing field.
- **Forward-compatible with ECS.** When `VersionSystem` lands, the setter is removed (or replaced with a thrown error) and `_version` becomes a getter that delegates to the World. Custom nodes will have had several versions of warnings to migrate.

## Alternatives Considered

- **Setter that redirects to `incrementVersion()`** (ignoring the assigned value). Rejected — silently changes behavior for nodes doing snapshot/restore.
- **ESLint `no-restricted-syntax` rule** on `_version` writes outside `LGraph.ts`. Useful internally but doesn't reach the custom-node ecosystem, which is the actual drift surface.
- **Do nothing.** The remaining drift surface is bounded and the ECS migration will eventually replace `_version` regardless. Acceptable but loses a cheap migration signal.

## Acceptance Criteria

- [ ] `LGraph._version` is implemented as a getter/setter over a private `__version` backing field
- [ ] Setter emits a deduped `warnDeprecated` message naming `incrementVersion()` as the replacement
- [ ] All in-tree writes that should not warn (`clear()` reset) use the backing field directly
- [ ] Reads do not emit warnings
- [ ] Unit test: writing `graph._version = N` triggers `LiteGraph.onDeprecationWarning`
- [ ] Unit test: calling `graph.incrementVersion()` does not trigger the warning
- [ ] Unit test: write-through preserves the assigned value (`graph._version = 5; expect(graph._version).toBe(5)`)

## Context

- Predecessor PR: #11698 (centralizes internal increments)
- Reviewer suggestion: https://github.com/Comfy-Org/ComfyUI_frontend/pull/11698#pullrequestreview-4184690993
- Linear: FE-165 (ECS Migration Phase 0a)
- Related: `docs/architecture/ecs-migration-plan.md`

┆Issue is synchronized with this [Notion page](https://www.notion.so/Issue-11701-refactor-litegraph-add-deprecation-warning-for-direct-graph-_version-writes-34f6d73d365081259521f5274677d6fd) by [Unito](https://www.unito.io)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.