[FSSDK-12296] Async Hook implementation#324
[FSSDK-12296] Async Hook implementation#324junaed-optimizely wants to merge 4 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds async counterparts to the existing “decide” React hooks so consumers can evaluate flags via the Optimizely SDK’s async decide APIs (needed for CMAB / async decisioning) while handling loading/error states, cancellation, and forced-decision reactivity.
Changes:
- Introduces
useDecideAsync,useDecideForKeysAsync, anduseDecideAllAsyncand exports them from the public entrypoints. - Adds shared
useAsyncDecisionstate machine to manage async execution, cancellation of stale promises, and store readiness transitions. - Adds comprehensive Vitest coverage for the 3 new hooks and extends hook test utilities with async decide method stubs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Exports the new async hooks from the package entrypoint. |
| src/hooks/index.ts | Exposes the new async hooks (and related types) from the hooks barrel. |
| src/hooks/useAsyncDecision.ts | Adds shared async state machine used by all async decide hooks. |
| src/hooks/useDecideAsync.ts | Implements async single-flag decision hook w/ forced decision subscription. |
| src/hooks/useDecideForKeysAsync.ts | Implements async multi-key decision hook w/ per-key forced decision subscription. |
| src/hooks/useDecideAllAsync.ts | Implements async all-flags decision hook w/ global forced decision subscription. |
| src/hooks/*.spec.tsx | Adds tests covering readiness states, cancellation, re-evaluation triggers, and forced-decision reactivity. |
| src/hooks/testUtils.tsx | Extends mock user context to support async decide APIs for testing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Store not ready — stay in loading | ||
| if (!hasConfig || userContext === null) { | ||
| setAsyncState({ result: emptyResult, error: null, isLoading: true }); |
There was a problem hiding this comment.
useAsyncDecision calls setAsyncState({ result: emptyResult, error: null, isLoading: true }) whenever the provider is not ready (!hasConfig || userContext === null). On initial mount (and on any store refresh while still not ready), this will schedule a state update even when the hook is already in the exact same loading state, causing an avoidable extra render. Consider using a functional setAsyncState with a guard (similar to the ready-path guard) so it returns prev when {isLoading:true,error:null,result===emptyResult} is already true.
| setAsyncState({ result: emptyResult, error: null, isLoading: true }); | |
| setAsyncState((prev) => { | |
| if (prev.isLoading && prev.error === null && prev.result === emptyResult) return prev; | |
| return { result: emptyResult, error: null, isLoading: true }; | |
| }); |
| interface AsyncState<TResult> { | ||
| result: TResult; | ||
| error: Error | null; | ||
| isLoading: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Shared async decision state machine used by useDecideAsync, | ||
| * useDecideForKeysAsync, and useDecideAllAsync. | ||
| * | ||
| * Handles: loading state, error propagation, cancellation of stale promises, | ||
| * and redundant re-render avoidance on first mount. | ||
| * | ||
| * @param state - Provider state from useProviderState | ||
| * @param client - Optimizely client instance | ||
| * @param fdVersion - Forced decision version counter (triggers re-evaluation) | ||
| * @param emptyResult - Default/empty result value (null for single, {} for multi) | ||
| * @param execute - Callback that performs the async SDK call | ||
| */ | ||
| export function useAsyncDecision<TResult>( | ||
| state: ProviderState, | ||
| client: Client, | ||
| fdVersion: number, | ||
| emptyResult: TResult, | ||
| execute: (userContext: OptimizelyUserContext) => Promise<TResult> | ||
| ): AsyncState<TResult> { | ||
| const [asyncState, setAsyncState] = useState<AsyncState<TResult>>({ | ||
| result: emptyResult, | ||
| error: null, | ||
| isLoading: true, | ||
| }); |
There was a problem hiding this comment.
The async hooks rely on as UseDecide*AsyncResult type assertions when returning { result, error, isLoading }, which can mask violations of the intended discriminated union (e.g., accidentally returning isLoading: true with a non-null error after future refactors). Consider making useAsyncDecision return a discriminated union (or a helper that maps its internal state into the union) so TypeScript enforces the invariants and the hooks can return without assertions.
Summary
useDecideAsync,useDecideForKeysAsync, anduseDecideAllAsynchooks — async counterparts to the existing sync decide hooks, required for CMAB, async UPS etc.Test plan
Issues