mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-24 14:45:36 +00:00
fix: address World substrate review feedback
- shallowReactive outer registry so first-bucket creation is observable - dev-mode collision guard for ComponentKey names - drop redundant `as string` casts in widgetValueStore.clearGraph - rename misleading reactive-bridging test, add stable-proxy invariant test - reword identity claims to match actual reactive(Map) proxy semantics Amp-Thread-ID: https://ampcode.com/threads/T-019dd61d-7103-737b-8dfb-be8cc784fc2d Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -1,5 +1,9 @@
|
||||
# Appendix: ECS Pattern Survey
|
||||
|
||||
_Note: cross-references to `temp/plans/world-consolidation.md` point to a
|
||||
working planning document that may not be present in all checkouts. The
|
||||
load-bearing claims are reproduced inline below._
|
||||
|
||||
_A survey of mainstream Entity Component System libraries — bitECS, miniplex,
|
||||
koota, ECSY, and Bevy — captured during the world-consolidation analysis that
|
||||
shipped slice 1 of [ADR 0008](../adr/0008-entity-component-system.md). This
|
||||
@@ -139,9 +143,13 @@ either would break
|
||||
[BaseWidget.\_state](../../src/lib/litegraph/src/widgets/BaseWidget.ts)
|
||||
shared reactive identity — the contract that lets DOM widget overrides,
|
||||
`useProcessedWidgets` memoization, and the 40+ extension ecosystem all
|
||||
read the same proxy. Our `setComponent(id, key, sharedRef)` is
|
||||
identity-preserving by design, and `widgetValueStore.registerWidget`
|
||||
returns the same proxy that `getComponent` returns
|
||||
read the same proxy. Our `setComponent(id, key, ref)` stores by reference
|
||||
and the inner `reactive(Map)` keeps a stable cached proxy per
|
||||
entity-component pair: every `getComponent` returns the same proxy,
|
||||
regardless of how many writes intervene. `widgetValueStore.registerWidget`
|
||||
returns that proxy (not the caller's input ref), so `BaseWidget._state`
|
||||
and every other reader observe the same object. Replace-on-write idioms
|
||||
would swap the cached proxy on each write and break that stability
|
||||
([temp/plans/world-consolidation.md §3.1 step
|
||||
6](../../temp/plans/world-consolidation.md), reactive-identity test).
|
||||
|
||||
@@ -166,15 +174,19 @@ world-consolidation plan — copied here for proximity):
|
||||
|
||||
```ts
|
||||
/**
|
||||
* Storage strategy: AoS (per-entity reactive object reference) backed by
|
||||
* `reactive(Map)`. Component values are stored by reference; mutating a
|
||||
* value's fields propagates to all readers through Vue's reactive proxy.
|
||||
* `setComponent(id, key, ref)` is intentionally identity-preserving.
|
||||
* `setComponent` stores values by reference (no clone). The inner
|
||||
* `reactive(Map)` produces a single cached Vue proxy per entity-component
|
||||
* pair: every `getComponent` call returns the same proxy, and mutations
|
||||
* through it propagate to all readers. Note that the proxy is NOT `===`
|
||||
* to the raw object passed to `setComponent` — read through `getComponent`
|
||||
* (or a `registerWidget`-style helper that does so internally) and treat
|
||||
* that proxy as canonical.
|
||||
*
|
||||
* NOT a sparse-set / archetype store. A future SoA migration would break
|
||||
* the shared-reactive-identity contract that BaseWidget._state and the
|
||||
* widgetValueStore facade rely on; do not refactor without revisiting
|
||||
* those consumers.
|
||||
* `BaseWidget._state` and `widgetValueStore` rely on this stable-proxy
|
||||
* invariant. Replace-on-write idioms (koota's `entity.set(...)`,
|
||||
* miniplex's `world.add(entity)`) would swap the cached proxy on each
|
||||
* write and break the contract; revisiting either consumer is required
|
||||
* before changing storage semantics.
|
||||
*/
|
||||
```
|
||||
|
||||
|
||||
@@ -1,11 +1,7 @@
|
||||
import { defineComponentKey } from '@/world/componentKey'
|
||||
import type { NodeEntityId, WidgetEntityId } from '@/world/entityIds'
|
||||
|
||||
/**
|
||||
* `WidgetState` collapses to `WidgetValue` at the component-key boundary;
|
||||
* the same reactive object reference is shared with `useWidgetValueStore`
|
||||
* so Vue tracking is preserved across both read paths.
|
||||
*/
|
||||
/** `WidgetState` collapses to `WidgetValue` at the component-key boundary. */
|
||||
export interface WidgetValue {
|
||||
value: unknown
|
||||
}
|
||||
|
||||
@@ -97,12 +97,12 @@ export const useWidgetValueStore = defineStore('widgetValue', () => {
|
||||
const widgetPrefix = graphWidgetPrefix(branded)
|
||||
const nodePrefix = graphNodePrefix(branded)
|
||||
for (const widgetId of world.entitiesWith(WidgetValueComponent)) {
|
||||
if ((widgetId as string).startsWith(widgetPrefix)) {
|
||||
if (widgetId.startsWith(widgetPrefix)) {
|
||||
world.removeComponent(widgetId, WidgetValueComponent)
|
||||
}
|
||||
}
|
||||
for (const nodeId of world.entitiesWith(WidgetContainerComponent)) {
|
||||
if ((nodeId as string).startsWith(nodePrefix)) {
|
||||
if (nodeId.startsWith(nodePrefix)) {
|
||||
world.removeComponent(nodeId, WidgetContainerComponent)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,8 +14,17 @@ export interface ComponentKey<TData, TEntity extends EntityId> {
|
||||
readonly [componentKeyEntity]?: TEntity
|
||||
}
|
||||
|
||||
const registeredNames = new Set<string>()
|
||||
|
||||
export function defineComponentKey<TData, TEntity extends EntityId>(
|
||||
name: string
|
||||
): ComponentKey<TData, TEntity> {
|
||||
if (import.meta.env.DEV && registeredNames.has(name)) {
|
||||
console.error(
|
||||
`[world] ComponentKey name collision: "${name}" was already registered. ` +
|
||||
`Two keys with the same name share storage and will silently overwrite each other.`
|
||||
)
|
||||
}
|
||||
registeredNames.add(name)
|
||||
return { name } as ComponentKey<TData, TEntity>
|
||||
}
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { computed } from 'vue'
|
||||
|
||||
import { defineComponentKey } from './componentKey'
|
||||
import type { NodeEntityId, WidgetEntityId } from './entityIds'
|
||||
@@ -29,7 +30,7 @@ describe('createWorld', () => {
|
||||
expect(world.getComponent(widgetId, TestWidgetThing)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('preserves shared object identity (reactive bridging)', () => {
|
||||
it('propagates mutations through the stored proxy', () => {
|
||||
const world = createWorld()
|
||||
const widgetId = widgetEntityId(graphId, 1, 'seed')
|
||||
const data = { value: 42 }
|
||||
@@ -38,6 +39,28 @@ describe('createWorld', () => {
|
||||
expect(world.getComponent(widgetId, TestWidgetThing)?.value).toBe(99)
|
||||
})
|
||||
|
||||
it('returns the same proxy across reads of the same (id, key)', () => {
|
||||
const world = createWorld()
|
||||
const widgetId = widgetEntityId(graphId, 1, 'seed')
|
||||
world.setComponent(widgetId, TestWidgetThing, { value: 42 })
|
||||
|
||||
const a = world.getComponent(widgetId, TestWidgetThing)
|
||||
const b = world.getComponent(widgetId, TestWidgetThing)
|
||||
expect(a).toBe(b)
|
||||
})
|
||||
|
||||
it('reacts when subscribing before the first component for a key exists', () => {
|
||||
const world = createWorld()
|
||||
const widgetId = widgetEntityId(graphId, 1, 'seed')
|
||||
const observed = computed(
|
||||
() => world.getComponent(widgetId, TestWidgetThing)?.value
|
||||
)
|
||||
|
||||
expect(observed.value).toBeUndefined()
|
||||
world.setComponent(widgetId, TestWidgetThing, { value: 42 })
|
||||
expect(observed.value).toBe(42)
|
||||
})
|
||||
|
||||
it('iterates entities for a given component key', () => {
|
||||
const world = createWorld()
|
||||
const a = widgetEntityId(graphId, 1, 'seed')
|
||||
|
||||
@@ -1,13 +1,12 @@
|
||||
import { reactive } from 'vue'
|
||||
import { reactive, shallowReactive } from 'vue'
|
||||
|
||||
import type { ComponentKey } from './componentKey'
|
||||
import type { EntityId } from './entityIds'
|
||||
|
||||
/**
|
||||
* `setComponent` is identity-preserving — `BaseWidget._state` and
|
||||
* `widgetValueStore` rely on the shared reactive object reference.
|
||||
* Storage changes that break this contract require revisiting both
|
||||
* consumers first.
|
||||
* `setComponent` stores by reference; `getComponent` returns a Vue proxy
|
||||
* cached per `(id, key)`. The proxy is stable across reads and is NOT
|
||||
* `===` to the input. Treat `getComponent` as the canonical read path.
|
||||
*/
|
||||
export interface World {
|
||||
getComponent<TData, TEntity extends EntityId>(
|
||||
@@ -29,7 +28,8 @@ export interface World {
|
||||
}
|
||||
|
||||
export function createWorld(): World {
|
||||
const store = new Map<string, Map<EntityId, unknown>>()
|
||||
// shallowReactive so first-bucket creation is observable to subscribers.
|
||||
const store = shallowReactive(new Map<string, Map<EntityId, unknown>>())
|
||||
|
||||
return {
|
||||
getComponent<TData, TEntity extends EntityId>(
|
||||
|
||||
Reference in New Issue
Block a user