feat: useSyncExternalStore #64
Conversation
|
WalkthroughAdds a new compat hook module implementing useSyncExternalStore and useSyncExternalStoreWithSelector, exposes them via the public index, and introduces comprehensive tests with a mock store covering subscription, updates, selectors, equality, switching stores, and error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Component
participant H as useSyncExternalStore
participant S as External Store
C->>H: render(subscribe, getSnapshot)
H->>S: getSnapshot()
S-->>H: snapshot
H-->>C: return snapshot
rect rgba(230,245,255,0.6)
note over H,S: Mount effects
H->>S: subscribe(onStoreChange)
S-->>H: unsubscribe fn
H->>H: sync internal store (value/getSnapshot)
end
rect rgba(240,255,240,0.6)
note over S: Store update
S-->>H: onStoreChange()
H->>S: getSnapshot()
H-->>C: force update if changed
end
rect rgba(255,240,240,0.6)
note over H,S: Unmount
H->>S: unsubscribe()
end
sequenceDiagram
autonumber
actor C as Component
participant HS as useSyncExternalStoreWithSelector
participant H as useSyncExternalStore (base)
participant S as External Store
C->>HS: render(subscribe, getSnapshot, selector, isEqual?)
HS->>S: getSnapshot()
HS->>HS: apply selector, compare via isEqual?
HS->>H: delegate subscribe/getSnapshot (memoized)
H->>S: subscribe(onStoreChange)
H-->>HS: snapshot changes
HS->>HS: recompute selection if snapshot changed
HS-->>C: return selected value (re-render only if changed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
create-inula
openinula
inula-cli
inula-intl
inula-request
inula-router
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
packages/inula/tests/CompactTest/mockStore.ts (3)
44-66: Avoid JSON.stringify deep compare in test helper (perf/noise)Deep JSON compare is O(n) and brittle for non-JSONable shapes. A shallow compare is enough for these tests and reduces flakiness/perf overhead.
Apply:
- if (typeof value === 'object' && value !== null && typeof newValue === 'object' && newValue !== null) { - // For objects: update if different reference OR different content - shouldUpdate = value !== newValue || JSON.stringify(value) !== JSON.stringify(newValue); - if (shouldUpdate) { - value = { ...newValue as any }; - } - } else { + if (typeof value === 'object' && value !== null && typeof newValue === 'object' && newValue !== null) { + // Shallow compare object props + const prev = value as any; + const next = newValue as any; + if (prev === next) { + shouldUpdate = false; + } else { + const prevKeys = Object.keys(prev); + const nextKeys = Object.keys(next); + shouldUpdate = + prevKeys.length !== nextKeys.length || + nextKeys.some((k) => prev[k] !== next[k]); + } + if (shouldUpdate) { + value = { ...(newValue as any) }; + } + } else {
53-55: Shallow clone may drop prototypes (ok for tests, noting intent)Spreading objects strips prototypes/Set/Map/Date. Fine for this test helper; add a short comment to document that only plain objects are assumed.
- value = { ...newValue as any }; + // Intentionally normalize to a plain object for test determinism. + value = { ...(newValue as any) };
78-86: Consider guarding duplicate subscriptions (optional)If the same listener is added twice, Set prevents duplication, but returning an idempotent disposer can avoid surprises if users call unsubscribe multiple times.
packages/inula/tests/CompactTest/UseSyncExternalStore.test.tsx (4)
415-416: Fix test title to match expectationsThe body expects a re-render on an equivalent object with a new reference; the title claims the opposite.
- it('should not re-render for equivalent objects', () => { + it('should re-render for equivalent objects (new reference)', () => {
588-591: Strengthen reference-stability assertionThe second expect is tautological. Compare against the store’s current items reference.
- // Should maintain same reference for items - expect(selections.length).toBe(1); - expect(selections[0]).toBe(selections[0]); // Same reference + // No re-render occurred and selector returned the same underlying reference + expect(selections.length).toBe(1); + expect(selections[0]).toBe((store.getValue() as any).items);
682-707: Consider adding a test for subscribe() throwingYou simulate subscribe errors in the mock; add a spec ensuring the hook surfaces/propagates this scenario (with or without an ErrorBoundary).
Example:
it('propagates subscribe errors', () => { const store = createMockStore('x'); store.enableSubscribeError(); function App() { // should throw during subscribe in effect useSyncExternalStore(store.subscribe, store.getValue); return <div>ok</div>; } expect(() => render(<App />, container)).toThrow('subscribe error'); });
49-52: Optional: wrap initial renders in act for consistencySome renders are inside act, others not. Wrapping all renders avoids timing flakiness in async effects.
Also applies to: 71-71, 95-96, 112-113, 149-152, 176-179, 231-237, 240-247, 297-304, 306-313, 315-321, 324-331, 371-377, 401-403, 425-427, 544-546, 580-582, 628-631, 668-670, 700-702
packages/inula/src/compat/UseSyncExternalStoreHook.ts (4)
29-35: Option: add getServerSnapshot to the base hook for React parity and SSR.Brings API to
useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot?)and uses server snapshot during SSR.export function useSyncExternalStore<T>( subscribe: (onStoreChange: StoreChangeListener) => Unsubscribe, - getSnapshot: () => T, + getSnapshot: () => T, + getServerSnapshot?: () => T, ): T { // 获取当前Store快照 - const value = getSnapshot(); + const value = + typeof window === 'undefined' && isFunction(getServerSnapshot) + ? getServerSnapshot() + : getSnapshot(); @@ - }, [subscribe, value, getSnapshot]); + }, [subscribe, value, getSnapshot, getServerSnapshot]);Also applies to: 59-59
59-59: Nit: drop unused dependency from layout effect.
subscribeisn’t read inside the effect; removing it avoids unnecessary re-runs.- }, [subscribe, value, getSnapshot]); + }, [value, getSnapshot]);
145-148: Keep cachedSnapshot fresh even when equal to reduce future recomputes.Updating the cached snapshot when deemed equal prevents spurious recomputes on subsequent equal transitions.
- if (isSame(previousSnapshot, snapshot)) { - return previousSelection; - } + if (isSame(previousSnapshot, snapshot)) { + cachedSnapshot = snapshot; + return previousSelection; + }
100-107: Type of getServerSnapshot can be simplified.Prefer
(() => Snapshot) | null | undefinedovervoid | null | (() => Snapshot)for clarity.- getServerSnapshot: void | null | (() => Snapshot), + getServerSnapshot: (() => Snapshot) | null | undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/inula/src/compat/UseSyncExternalStoreHook.ts(1 hunks)packages/inula/src/index.ts(3 hunks)packages/inula/tests/CompactTest/UseSyncExternalStore.test.tsx(1 hunks)packages/inula/tests/CompactTest/mockStore.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/inula/tests/CompactTest/UseSyncExternalStore.test.tsx (3)
packages/inula/src/index.ts (5)
unmountComponentAtNode(162-162)useSyncExternalStore(152-152)act(163-163)render(158-158)useSyncExternalStoreWithSelector(153-153)packages/inula/tests/CompactTest/mockStore.ts (1)
createMockStore(30-111)packages/inula/src/compat/UseSyncExternalStoreHook.ts (2)
useSyncExternalStore(29-72)useSyncExternalStoreWithSelector(101-178)
packages/inula/src/compat/UseSyncExternalStoreHook.ts (2)
packages/inula/tests/CompactTest/mockStore.ts (1)
subscribe(78-86)packages/inula-intl/src/vueI18n-adapter/src/utils/utils.ts (1)
isFunction(82-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: commitlint
🔇 Additional comments (7)
packages/inula/src/index.ts (2)
76-76: Public API import looks goodImporting the new compat hooks from './compat/UseSyncExternalStoreHook' is correct and side‑effect free.
97-99: Wiring hooks into both the Inula namespace and named exports is correctThe additions ensure both default namespace usage (Inula.useSyncExternalStore*) and named imports work.
If you ship type declarations, confirm these symbols are included in the published d.ts so TS consumers don’t see missing types.
Also applies to: 152-154
packages/inula/tests/CompactTest/mockStore.ts (1)
23-27: Nice: explicit error toggles for subscribe/snapshotClear, focused API for simulating error paths in tests.
packages/inula/tests/CompactTest/UseSyncExternalStore.test.tsx (4)
40-61: LGTM: lifecycle/unsubscribe behavior is coveredSubscribes, asserts listener count, and verifies cleanup on unmount.
166-191: Good coverage of re-subscribe on store changeValidates effect cleanup and new subscription when subscribe changes.
335-389: Error boundary path for getSnapshot exceptions is well coveredCovers throwing snapshots and recovery via boundary.
485-522: Selector + custom equality scenario is solidDemonstrates controlled re-renders via isEqual.
| // 创建带选择器的快照获取函数 | ||
| const snapshotWithSelector = useMemo(() => { | ||
| // 使用闭包变量追踪缓存状态，在 getSnapshot / selector / isEqual 不变时 | ||
| let initialized = false; | ||
| let cachedSnapshot: Snapshot; | ||
| let cachedSelection: Selection; | ||
|
|
||
| const memoizedSelectorFn = (snapshot: Snapshot) => { | ||
| // 首次调用时的处理 | ||
| if (!initialized) { | ||
| initialized = true; | ||
| cachedSnapshot = snapshot; | ||
| const selection = selector(snapshot); | ||
|
|
||
| // 尝试复用当前渲染值 | ||
| if (isFunction(isEqual) && instRef.current.hasValue) { | ||
| const current = instRef.current.value; | ||
| if (isEqual(current, selection)) { | ||
| cachedSelection = current; | ||
| return current; | ||
| } | ||
| } | ||
|
|
||
| cachedSelection = selection; | ||
| return selection; | ||
| } | ||
|
|
||
| // 尝试复用之前的结果 | ||
| const previousSnapshot = cachedSnapshot; | ||
| const previousSelection = cachedSelection; | ||
|
|
||
| // 快照未变化时直接返回之前的选择结果 | ||
| if (isSame(previousSnapshot, snapshot)) { | ||
| return previousSelection; | ||
| } | ||
|
|
||
| // 快照已变化，需要计算新的选择结果 | ||
| const newSelection = selector(snapshot); | ||
|
|
||
| // 使用自定义相等判断函数检查数据是否真正发生变化 | ||
| if (isFunction(isEqual) && isEqual(previousSelection, newSelection)) { | ||
| // 虽然选择结果相等，但仍需更新快照避免保留旧引用 | ||
| cachedSnapshot = snapshot; | ||
| return previousSelection; | ||
| } | ||
|
|
||
| // 更新缓存并返回新结果 | ||
| cachedSnapshot = snapshot; | ||
| cachedSelection = newSelection; | ||
| return newSelection; | ||
| }; | ||
|
|
||
| return () => memoizedSelectorFn(getSnapshot()); | ||
| }, [getSnapshot, selector, isEqual]); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity-check: identity stability of selectedValue across equal updates.
After fixing isFunction, the selector path should now retain referential identity when isEqual says equal. Please confirm tests assert identity stability (Object.is) across updates.
🏁 Script executed:
#!/bin/bash
# Verify selector identity stability when isEqual returns true.
rg -nC3 "useSyncExternalStoreWithSelector\(" packages | cat
rg -n "Object\.is\(|toBe\(.*same.*reference|toBe\(.*selectedValue" packages/inula/tests -g '!**/node_modules/**' || trueLength of output: 5466
Add identity-stability test for selector
Tests don’t currently verify that selectedValue preserves referential equality when isEqual returns true (no Object.is or toBe assertions found). Add a test in UseSyncExternalStore.test.tsx that updates the store with an equal value and asserts the same reference is returned.
| return () => memoizedSelectorFn(getSnapshot()); | ||
| }, [getSnapshot, selector, isEqual]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use getServerSnapshot for SSR to avoid hydration mismatches.
getServerSnapshot is accepted but not used. Wire it in and include as a dependency.
- return () => memoizedSelectorFn(getSnapshot());
- }, [getSnapshot, selector, isEqual]);
+ return () =>
+ memoizedSelectorFn(
+ (typeof window === 'undefined' && getServerSnapshot
+ ? getServerSnapshot
+ : getSnapshot)()
+ );
+ }, [getSnapshot, getServerSnapshot, selector, isEqual]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return () => memoizedSelectorFn(getSnapshot()); | |
| }, [getSnapshot, selector, isEqual]); | |
| return () => | |
| memoizedSelectorFn( | |
| (typeof window === 'undefined' && getServerSnapshot | |
| ? getServerSnapshot | |
| : getSnapshot)() | |
| ); | |
| }, [getSnapshot, getServerSnapshot, selector, isEqual]); |
🤖 Prompt for AI Agents
In packages/inula/src/compat/UseSyncExternalStoreHook.ts around lines 166-167,
the getServerSnapshot argument is accepted but not used; update the effect to
use getServerSnapshot() when available (e.g., call
memoizedSelectorFn(getServerSnapshot()) instead of always using getSnapshot())
for SSR initial reads and any server snapshot reads, and add getServerSnapshot
to the hook's dependency array so React re-runs the effect when it changes
(final deps: [getSnapshot, getServerSnapshot, selector, isEqual]).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/inula/src/compat/UseSyncExternalStoreHook.ts (2)
114-165: Add test to assert selector identity stability when isEqual returns true.Implementation preserves reference when equal, but tests don’t assert it.
Run to confirm absence of such assertions; I can provide a test if missing.
#!/bin/bash # Look for identity/Reference assertions in selector tests rg -nC2 -g '!**/node_modules/**' "(Object\.is|toBe\(.+same.+reference)" packages/inula/tests || trueProposed test snippet to add under CompactTest/UseSyncExternalStore.test.tsx:
it('keeps selectedValue reference stable when isEqual returns true', () => { const store = createMockStore({ n: 0, meta: { x: 1 } }); const selector = (s: typeof store.getSnapshot()) => s.meta; const isEqual = (a: any, b: any) => a.x === b.x; function Comp() { const selected = useSyncExternalStoreWithSelector( store.subscribe, store.getSnapshot, undefined, selector, isEqual ); useEffect(() => void (ref.current = selected)); return null; } const ref = { current: null as any }; render(<Comp />); const first = ref.current; store.setState({ n: 1, meta: { x: 1 } }); // equal by isEqual const second = ref.current; expect(second).toBe(first); // same reference });
166-168: Wire up getServerSnapshot to prevent SSR hydration mismatches.The third arg is accepted but never used. Use it on the server and add it to deps so selector reads match server/client.
- return () => memoizedSelectorFn(getSnapshot()); - }, [getSnapshot, selector, isEqual]); + return () => { + const read = + typeof window === 'undefined' && typeof getServerSnapshot === 'function' + ? getServerSnapshot + : getSnapshot; + return memoizedSelectorFn(read()); + }; + }, [getSnapshot, getServerSnapshot, selector, isEqual]);
🧹 Nitpick comments (3)
packages/inula/src/compat/UseSyncExternalStoreHook.ts (3)
51-60: Optional: include getServerSnapshot in deps for clarity.Not strictly required (value already depends on it), but adding it makes intent explicit.
- }, [subscribe, value, getSnapshot]); + }, [subscribe, value, getSnapshot, getServerSnapshot]);
95-98: Tighten isFunction type guard to avoid generic-narrowing quirks.Using unknown here yields cleaner narrowing for union types like
(fn | undefined).-function isFunction<T>(value: T): value is T & ((...args: any[]) => any) { - return typeof value === 'function'; -} +function isFunction(value: unknown): value is (...args: any[]) => any { + return typeof value === 'function'; +}
36-43: Comment nit: fix example argument name in forceUpdate note.The comment mentions inst but the code uses store.
- // 需要强制更新时调用 `forceUpdate({inst})`，由于是新的对象引入，会导致组件强制更新 + // 需要强制更新时调用 `forceUpdate({ store })`，由于是新的对象引入，会导致组件强制更新
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/inula/src/compat/UseSyncExternalStoreHook.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/inula/src/compat/UseSyncExternalStoreHook.ts (2)
packages/inula/src/index.ts (7)
useSyncExternalStore(152-152)useState(151-151)useLayoutEffect(147-147)useEffect(145-145)useSyncExternalStoreWithSelector(153-153)useRef(150-150)useMemo(148-148)packages/inula/tests/CompactTest/mockStore.ts (1)
subscribe(78-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: continuous-release
🔇 Additional comments (1)
packages/inula/src/compat/UseSyncExternalStoreHook.ts (1)
79-88: Change detection logic LGTM.Catching getSnapshot errors and treating them as a change matches resilient semantics.
| export function useSyncExternalStore<T>( | ||
| subscribe: (onStoreChange: StoreChangeListener) => Unsubscribe, | ||
| getSnapshot: () => T, | ||
| ): T { | ||
| // 获取当前Store快照 | ||
| const value = getSnapshot(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align base hook with React API: support optional getServerSnapshot.
Adding the optional third param improves compat and prevents SSR/client divergence for non-selector usage.
-export function useSyncExternalStore<T>(
- subscribe: (onStoreChange: StoreChangeListener) => Unsubscribe,
- getSnapshot: () => T,
-): T {
- // 获取当前Store快照
- const value = getSnapshot();
+export function useSyncExternalStore<T>(
+ subscribe: (onStoreChange: StoreChangeListener) => Unsubscribe,
+ getSnapshot: () => T,
+ getServerSnapshot?: () => T,
+): T {
+ // 获取当前Store快照（SSR 时优先使用 getServerSnapshot）
+ const readSnapshot =
+ typeof window === 'undefined' && typeof getServerSnapshot === 'function'
+ ? getServerSnapshot
+ : getSnapshot;
+ const value = readSnapshot();🤖 Prompt for AI Agents
In packages/inula/src/compat/UseSyncExternalStoreHook.ts around lines 29-35, the
exported useSyncExternalStore hook signature and implementation lack the
optional third parameter getServerSnapshot required by the React API; update the
function signature to accept getServerSnapshot?: () => T, update internal usage
to call getServerSnapshot when available for server snapshot resolution (falling
back to getSnapshot), and update types so the third arg is optional and returned
value still typed as T; ensure existing call sites compile with the new optional
parameter.
|
Summary by CodeRabbit