Comfy-Org / Comfy-Org/ComfyUI_frontend

Two litegraph deprecation warnings bypass warnDeprecated(), so they cannot be observed, suppressed or deduped

Open
#15,711 0 comments 0 reactions 1 assignee Claimed by @christian-byrne View on GitHub
agent-ok area:testing developer experience Public API
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Problem

litegraph has a deprecation channel — `warnDeprecated()` — that dedupes per unique message, routes
through the public `LiteGraph.onDeprecationWarning` callback array, and can be made verbose with
`LiteGraph.alwaysRepeatWarnings`. Three call sites use it. **Two other deprecation warnings bypass it
entirely and call `console.warn` directly.**

The consequences are not cosmetic:

* **They cannot be observed or suppressed by consumers.** Anything that installs an
`onDeprecationWarning` callback (to count deprecated-API usage, to route it to telemetry, to
silence it in tests) sees these two warnings not at all.
* **They are not deduped.** The `widgets_up` one lives inside `LGraphNode.configure()`, so it fires
**once per node per workflow load** — a 40-node workflow using that property produces 40 identical
console lines every time it is opened. `warnDeprecated` would emit one per session.
* **They cannot be tested with the pattern the repo already uses** for every other deprecation
(`LiteGraph.onDeprecationWarning = [spy]`).

Both conversions are executed and green at `origin/main` `a08a7598aa`, with a failing-before,
passing-after test in each case and no change to the rest of the suite.

## Provenance

* **Found by:** deprecation-surface audit of `src/lib/litegraph/` · re-derived against `origin/main`
`a08a7598aa` on 2026-08-23
* **How:** enumerated every `console.warn` in `src/lib/litegraph/src` (91 non-test occurrences) and
cross-checked which of the deprecation-related ones route through `warnDeprecated`; wrote two probe
tests asserting the `onDeprecationWarning` callback fires; confirmed both fail, applied the
conversion, confirmed both pass, then ran the whole `src/lib/litegraph` suite and `vue-tsc` in both
arms
* **Why now:** the channel already exists and is already used; this is bringing two stragglers onto
it, with the tests to keep them there
* **Confidence:** verified by execution

## Evidence

All anchors opened at `origin/main` `a08a7598aa`.

**The channel.** `src/lib/litegraph/src/utils/feedback.ts:13`:

```ts
export function warnDeprecated(message: string, source?: object): void {
if (!LiteGraph.alwaysRepeatWarnings) {
if (sentWarnings.has(message)) return // :16 — dedupe by message
if (sentWarnings.size > UNIQUE_MESSAGE_LIMIT) return
sentWarnings.add(message)
}
for (const callback of LiteGraph.onDeprecationWarning) callback(message, source) // :24-26
}
```

`LiteGraph.onDeprecationWarning` is declared at `src/lib/litegraph/src/LiteGraphGlobal.ts:285` and
defaults to `[console.warn]`, so converting a site **preserves the current console output** for
anyone who has not overridden it. `alwaysRepeatWarnings` is at `:279`.

**Already on the channel** (the pattern to copy):

| Site | |
| ---- | --- |
| `src/lib/litegraph/src/LGraph.ts:1389` | `LGraph.onBeforeChange` deprecation |
| `src/lib/litegraph/src/LGraphNode.ts:3621` | `captureInput` deprecation |
| `src/lib/litegraph/src/widgets/ComboWidget.ts:132` | deprecated function values |

**Bypassing it — the two sites to convert:**

1. `src/lib/litegraph/src/LGraphNode.ts:1035-1041`, inside `configure()`:

```ts
if (this.widgets_up) {
console.warn(
`[LiteGraph] Node type "${this.type}" uses deprecated property "widgets_up". ` +
'This property is unsupported and will be removed. ' +
'Use "widgets_start_y" or a custom arrange() override instead.'
)
}
```

`warnDeprecated` is **already imported in this file** at `:100`.

Note the interpolated `${this.type}`. `feedback.ts:11` documents that the message
"**should not** include unique data; use `source`" — precisely because per-instance data defeats the
dedupe. So the conversion must make the message static and pass `this` as `source`. That is not
optional dressing; interpolating the type would give one console line per node type instead of one
per session.

2. `src/lib/litegraph/src/LGraphCanvas.ts:8705-8708`:

```ts
/** @deprecated */
getGroupMenuOptions(group: LGraphGroup) {
console.warn(
'LGraphCanvas.getGroupMenuOptions is deprecated, use LGraphGroup.getMenuOptions instead'
)
return group.getMenuOptions()
}
```

The message is already static. `LGraphCanvas.ts` does **not** currently import `warnDeprecated`; the
import goes next to the existing `./utils/*` imports around `:119-121`.

### Executed at `origin/main` `a08a7598aa`

Probe, in the style of the existing `LGraph.test.ts:595-608`:

```ts
LiteGraph.alwaysRepeatWarnings = true // the module-level dedupe is per test file, see below

it('routes the widgets_up deprecation through onDeprecationWarning', () => {
const cb = vi.fn()
LiteGraph.onDeprecationWarning = [cb]
const graph = new LGraph()
const node = new LGraphNode('probe')
graph.add(node)
node.widgets_up = true
node.configure({ id: node.id, type: 'probe' } as never)
expect(cb).toHaveBeenCalledWith(expect.stringContaining('widgets_up'), expect.anything())
})

it('routes the getGroupMenuOptions deprecation through onDeprecationWarning', () => {
const cb = vi.fn()
LiteGraph.onDeprecationWarning = [cb]
const canvas = Object.create(LGraphCanvas.prototype) as LGraphCanvas
canvas.getGroupMenuOptions(new LGraphGroup('g'))
expect(cb).toHaveBeenCalledWith(
expect.stringContaining('getGroupMenuOptions is deprecated'),
undefined
)
})
```

