fix(core): key the snapshot cache on every input it derives from
Copilot review round 1. The cache was keyed on the store and its revision alone, so a changed `read` was answered with the previous reader's output, and with a null store — where the revision is pinned at 0 — a changed `whenAbsent` was ignored outright. Key on `event`, `read` and `whenAbsent` too. This makes an unstable argument defeat the cache rather than silently serve a value derived from the previous one, so React fails it with "The result of getSnapshot should be cached" at first render. That is the intended failure mode and is now documented as such: a loud error beats a wrong list nobody can account for. Also correct the docs, which described the helper as universal. That misread useListError as a violation: its snapshot is the stored Error instance, already referentially stable, so it subscribes with useSyncExternalStore directly and needs no cache. The rule is read-during-render; the helper is for getters that return a fresh value per read. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
50502269f4
commit
f6105773f0
1 changed files with 101 additions and 17 deletions
3
.github/copilot-instructions.md
vendored
3
.github/copilot-instructions.md
vendored
|
|
@ -55,7 +55,8 @@ const CardContent = Group.withProps({
|
|||
- Use **`useValueChange(value, onChange)`** (`src/hooks/useValueChange.ts`) — React's documented "adjusting state during render" pattern. It does not fire on the first render; seed the state with `useState`. The comparison is `Object.is`, so pass a **referentially stable** value — a primitive key (id/name/URI) or a memoized one, never a fresh object literal.
|
||||
- The `onChange` runs **during render**, so it must be pure — `setState` and nothing else. No fetches, DOM writes, logging, ref mutation, or parent callbacks; a render can be replayed or abandoned.
|
||||
- Effects remain correct for real external-system synchronization (DOM measurement, rAF, subscriptions, timers).
|
||||
- **Subscribing to an `@inspector/core` state store is `useSyncExternalStore`, never `useState` + a subscribing `useEffect`** — that shape re-seeds state from the store prop, so it carries the same stale frame plus a window where an event dispatched between render and subscribe is lost. Use **`useStoreSnapshot(store, event, read, whenAbsent)`** (`core/react/useStoreSnapshot.ts`), once per value; `useValueChange` is web-only and `core/react/` is shared with the CLI and TUI. Pass `read`/`whenAbsent` as **module-scope constants**.
|
||||
- **Subscribing to an `@inspector/core` state store is `useSyncExternalStore`, never `useState` + a subscribing `useEffect`** — that shape re-seeds state from the store prop, so it carries the same stale frame plus a window where an event dispatched between render and subscribe is lost. `useValueChange` is web-only and `core/react/` is shared with the CLI and TUI.
|
||||
- Use **`useStoreSnapshot(store, event, read, whenAbsent)`** (`core/react/useStoreSnapshot.ts`), once per value, when the getter returns a **fresh value per read** (a defensive copy, or a freshly built object) — nearly all of them. When the snapshot is **already referentially stable**, subscribe directly and skip the helper, as `useListError` does with its `Error | null`. Pass `read`/`whenAbsent` as **module-scope constants**: they are part of the cache key, so an unstable one trips React's "getSnapshot should be cached" error.
|
||||
- The snapshot is cached against the store's per-event dispatch counter (`TypedEventTarget.getEventRevision`), **not** its contents — a contents compare cannot see a dispatch that mutated an entry already in the list. Don't replace it with a shallow compare.
|
||||
- The store is the source of truth; the event is only the signal. A test firing a store event must set the value on the store first, and a fake store must extend the real `TypedEventTarget` (a bare `EventTarget` lacks `getEventRevision`).
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue