Comfy-Org / Comfy-Org/ComfyUI_frontend

Audit: Replace plain `string` node ID typings with shared `NodeId` type (pre-work for adr008)

Open
#11,428 1 comment 0 reactions 1 assignee Claimed by @christian-byrne View on GitHub
developer experience
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Summary

This is an audit/cleanup task to replace bare `string` (and Vue prop `String`) typings for node IDs throughout the codebase with the appropriate shared `NodeId` type. This will make it significantly easier to implement adr008 (centralized stable identification) later, because a single type change will propagate everywhere rather than requiring a grep-and-replace across 150+ sites.

## Background

Identified during code review on PR #11423 (comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/11423#discussion_r3107604798).

Requested by: @christian-byrne

## Current State — Three competing `NodeId` definitions

There are currently **three** separate `NodeId` type aliases in the codebase, which leads to inconsistent imports and silent type mismatches:

| Source | Definition |
|---|---|
| `src/lib/litegraph/src/LGraphNode.ts` | `type NodeId = number \| string` |
| `src/platform/workflow/validation/schemas/workflowSchema.ts` | `type NodeId = z.infer` → `number \| string` |
| `src/renderer/core/layout/types.ts` | `type NodeId = string` |

Files already importing a shared `NodeId` do so inconsistently (some from litegraph, some from workflowSchema, some from layout/types).

## Scope

A quick search shows **~151 sites** where `nodeId` is typed as a bare `string` or Vue prop `String` in `.ts` files alone (not counting `.vue` SFCs). Examples include:

- Vue component `nodeId` props typed as `String` instead of the shared type
- Function parameters typed as `string` for node IDs
- Store state/getters using `string` for node IDs
- Composable arguments typed as `string` for node IDs

## Suggested Steps

1. **Decide on a single canonical `NodeId` type** (or a small layered set, e.g., `LGraphNodeId` vs `RendererNodeId`) and document it — ideally as part of or alongside adr008.
2. **Consolidate** the three existing definitions into the canonical location and re-export from the others for backward compatibility during migration.
3. **Replace** all bare `string`/`String` node-ID typings with the canonical type.
4. **Update Vue component props** (`nodeId: { type: String }`) to use the canonical type with a runtime validator or typed prop where feasible.
5. **CI guard** — consider adding a lint rule or type alias to prevent regressions.

## References

- PR #11423 — fix: include actual slot index in InputSlot/OutputSlot keys (where this was first noticed)
- adr008 — centralized stable identification (future ADR)

┆Issue is synchronized with this [Notion page](https://www.notion.so/Issue-11428-Audit-Replace-plain-string-node-ID-typings-with-shared-NodeId-type-pre-work-for-3476d73d36508108ae66cbf2eb2c80f4) 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.