diff --git a/src/hooks/useDecide.spec.tsx b/src/hooks/useDecide.spec.tsx index eeb6967..effd118 100644 --- a/src/hooks/useDecide.spec.tsx +++ b/src/hooks/useDecide.spec.tsx @@ -16,9 +16,10 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import React from 'react'; -import { act } from '@testing-library/react'; +import { act, waitFor } from '@testing-library/react'; import { renderHook } from '@testing-library/react'; -import { OptimizelyContext, ProviderStateStore } from '../provider/index'; +import { OptimizelyContext, ProviderStateStore, OptimizelyProvider } from '../provider/index'; +import { REACT_CLIENT_META } from '../client/index'; import { useDecide } from './useDecide'; import type { OptimizelyUserContext, @@ -68,6 +69,49 @@ function createMockClient(hasConfig = false): Client { } as unknown as Client; } +/** + * Creates a mock client with notification center support and wraps it in OptimizelyProvider. + * Used for integration-style tests that need the full Provider lifecycle. + */ +function createProviderWrapper(mockUserContext: OptimizelyUserContext) { + let configUpdateCallback: (() => void) | undefined; + + const client = { + getOptimizelyConfig: vi.fn().mockReturnValue({ revision: '1' }), + createUserContext: vi.fn().mockReturnValue(mockUserContext), + onReady: vi.fn().mockResolvedValue(undefined), + isOdpIntegrated: vi.fn().mockReturnValue(false), + notificationCenter: { + addNotificationListener: vi.fn().mockImplementation((type: string, cb: () => void) => { + if (type === 'OPTIMIZELY_CONFIG_UPDATE') { + configUpdateCallback = cb; + } + return 1; + }), + removeNotificationListener: vi.fn(), + }, + } as unknown as Client; + + (client as unknown as Record)[REACT_CLIENT_META] = { + hasOdpManager: false, + hasVuidManager: false, + }; + + function Wrapper({ children }: { children: React.ReactNode }) { + return ( + + {children} + + ); + } + + return { + wrapper: Wrapper, + client, + fireConfigUpdate: () => configUpdateCallback?.(), + }; +} + function createWrapper(store: ProviderStateStore, client: Client) { const contextValue: OptimizelyContextValue = { store, client }; @@ -177,25 +221,6 @@ describe('useDecide', () => { expect(result.current.decision).toBe(MOCK_DECISION); }); - it('should re-evaluate when setClientReady fire', async () => { - const mockUserContext = createMockUserContext(); - store.setUserContext(mockUserContext); - // Client has no config yet - const wrapper = createWrapper(store, mockClient); - const { result } = renderHook(() => useDecide('flag_1'), { wrapper }); - - expect(result.current.isLoading).toBe(true); - - // Simulate config becoming available when onReady resolves - (mockClient.getOptimizelyConfig as ReturnType).mockReturnValue({ revision: '1' }); - await act(async () => { - store.setClientReady(true); - }); - - expect(result.current.isLoading).toBe(false); - expect(result.current.decision).toBe(MOCK_DECISION); - }); - it('should return error from store with isLoading: false', async () => { const wrapper = createWrapper(store, mockClient); const { result } = renderHook(() => useDecide('flag_1'), { wrapper }); @@ -319,31 +344,37 @@ describe('useDecide', () => { expect(result.current.decision).toBeNull(); }); - it('should re-call decide() when setClientReady fires after sync decision was already served', async () => { - // Sync datafile scenario: config + userContext available before onReady - mockClient = createMockClient(true); + it('should re-evaluate decision when OPTIMIZELY_CONFIG_UPDATE fires from the client', async () => { const mockUserContext = createMockUserContext(); - store.setUserContext(mockUserContext); + const { wrapper, fireConfigUpdate } = createProviderWrapper(mockUserContext); - const wrapper = createWrapper(store, mockClient); const { result } = renderHook(() => useDecide('flag_1'), { wrapper }); - // Decision already served - expect(result.current.isLoading).toBe(false); + // Wait for Provider's onReady + UserContextManager + queueMicrotask chain to complete + await waitFor(() => { + expect(result.current.isLoading).toBe(false); + }); + expect(result.current.decision).toBe(MOCK_DECISION); - expect(mockUserContext.decide).toHaveBeenCalledTimes(1); - // onReady() resolves → setClientReady(true) fires → store state changes → - // useSyncExternalStore re-renders → useMemo recomputes → decide() called again. - // This is a redundant call since config + userContext haven't changed, - // but it's a one-time cost per flag per page load. + const callCountBeforeUpdate = (mockUserContext.decide as ReturnType).mock.calls.length; + + // Simulate a new datafile with a different decision + const updatedDecision: OptimizelyDecision = { + ...MOCK_DECISION, + variationKey: 'variation_2', + variables: { color: 'blue' }, + }; + (mockUserContext.decide as ReturnType).mockReturnValue(updatedDecision); + + // Fire the config update notification (as the SDK would on datafile poll) await act(async () => { - store.setClientReady(true); + fireConfigUpdate(); }); - expect(mockUserContext.decide).toHaveBeenCalledTimes(2); + expect(mockUserContext.decide).toHaveBeenCalledTimes(callCountBeforeUpdate + 1); + expect(result.current.decision).toBe(updatedDecision); expect(result.current.isLoading).toBe(false); - expect(result.current.decision).toBe(MOCK_DECISION); }); describe('forced decision reactivity', () => { diff --git a/src/hooks/useOptimizelyClient.spec.tsx b/src/hooks/useOptimizelyClient.spec.tsx index c65ae36..f1d10d8 100644 --- a/src/hooks/useOptimizelyClient.spec.tsx +++ b/src/hooks/useOptimizelyClient.spec.tsx @@ -81,12 +81,12 @@ describe('useOptimizelyClient', () => { // Trigger store state changes that should NOT cause useOptimizelyClient to re-render act(() => { - store.setClientReady(true); + store.setError(new Error('test')); }); expect(capturedRenderCount).toBe(initialRenderCount); act(() => { - store.setError(new Error('test')); + store.refresh(); }); expect(capturedRenderCount).toBe(initialRenderCount); }); diff --git a/src/hooks/useOptimizelyUserContext.spec.tsx b/src/hooks/useOptimizelyUserContext.spec.tsx index ec35bb2..55acdcd 100644 --- a/src/hooks/useOptimizelyUserContext.spec.tsx +++ b/src/hooks/useOptimizelyUserContext.spec.tsx @@ -206,11 +206,11 @@ describe('useOptimizelyUserContext', () => { const initialRenderCount = capturedRenderCount; - // Changing isClientReady triggers a store notification, + // Triggering a store notification via setState, // but since the derived result hasn't changed, useMemo returns // the same reference and React bails out act(() => { - store.setClientReady(true); + store.refresh(); }); expect(capturedRenderCount).toBe(initialRenderCount); diff --git a/src/provider/OptimizelyProvider.spec.tsx b/src/provider/OptimizelyProvider.spec.tsx index b7d5e5d..b7ee639 100644 --- a/src/provider/OptimizelyProvider.spec.tsx +++ b/src/provider/OptimizelyProvider.spec.tsx @@ -55,7 +55,10 @@ function createMockClient( createUserContext: vi.fn().mockReturnValue(mockUserContext), close: vi.fn(), getOptimizelyConfig: vi.fn(), - notificationCenter: {} as OptimizelyClient['notificationCenter'], + notificationCenter: { + addNotificationListener: vi.fn().mockReturnValue(1), + removeNotificationListener: vi.fn(), + } as unknown as OptimizelyClient['notificationCenter'], sendOdpEvent: vi.fn(), isOdpIntegrated: vi.fn().mockReturnValue(false), ...overrides, @@ -151,7 +154,7 @@ describe('OptimizelyProvider', () => { expect(mockClient.onReady).toHaveBeenCalledWith({ timeout: 5000 }); }); - it('should set isClientReady to true when onReady succeeds', async () => { + it('should not set error when onReady succeeds', async () => { const mockClient = createMockClient({ onReady: vi.fn().mockResolvedValue(undefined), }); @@ -165,13 +168,12 @@ describe('OptimizelyProvider', () => { await waitFor(() => { expect(capturedContext).not.toBeNull(); - expect(capturedContext!.store.getState().isClientReady).toBe(true); }); expect(capturedContext!.store.getState().error).toBeNull(); }); - it('should set isClientReady to false and set error when onReady rejects', async () => { + it('should set error when onReady rejects', async () => { const testError = new Error('Client initialization failed'); const mockClient = createMockClient({ onReady: vi.fn().mockRejectedValue(testError), @@ -188,9 +190,6 @@ describe('OptimizelyProvider', () => { expect(capturedContext).not.toBeNull(); expect(capturedContext!.store.getState().error).toBe(testError); }); - - // Client is NOT ready when onReady rejects - expect(capturedContext!.store.getState().isClientReady).toBe(false); }); it('should set error when onReady times out (rejects)', async () => { @@ -210,8 +209,6 @@ describe('OptimizelyProvider', () => { expect(capturedContext).not.toBeNull(); expect(capturedContext!.store.getState().error).toBe(timeoutError); }); - - expect(capturedContext!.store.getState().isClientReady).toBe(false); }); }); @@ -243,7 +240,6 @@ describe('OptimizelyProvider', () => { await waitFor(() => { expect(capturedContext).not.toBeNull(); - expect(capturedContext!.store.getState().isClientReady).toBe(true); }); const store = capturedContext!.store; @@ -251,7 +247,6 @@ describe('OptimizelyProvider', () => { unmount(); // Store should be reset - expect(store.getState().isClientReady).toBe(false); expect(store.getState().userContext).toBeNull(); expect(store.getState().error).toBeNull(); }); @@ -569,8 +564,8 @@ describe('OptimizelyProvider', () => { resolveOnReady!(); }); - // Store was reset on unmount, and onReady resolution should not set isClientReady - expect(store.getState().isClientReady).toBe(false); + // Store was reset on unmount, onReady resolution should not affect store + expect(store.getState().error).toBeNull(); }); it('should call onReady again when client changes', async () => { @@ -638,6 +633,91 @@ describe('OptimizelyProvider', () => { }); }); + describe('config update subscription', () => { + it('should subscribe to OPTIMIZELY_CONFIG_UPDATE on mount', () => { + const mockClient = createMockClient(); + + render( + +
Child
+
+ ); + + expect(mockClient.notificationCenter.addNotificationListener).toHaveBeenCalledWith( + 'OPTIMIZELY_CONFIG_UPDATE', + expect.any(Function) + ); + }); + + it('should remove notification listener on unmount', () => { + const mockClient = createMockClient(); + + const { unmount } = render( + +
Child
+
+ ); + + unmount(); + + expect(mockClient.notificationCenter.removeNotificationListener).toHaveBeenCalledWith(1); + }); + + it('should trigger store state change when config update fires', async () => { + const mockClient = createMockClient(); + let capturedContext: OptimizelyContextValue | null = null; + + render( + + (capturedContext = ctx)} /> + + ); + + await waitFor(() => { + expect(capturedContext).not.toBeNull(); + }); + + const stateBefore = capturedContext!.store.getState(); + + // Get the callback that was registered and invoke it + const configUpdateCallback = ( + mockClient.notificationCenter.addNotificationListener as ReturnType + ).mock.calls.find((call: unknown[]) => call[0] === 'OPTIMIZELY_CONFIG_UPDATE')![1]; + + await act(() => { + configUpdateCallback(); + }); + + const stateAfter = capturedContext!.store.getState(); + + // State should be a new reference (triggers useSyncExternalStore subscribers) + expect(stateBefore).not.toBe(stateAfter); + }); + + it('should re-subscribe when client changes', () => { + const mockClient1 = createMockClient(); + const mockClient2 = createMockClient(); + + const { rerender } = render( + +
Child
+
+ ); + + expect(mockClient1.notificationCenter.addNotificationListener).toHaveBeenCalledTimes(1); + + rerender( + +
Child
+
+ ); + + // Old listener cleaned up, new one registered + expect(mockClient1.notificationCenter.removeNotificationListener).toHaveBeenCalledWith(1); + expect(mockClient2.notificationCenter.addNotificationListener).toHaveBeenCalledTimes(1); + }); + }); + describe('context reference identity', () => { it('should change context value reference when client changes', async () => { const mockClient1 = createMockClient(); diff --git a/src/provider/OptimizelyProvider.tsx b/src/provider/OptimizelyProvider.tsx index 311fd6a..1f3a6a8 100644 --- a/src/provider/OptimizelyProvider.tsx +++ b/src/provider/OptimizelyProvider.tsx @@ -15,6 +15,7 @@ */ import React, { createContext, useRef, useMemo, useEffect } from 'react'; +import { NOTIFICATION_TYPES } from '@optimizely/optimizely-sdk'; import { ProviderStateStore } from './ProviderStateStore'; import { UserContextManager } from '../utils/UserContextManager'; @@ -76,7 +77,8 @@ export function OptimizelyProvider({ userManagerRef.current.resolveUserContext(user, qualifiedSegments, skipSegments); } - // Effect: Client onReady + // Effect: Client onReady — only needed for error handling. + // Readiness is derived from userContext + getOptimizelyConfig() by hooks. useEffect(() => { if (!client) { logger?.error('OptimizelyProvider must be passed an Optimizely client instance'); @@ -86,29 +88,32 @@ export function OptimizelyProvider({ let isMounted = true; - const waitForClientReady = async (): Promise => { - try { - await client.onReady({ timeout }); + client.onReady({ timeout }).catch((error) => { + if (!isMounted) return; + const err = error instanceof Error ? error : new Error(String(error)); + store.setError(err); + }); - if (!isMounted) return; - - store.setClientReady(true); - } catch (error) { - if (!isMounted) return; - const err = error instanceof Error ? error : new Error(String(error)); - store.setState({ - isClientReady: false, - error: err, - }); - } + return () => { + isMounted = false; }; + }, [client, timeout, store]); - waitForClientReady(); + // Effect: Subscribe to config/datafile updates (e.g., polling) + useEffect(() => { + if (!client) return; + + const listenerId = client.notificationCenter.addNotificationListener( + NOTIFICATION_TYPES.OPTIMIZELY_CONFIG_UPDATE, + () => { + store.refresh(); + } + ); return () => { - isMounted = false; + client.notificationCenter.removeNotificationListener(listenerId); }; - }, [client, timeout, store]); + }, [client, store]); // Cleanup on unmount useEffect(() => { diff --git a/src/provider/ProviderStateStore.spec.ts b/src/provider/ProviderStateStore.spec.ts index b0cf2d2..22f488d 100644 --- a/src/provider/ProviderStateStore.spec.ts +++ b/src/provider/ProviderStateStore.spec.ts @@ -47,7 +47,6 @@ describe('ProviderStateStore', () => { it('should have correct initial state', () => { const state = store.getState(); - expect(state.isClientReady).toBe(false); expect(state.userContext).toBeNull(); expect(state.error).toBeNull(); }); @@ -66,13 +65,13 @@ describe('ProviderStateStore', () => { const listener = vi.fn(); store.subscribe(listener); - store.setClientReady(true); + store.setError(new Error('test')); await flushMicrotasks(); expect(listener).toHaveBeenCalledTimes(1); expect(listener).toHaveBeenCalledWith( expect.objectContaining({ - isClientReady: true, + error: expect.any(Error), }) ); }); @@ -83,7 +82,7 @@ describe('ProviderStateStore', () => { store.subscribe(listener1); store.subscribe(listener2); - store.setClientReady(true); + store.setError(new Error('test')); await flushMicrotasks(); expect(listener1).toHaveBeenCalledTimes(1); @@ -95,7 +94,7 @@ describe('ProviderStateStore', () => { const unsubscribe = store.subscribe(listener); unsubscribe(); - store.setClientReady(true); + store.setError(new Error('test')); await flushMicrotasks(); expect(listener).not.toHaveBeenCalled(); @@ -108,7 +107,7 @@ describe('ProviderStateStore', () => { unsubscribe(); unsubscribe(); // Second call should not throw - store.setClientReady(true); + store.setError(new Error('test')); await flushMicrotasks(); expect(listener).not.toHaveBeenCalled(); }); @@ -120,7 +119,7 @@ describe('ProviderStateStore', () => { unsubscribe1(); const unsubscribe2 = store.subscribe(listener); - store.setClientReady(true); + store.setError(new Error('test')); await flushMicrotasks(); expect(listener).toHaveBeenCalledTimes(1); @@ -129,38 +128,6 @@ describe('ProviderStateStore', () => { }); }); - describe('setClientReady', () => { - it('should update isClientReady state', () => { - store.setClientReady(true); - - expect(store.getState().isClientReady).toBe(true); - }); - - it('should not notify if value has not changed', async () => { - const listener = vi.fn(); - store.subscribe(listener); - - store.setClientReady(false); // Same as initial value - await flushMicrotasks(); - - expect(listener).not.toHaveBeenCalled(); - }); - - it('should preserve other state properties', () => { - const mockUserContext = createMockUserContext(); - const mockError = new Error('test'); - - store.setUserContext(mockUserContext); - store.setError(mockError); - store.setClientReady(true); - - const state = store.getState(); - expect(state.userContext).toBe(mockUserContext); - expect(state.error).toBe(mockError); - expect(state.isClientReady).toBe(true); - }); - }); - describe('setUserContext', () => { it('should update userContext state', () => { const mockUserContext = createMockUserContext(); @@ -181,14 +148,12 @@ describe('ProviderStateStore', () => { it('should preserve other state properties', () => { const mockError = new Error('test'); - store.setClientReady(true); store.setError(mockError); const mockUserContext = createMockUserContext(); store.setUserContext(mockUserContext); const state = store.getState(); - expect(state.isClientReady).toBe(true); expect(state.error).toBe(mockError); }); }); @@ -227,13 +192,11 @@ describe('ProviderStateStore', () => { it('should not clear other state when error is set', () => { const mockUserContext = createMockUserContext(); - store.setClientReady(true); store.setUserContext(mockUserContext); store.setError(new Error('test')); const state = store.getState(); - expect(state.isClientReady).toBe(true); expect(state.userContext).toBe(mockUserContext); }); }); @@ -245,7 +208,6 @@ describe('ProviderStateStore', () => { const mockUserContext = createMockUserContext(); store.setState({ - isClientReady: true, userContext: mockUserContext, }); await flushMicrotasks(); @@ -254,7 +216,6 @@ describe('ProviderStateStore', () => { expect(listener).toHaveBeenCalledTimes(1); const state = store.getState(); - expect(state.isClientReady).toBe(true); expect(state.userContext).toBe(mockUserContext); }); @@ -263,28 +224,27 @@ describe('ProviderStateStore', () => { store.subscribe(listener); const mockUserContext = createMockUserContext(); - store.setClientReady(true); store.setUserContext(mockUserContext); store.setError(new Error('test')); await flushMicrotasks(); - // Three state changes, but only one notification due to microtask batching + // Two state changes, but only one notification due to microtask batching expect(listener).toHaveBeenCalledTimes(1); expect(listener).toHaveBeenCalledWith( expect.objectContaining({ - isClientReady: true, userContext: mockUserContext, }) ); }); it('should allow partial updates', () => { - store.setClientReady(true); + const mockUserContext = createMockUserContext(); + store.setUserContext(mockUserContext); store.setState({ error: new Error('test') }); const state = store.getState(); - expect(state.isClientReady).toBe(true); + expect(state.userContext).toBe(mockUserContext); expect(state.error).not.toBeNull(); }); }); @@ -292,21 +252,19 @@ describe('ProviderStateStore', () => { describe('reset', () => { it('should reset to initial state', () => { const mockUserContext = createMockUserContext(); - store.setClientReady(true); store.setUserContext(mockUserContext); store.setError(new Error('test')); store.reset(); const state = store.getState(); - expect(state.isClientReady).toBe(false); expect(state.userContext).toBeNull(); expect(state.error).toBeNull(); }); it('should notify listeners on reset', async () => { const listener = vi.fn(); - store.setClientReady(true); + store.setUserContext(createMockUserContext()); store.subscribe(listener); store.reset(); diff --git a/src/provider/ProviderStateStore.ts b/src/provider/ProviderStateStore.ts index 8840d79..92bc40e 100644 --- a/src/provider/ProviderStateStore.ts +++ b/src/provider/ProviderStateStore.ts @@ -31,7 +31,6 @@ export type ForcedDecisionListener = () => void; * Initial state for the provider store. */ const initialState: ProviderState = { - isClientReady: false, userContext: null, error: null, }; @@ -80,19 +79,6 @@ export class ProviderStateStore { }; } - /** - * Set whether the client is ready. - * e.g: Called by Provider after client.onReady() resolves. - */ - setClientReady(ready: boolean): void { - if (this.state.isClientReady === ready) { - return; - } - - this.state = { ...this.state, isClientReady: ready }; - this.notifyListeners(); - } - /** * Set the current user context. * e.g: Called by UserContextManager when user context is created. @@ -114,7 +100,7 @@ export class ProviderStateStore { /** * Set an error that occurred during initialization. - * Setting an error does NOT clear userContext or isClientReady. + * Setting an error does NOT clear userContext. */ setError(error: Error | null): void { if (this.state.error === error) { @@ -137,6 +123,16 @@ export class ProviderStateStore { this.notifyListeners(); } + /** + * Signal that external state (e.g. client config) has changed. + * Creates a new state reference so useSyncExternalStore triggers + * re-renders and hooks re-evaluate decisions. + */ + refresh(): void { + this.state = { ...this.state }; + this.notifyListeners(); + } + /** * Reset store to initial state. * Useful for testing or when Provider unmounts. diff --git a/src/provider/types.ts b/src/provider/types.ts index a02f5ad..c7b5c28 100644 --- a/src/provider/types.ts +++ b/src/provider/types.ts @@ -71,11 +71,6 @@ export interface OptimizelyProviderProps { * This is the reactive state that hooks subscribe to. */ export interface ProviderState { - /** - * Whether js onReady() is resolved. - */ - isClientReady: boolean; - /** * The current user context for making decisions. * null while initializing or if user creation failed.