Before the conversion: `Tests 2 failed (2)`, both with `Number of calls: 0`.
After the conversion: `Tests 2 passed (2)`.

Full-suite arms, `vitest run src/lib/litegraph`:

| Arm | Result |
| --- | ------ |
| baseline, no change, no probes | `Test Files 71 passed (71)` · `Tests 1048 passed \| 3 expected fail \| 5 skipped` |
| conversion + both probes | `Test Files 71 passed (71)` · `Tests 1050 passed \| 3 expected fail \| 5 skipped` |

Delta is exactly the two probes. `NODE_OPTIONS=--max-old-space-size=8192 vue-tsc --noEmit` -> **exit
0, zero `error TS`** in both arms.

## Acceptance criteria

- [ ] `src/lib/litegraph/src/LGraphNode.ts:1036` uses `warnDeprecated(...)` instead of
`console.warn(...)`, with a **static** message and `this` passed as the `source` argument. The
node type must not be interpolated into the message string
- [ ] `src/lib/litegraph/src/LGraphCanvas.ts:8706` uses `warnDeprecated(...)` instead of
`console.warn(...)`, with the `warnDeprecated` import added alongside the existing
`./utils/*` imports
- [ ] A test for each, asserting the installed `LiteGraph.onDeprecationWarning` callback receives the
message. Use the existing pattern at `src/lib/litegraph/src/LGraph.test.ts:595-608`
- [ ] Both tests are mutation-verified: revert each conversion in turn and confirm its test — and
only its test — goes red. Report the before/after `vitest run src/lib/litegraph` counts in the PR
- [ ] `vitest run src/lib/litegraph` is green with the same `expected fail` (3) and `skipped` (5)
counts as before your change
- [ ] `NODE_OPTIONS=--max-old-space-size=8192 vue-tsc --noEmit` exits 0

## Out of scope

Do not do these in this PR. Each is a separate decision.

* **Do not change what the `widgets_up` message *says* about support status.** The current wording
claims the property "is unsupported", while the classic canvas renderer still honours it at
`LGraphNode.ts:1938` and `:4251`. Whether that wording is accurate is a maintainer's call about
deprecation policy, not a mechanical conversion. Keep the meaning as close to the existing text as
the static-message constraint allows, and raise the wording separately if you think it is wrong.
* **Do not touch `src/lib/litegraph/src/contextMenuCompat.ts:89-95`.** It is the third bare
`console.warn` deprecation, but it is deliberately different: it carries its own `hasWarned` dedupe
set and passes `%c` CSS styling arguments that `warnDeprecated`'s single-string signature cannot
convey. Converting it would silently drop the styling and duplicate the dedupe. Leave it alone.
* **Do not sweep the other ~88 `console.warn` calls in `src/lib/litegraph/src`.** Most are not
deprecations. This issue is scoped to the two deprecation warnings that bypass an existing channel.
* **Do not change `warnDeprecated`, `feedback.ts`, `alwaysRepeatWarnings`, or the default
`onDeprecationWarning` array.** The channel works; only its coverage is incomplete.
* Adjacent but unrelated: #11701 asks for a *new* deprecation warning on `graph._version` writes.
Different ask, do not fold it in.

## Working notes for whoever picks this up

Traps in this repo that will otherwise cost you a cycle:

* **`warnDeprecated` dedupes across the whole module session** via a module-level `Set` in
`feedback.ts:5`. Vitest isolates modules per test **file**, so one assertion per message per file
is safe — but a second test in the same file asserting the same message will see zero calls. Set
`LiteGraph.alwaysRepeatWarnings = true` in a `beforeEach` if you need more than one, and restore
`LiteGraph.onDeprecationWarning` if you follow `ComboWidget.test.ts:560,590`'s save/restore style.
* **Import litegraph classes from the barrel**, `@/lib/litegraph/src/litegraph`, never from
`'./LGraphNode'` directly. A deep import makes the suite die at collection with
`Class extends value undefined` and report **zero tests run**, which is easy to misread as green.
* **This is a pnpm workspace with eight `node_modules` directories** — the root plus
`apps/desktop-ui`, `apps/website`, and `packages/{design-system,object-info-parser,tailwind-utils,shared-frontend-utils,ingest-types}`.
With only the root linked, `vitest` dies at `Failed to resolve import "clsx"` **at every commit**,
which makes any comparison meaningless.
* **Run a positive control before trusting any red result.**
`vitest run src/lib/litegraph/src/LLink.test.ts` should report `Tests 3 passed (3)`.
* **Vitest 4 has no `basic` reporter.** `--reporter=basic` exits 1 with
`Failed to load custom Reporter from basic` before running anything. Omit the flag.
* **There is no root `vitest.config.ts`.** The config is `vite.config.mts`, read by default.
`vitest run --config vitest.config.ts` dies with `UNRESOLVED_ENTRY`.
* **`vue-tsc --noEmit` OOMs at Node's 2GB default heap and exits 134**, and a grep filtered for
`error TS` then returns zero — byte-identical to a clean run. Always pass
`NODE_OPTIONS=--max-old-space-size=8192` and always report the exit code.
* **`oxlint`'s exit code is ambiguous in both directions.** Most rules here run at warning level, so
exit 0 does not mean no findings; and it also exits 1 when every input path is ignored
(`No files found to lint`). Read the output, not just `$?`.
* The suite in this tree has **3 `it.fails` tests that are expected to fail**. A run reporting
`3 expected fail` is green. Do not "fix" them.

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.