diff --git a/.changeset/6817-usetagcache-config-alloc.md b/.changeset/6817-usetagcache-config-alloc.md new file mode 100644 index 000000000..727073691 --- /dev/null +++ b/.changeset/6817-usetagcache-config-alloc.md @@ -0,0 +1,20 @@ +--- +'@object-ui/react': patch +--- + +`useETagCache` builds its config object once per hook instance instead of once +per render (objectui#6817). + +`useRef({ enabled, storage, storagePrefix, maxEntries, ttl })` evaluated that +literal on **every** render and kept only the first result, so every later +render allocated a five-key object that was discarded. It now comes from a +`useMemo` keyed on the five values, which is also what the ref's +`useInsertionEffect` write publishes. + +`patch`, not `minor`: nothing a published consumer can observe changes. The +public shape, the returned callbacks' identities and the values the stable +`[]`-deps callbacks read off the ref are all unchanged — the object's identity +is private to the hook, so the only difference is the allocation that no longer +happens. Same pattern PR objectui#6796 repaired in `useSchemaPersistence`; this +is the half of that class the `react-hooks/refs` rule structurally cannot see, +which is why it needed a test rather than a lint fix. diff --git a/packages/react/src/hooks/__tests__/useETagCache.configAlloc.test.tsx b/packages/react/src/hooks/__tests__/useETagCache.configAlloc.test.tsx new file mode 100644 index 000000000..821d4ae7e --- /dev/null +++ b/packages/react/src/hooks/__tests__/useETagCache.configAlloc.test.tsx @@ -0,0 +1,134 @@ +/** + * ObjectUI — useETagCache config-object allocation pin (objectui#6817) + * Copyright (c) 2024-present ObjectStack Inc. + * + * `useETagCache` seeded its config ref with an inline object literal: + * + * const configRef = useRef({ enabled, storage, storagePrefix, maxEntries, ttl }); + * + * `useRef` evaluates that argument on EVERY render and keeps only the first + * result, so every later render allocated a five-key object that was thrown + * away. It is the same shape PR objectui#6796 repaired in `useSchemaPersistence` + * (`useRef(createLocalStorageAdapter())`), with one difference that is the whole + * reason this needed its own card: clearing that one's `react-hooks/refs` + * warning REQUIRED the change, and clearing this one's did not — so the lint + * rule structurally cannot see this half of the class, and only a test can. + * + * ## Why this file mocks `useRef` + * + * The config object is private to the hook: nothing exports it, no in-repo + * consumer renders the hook (`packages/react/src/hooks/index.ts` exports it and + * nothing else calls it), and its identity is deliberately invisible from the + * outside. So "an object was allocated and discarded" has exactly one external + * observation point — the value handed to `useRef` on each render. The wrapper + * below records that argument and delegates to the real `useRef`, so the hook + * still runs on genuine React. + * + * The pins come in pairs on purpose. Pin 1 fails on the old code (three renders + * hand `useRef` three different objects). Pins 2a-2e pass on BOTH the old and + * the new code, and exist to stop the over-fix: memoizing on `[]` would make + * pin 1 pass while freezing the config at its first render — a semantics break + * the timing pins in `useETagCache.configTiming.test.tsx` would then catch, but + * these say it in the same file as the claim they qualify. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { renderHook } from '@testing-library/react'; +import { useETagCache, type ETagCacheConfig } from '../useETagCache'; + +const { seeds } = vi.hoisted(() => ({ seeds: [] as unknown[] })); + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal(); + const useRef = (initialValue: unknown) => { + seeds.push(initialValue); + return (actual.useRef as (v: unknown) => unknown)(initialValue); + }; + return { ...actual, useRef } as unknown as typeof import('react'); +}); + +/** The five keys the hook keeps in its config ref, per useETagCache.ts. */ +const CONFIG_KEYS = ['enabled', 'storage', 'storagePrefix', 'maxEntries', 'ttl'] as const; + +type ConfigSeed = Record<(typeof CONFIG_KEYS)[number], unknown>; + +/** + * Every `useRef` seed recorded so far that has the config shape. The filter + * matters: the recorder sees every `useRef` call in the rendered tree, not just + * this hook's, so an unrelated ref elsewhere must not be counted as a config + * allocation (and must not be able to mask one either). + */ +function configSeeds(): ConfigSeed[] { + return seeds.filter( + (value): value is ConfigSeed => + typeof value === 'object' && + value !== null && + CONFIG_KEYS.every((key) => key in (value as Record)), + ); +} + +beforeEach(() => { + seeds.length = 0; +}); + +describe('useETagCache — the config object is built once per hook instance (#6817)', () => { + // ---- pin 1: THE DEFECT — red before the repair, green after --------------- + it('hands useRef the same config object on every render when nothing changed', () => { + const { rerender } = renderHook( + ({ ttl }: { ttl: number }) => useETagCache({ ttl, storagePrefix: 'alloc-pin' }), + { initialProps: { ttl: 1_000 } }, + ); + + rerender({ ttl: 1_000 }); + rerender({ ttl: 1_000 }); + + const seen = configSeeds(); + // Three renders must have reached the ref seed, or this pin is measuring + // nothing and the identity assertion below would pass vacuously. + expect(seen.length).toBe(3); + expect(seen[1]).toBe(seen[0]); + expect(seen[2]).toBe(seen[0]); + expect(new Set(seen).size).toBe(1); + }); + + // ---- pin 2: NOT an over-fix — a real config change still allocates -------- + // `useMemo(..., [])` would satisfy pin 1 and freeze the config at its first + // render. One case per key, so a dependency list missing any single one of + // the five fails here rather than in a consumer. + const changes: Array<{ key: string; before: ETagCacheConfig; after: ETagCacheConfig }> = [ + { key: 'enabled', before: { enabled: true }, after: { enabled: false } }, + { key: 'storage', before: { storage: 'memory' }, after: { storage: 'localStorage' } }, + { key: 'storagePrefix', before: { storagePrefix: 'p-a' }, after: { storagePrefix: 'p-b' } }, + { key: 'maxEntries', before: { maxEntries: 10 }, after: { maxEntries: 20 } }, + { key: 'ttl', before: { ttl: 1_000 }, after: { ttl: 2_000 } }, + ]; + + for (const { key, before, after } of changes) { + it(`builds a fresh config carrying the new value when \`${key}\` changes`, () => { + const { rerender } = renderHook( + ({ config }: { config: ETagCacheConfig }) => useETagCache(config), + { initialProps: { config: before } }, + ); + + rerender({ config: after }); + + const seen = configSeeds(); + expect(seen.length).toBe(2); + expect(seen[1]).not.toBe(seen[0]); + expect(seen[0][key as keyof ConfigSeed]).toBe(before[key as keyof ETagCacheConfig]); + expect(seen[1][key as keyof ConfigSeed]).toBe(after[key as keyof ETagCacheConfig]); + }); + } + + // ---- pin 3: the recorder is real ---------------------------------------- + // If the `vi.mock` above ever stopped intercepting, every pin here would go + // vacuously green on an empty `seeds` array except for the length assertions. + // This states the same guarantee once, directly. + it('records a config seed at all (the useRef interception is live)', () => { + renderHook(() => useETagCache({ storagePrefix: 'recorder-probe' })); + + const seen = configSeeds(); + expect(seen.length).toBe(1); + expect(seen[0].storagePrefix).toBe('recorder-probe'); + }); +}); diff --git a/packages/react/src/hooks/useETagCache.ts b/packages/react/src/hooks/useETagCache.ts index f1cf647ed..496af5bf1 100644 --- a/packages/react/src/hooks/useETagCache.ts +++ b/packages/react/src/hooks/useETagCache.ts @@ -208,9 +208,23 @@ export function useETagCache(userConfig: ETagCacheConfig = {}): ETagCacheResult // every layout effect, ref attachment and paint, so the sole window deferred // is the render phase — where `fetchWithETag` / `clearCache` and the other // side effects are not callable anyway. - const configRef = useRef({ enabled, storage, storagePrefix, maxEntries, ttl }); + // + // The object itself is built by `useMemo` rather than inline at the two + // places that consume it. `useRef({ ... })` evaluates its argument on EVERY + // render and keeps only the first result, so every later render allocated a + // five-key object that was thrown away (objectui#6817) — the same shape PR + // objectui#6796 repaired in `useSchemaPersistence`. Its identity is + // unobservable from outside the hook: the ref is private and every reader + // only reads fields off `.current`, so a memo React chooses to discard is + // harmless — it rebuilds an equal object, which is what every render used + // to do unconditionally. + const config = useMemo( + () => ({ enabled, storage, storagePrefix, maxEntries, ttl }), + [enabled, storage, storagePrefix, maxEntries, ttl], + ); + const configRef = useRef(config); useInsertionEffect(() => { - configRef.current = { enabled, storage, storagePrefix, maxEntries, ttl }; + configRef.current = config; }); // Hydrate memory cache from localStorage on mount