docs(core): correct the useStoreSnapshot caller contract
Copilot review round 2. The contract claimed an unstable `read`/`whenAbsent` trips React's "The result of getSnapshot should be cached" error. It does not. Measured on React 19: an inline `read` closure throws nothing, logs nothing, and forces no extra render pass. React's dev check calls getSnapshot twice within a single render, where the closure is unchanged, so the cache answers both calls identically and the warning never fires. The real cost is quiet — a fresh value every render, defeating downstream useMemo/React.memo/effect deps — which is a better reason to state the contract than an error that never arrives. Also stop requiring module scope for every fallback. A primitive `whenAbsent` (`false`, `undefined`, `"disconnected"`) is stable under Object.is and is passed inline throughout these hooks, so the rule contradicted the code it was describing. Only object/array fallbacks need a constant, which is what NO_TOOLS and NO_PAGINATION are. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
f6105773f0
commit
8f3619f618
1 changed files with 22 additions and 14 deletions
|
|
@ -1219,7 +1219,8 @@ Nothing **enforces** the boundary: no path alias keys off it, and the coverage `
|
|||
- Reach for **`useStoreSnapshot(store, event, read, whenAbsent)`** (`core/react/useStoreSnapshot.ts`) when the getter returns a **fresh value per read** — a defensive copy (`getTools()` is `[...this.items]`) or a freshly built object (`getPagination()`). That is nearly all of them, and the caching it adds is what keeps `useSyncExternalStore` from looping. Call it once per value.
|
||||
- When a getter's value is **already referentially stable**, subscribe with `useSyncExternalStore` directly and skip the helper — `useListError` does, because its snapshot is the stored `Error` instance itself (or `null`). The rule is read-during-render, not "always use the helper".
|
||||
- Either way `useValueChange` is *not* the tool here — it lives in `clients/web/src`, and `core/react/` is consumed by the CLI and TUI too.
|
||||
- `read` and `whenAbsent` must be **module-scope constants**, not built in the component body. They are part of the snapshot's cache key, so an unstable one defeats the cache and React fails it outright with "The result of getSnapshot should be cached" — deliberately, since the alternative is silently serving a value derived from the previous argument.
|
||||
- `read` and `whenAbsent` must be **referentially stable across renders** — they are part of the snapshot's cache key. `read` is a function, so declare it at module scope. `whenAbsent` only needs a module-scope constant when it is an **object or array** (`NO_TOOLS`, `NO_PAGINATION`); a primitive fallback (`false`, `undefined`, `"disconnected"`) is already stable under `Object.is` and is passed inline throughout these hooks.
|
||||
- An unstable one **fails quietly, so don't expect to be told**: measured on React 19, an inline `read` throws nothing, logs nothing — not even React's "getSnapshot should be cached" dev warning, whose double-call happens within a single render where the closure is unchanged — and forces no extra render. It simply returns a fresh value every render, defeating every downstream `useMemo` / `React.memo` / effect dep that keys on it.
|
||||
- The snapshot is cached against the store's **per-event dispatch counter** (`TypedEventTarget.getEventRevision`), which every dispatch advances automatically — not against the snapshot's contents. That is deliberate and load-bearing: these getters return a defensive copy, so contents are the only alternative, and a contents comparison cannot see a dispatch that mutated an entry the list already holds (`MessageLogState` folding a response into its request entry does exactly that). Don't "optimize" it into a shallow compare.
|
||||
- Consequently the **store is the source of truth and the event is only the signal** — the hook re-reads the store rather than taking the event's `detail`. A test that fires a store event must put the value on the store first; dispatching alone announces a change that isn't there. A hand-rolled fake must extend the real `TypedEventTarget` (a bare `EventTarget` has no `getEventRevision` and fails at runtime).
|
||||
- Genuinely local state accumulated from an event stream, with no getter to read it back from, stays `useState` — `useInspectorClient`'s `lastError` is the one such case. Reset it on a store swap **during render**, not in an effect.
|
||||
|
|
|
|||
Loading…
Reference in a new issue