Skip to content

Commit 3105595

Browse files
beeme1mrDevelopergemini-code-assist[bot]lukas-reiningtoddbaert
authored
fix(react): re-evaluate flags on re-render to detect silent provider … (#1226)
## This PR - Added `useEffect` that runs on re-render to re-evaluate the flag value - Only updates state if the resolved value actually changed (using `isEqual` comparison) - Used lazy initialization for `useState` to avoid redundant initial evaluation - Added `useCallback` memoization for event handlers - Fixed `AbortController` scope issue ### Notes This resolves a subtle issue where the provider state may update without emitting a change event, leading to confusing results. The `useFlag` hook sets the initial evaluated value in a `useState`. Since this wasn't in a closure, this evaluation happened any time the component using the hook rerendered but the result was essentially ignored. Adding a logging hook shows that the current results but since this evaluation was made in an existing `useState`, the result had no effect. This resolves a subtle issue where the provider state may update without emitting a change event, leading to stale flag values being displayed. The `useFlag` hook was evaluating the flag on every re-render (as part of the `useState` initialization), but because `useState` only uses its initial value on the first render, these subsequent evaluations were being discarded. This meant that even though the hook was fetching the correct updated value from the provider on each re-render, it was throwing that value away and continuing to display the stale cached value. Adding a logging hook would show the correct evaluation happening (proving the provider had the updated value), but the UI would remain stuck with the old value because the `useState` was ignoring the re-evaluated result. The fix ensures that these re-evaluations on re-render actually update the component's state when the resolved value changes. The key insight is that the evaluation WAS happening on every re-render (due to how useState works), but React was discarding the result. Your fix makes those evaluations actually matter by checking if the value changed and updating state accordingly. Original thread: https://cloud-native.slack.com/archives/C06E4DE6S07/p1754508917397519 ### How to test I created a test that reproduced the issue, and it failed. I then implemented the changes and verified that the test passed. --------- Signed-off-by: Developer <[email protected]> Signed-off-by: Michael Beemer <[email protected]> Co-authored-by: Developer <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Lukas Reining <[email protected]> Co-authored-by: Todd Baert <[email protected]>
1 parent 1dbbd51 commit 3105595

File tree

3 files changed

+149
-19
lines changed

3 files changed

+149
-19
lines changed

packages/react/src/evaluation/use-feature-flag.ts

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import type {
88
JsonValue,
99
} from '@openfeature/web-sdk';
1010
import { ProviderEvents, ProviderStatus } from '@openfeature/web-sdk';
11-
import { useEffect, useRef, useState } from 'react';
11+
import { useCallback, useEffect, useRef, useState } from 'react';
1212
import {
1313
DEFAULT_OPTIONS,
1414
isEqual,
@@ -287,8 +287,7 @@ function attachHandlersAndResolve<T extends FlagValue>(
287287
const client = useOpenFeatureClient();
288288
const status = useOpenFeatureClientStatus();
289289
const provider = useOpenFeatureProvider();
290-
291-
const controller = new AbortController();
290+
const isFirstRender = useRef(true);
292291

293292
if (defaultedOptions.suspendUntilReady && status === ProviderStatus.NOT_READY) {
294293
suspendUntilInitialized(provider, client);
@@ -298,17 +297,30 @@ function attachHandlersAndResolve<T extends FlagValue>(
298297
suspendUntilReconciled(client);
299298
}
300299

301-
const [evaluationDetails, setEvaluationDetails] = useState<EvaluationDetails<T>>(
300+
const [evaluationDetails, setEvaluationDetails] = useState<EvaluationDetails<T>>(() =>
302301
resolver(client).call(client, flagKey, defaultValue, options),
303302
);
303+
304+
// Re-evaluate when dependencies change (handles prop changes like flagKey), or if during a re-render, we have detected a change in the evaluated value
305+
useEffect(() => {
306+
if (isFirstRender.current) {
307+
isFirstRender.current = false;
308+
return;
309+
}
310+
311+
const newDetails = resolver(client).call(client, flagKey, defaultValue, options);
312+
if (!isEqual(newDetails.value, evaluationDetails.value)) {
313+
setEvaluationDetails(newDetails);
314+
}
315+
}, [client, flagKey, defaultValue, options, resolver, evaluationDetails]);
304316

305317
// Maintain a mutable reference to the evaluation details to have a up-to-date reference in the handlers.
306318
const evaluationDetailsRef = useRef<EvaluationDetails<T>>(evaluationDetails);
307319
useEffect(() => {
308320
evaluationDetailsRef.current = evaluationDetails;
309321
}, [evaluationDetails]);
310322

311-
const updateEvaluationDetailsCallback = () => {
323+
const updateEvaluationDetailsCallback = useCallback(() => {
312324
const updatedEvaluationDetails = resolver(client).call(client, flagKey, defaultValue, options);
313325

314326
/**
@@ -319,15 +331,19 @@ function attachHandlersAndResolve<T extends FlagValue>(
319331
if (!isEqual(updatedEvaluationDetails.value, evaluationDetailsRef.current.value)) {
320332
setEvaluationDetails(updatedEvaluationDetails);
321333
}
322-
};
334+
}, [client, flagKey, defaultValue, options, resolver]);
323335

324-
const configurationChangeCallback: EventHandler<ClientProviderEvents.ConfigurationChanged> = (eventDetails) => {
325-
if (shouldEvaluateFlag(flagKey, eventDetails?.flagsChanged)) {
326-
updateEvaluationDetailsCallback();
327-
}
328-
};
336+
const configurationChangeCallback = useCallback<EventHandler<ClientProviderEvents.ConfigurationChanged>>(
337+
(eventDetails) => {
338+
if (shouldEvaluateFlag(flagKey, eventDetails?.flagsChanged)) {
339+
updateEvaluationDetailsCallback();
340+
}
341+
},
342+
[flagKey, updateEvaluationDetailsCallback],
343+
);
329344

330345
useEffect(() => {
346+
const controller = new AbortController();
331347
if (status === ProviderStatus.NOT_READY) {
332348
// update when the provider is ready
333349
client.addHandler(ProviderEvents.Ready, updateEvaluationDetailsCallback, { signal: controller.signal });
@@ -348,7 +364,14 @@ function attachHandlersAndResolve<T extends FlagValue>(
348364
// cleanup the handlers
349365
controller.abort();
350366
};
351-
}, []);
367+
}, [
368+
client,
369+
status,
370+
defaultedOptions.updateOnContextChanged,
371+
defaultedOptions.updateOnConfigurationChanged,
372+
updateEvaluationDetailsCallback,
373+
configurationChangeCallback,
374+
]);
352375

353376
return evaluationDetails;
354377
}

packages/react/test/evaluation.spec.tsx

Lines changed: 113 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -924,17 +924,124 @@ describe('evaluation', () => {
924924
OpenFeature.setContext(SUSPEND_OFF, { user: TARGETED_USER });
925925
});
926926

927-
// expect to see static value until we reconcile
928-
await waitFor(() => expect(screen.queryByText(STATIC_FLAG_VALUE_A)).toBeInTheDocument(), {
929-
timeout: DELAY / 2,
930-
});
931-
932-
// make sure we updated after reconciling
927+
// With the fix for useState initialization, the hook now immediately
928+
// reflects provider state changes. This is intentional to handle cases
929+
// where providers don't emit proper events.
930+
// The value updates immediately to the targeted value.
933931
await waitFor(() => expect(screen.queryByText(TARGETED_FLAG_VALUE)).toBeInTheDocument(), {
934932
timeout: DELAY * 2,
935933
});
936934
});
937935
});
936+
937+
describe('re-render behavior when flag values change without provider events', ()=> {
938+
it('should reflect provider state changes on re-render even without provider events', async () => {
939+
let providerValue = 'initial-value';
940+
941+
class SilentUpdateProvider extends InMemoryProvider {
942+
resolveBooleanEvaluation() {
943+
return {
944+
value: true,
945+
variant: 'on',
946+
reason: StandardResolutionReasons.STATIC,
947+
};
948+
}
949+
950+
resolveStringEvaluation() {
951+
return {
952+
value: providerValue,
953+
variant: providerValue,
954+
reason: StandardResolutionReasons.STATIC,
955+
};
956+
}
957+
}
958+
959+
const provider = new SilentUpdateProvider({});
960+
await OpenFeature.setProviderAndWait('test', provider);
961+
962+
// The triggerRender prop forces a re-render
963+
const TestComponent = ({ triggerRender }: { triggerRender: number }) => {
964+
const { value } = useFlag('test-flag', 'default');
965+
return <div data-testid="flag-value" data-render-count={triggerRender}>{value}</div>;
966+
};
967+
968+
const WrapperComponent = () => {
969+
const [renderCount, setRenderCount] = useState(0);
970+
return (
971+
<>
972+
<button onClick={() => setRenderCount(c => c + 1)}>Force Re-render</button>
973+
<TestComponent triggerRender={renderCount} />
974+
</>
975+
);
976+
};
977+
978+
const { getByText } = render(
979+
<OpenFeatureProvider client={OpenFeature.getClient('test')}>
980+
<WrapperComponent />
981+
</OpenFeatureProvider>
982+
);
983+
984+
// Initial value should be rendered
985+
await waitFor(() => {
986+
expect(screen.getByTestId('flag-value')).toHaveTextContent('initial-value');
987+
});
988+
989+
// Change the provider's internal state (without emitting events)
990+
providerValue = 'updated-value';
991+
992+
// Force a re-render of the component
993+
act(() => {
994+
getByText('Force Re-render').click();
995+
});
996+
997+
await waitFor(() => {
998+
expect(screen.getByTestId('flag-value')).toHaveTextContent('updated-value');
999+
});
1000+
});
1001+
1002+
it('should update flag value when flag key prop changes without provider events', async () => {
1003+
const provider = new InMemoryProvider({
1004+
'flag-a': {
1005+
disabled: false,
1006+
variants: { on: 'value-a' },
1007+
defaultVariant: 'on',
1008+
},
1009+
'flag-b': {
1010+
disabled: false,
1011+
variants: { on: 'value-b' },
1012+
defaultVariant: 'on',
1013+
},
1014+
});
1015+
1016+
await OpenFeature.setProviderAndWait(EVALUATION, provider);
1017+
1018+
const TestComponent = ({ flagKey }: { flagKey: string }) => {
1019+
const { value } = useFlag(flagKey, 'default');
1020+
return <div data-testid="flag-value">{value}</div>;
1021+
};
1022+
1023+
const { rerender } = render(
1024+
<OpenFeatureProvider client={OpenFeature.getClient(EVALUATION)}>
1025+
<TestComponent flagKey="flag-a" />
1026+
</OpenFeatureProvider>
1027+
);
1028+
1029+
await waitFor(() => {
1030+
expect(screen.getByTestId('flag-value')).toHaveTextContent('value-a');
1031+
});
1032+
1033+
// Change to flag-b (without any provider events)
1034+
rerender(
1035+
<OpenFeatureProvider client={OpenFeature.getClient(EVALUATION)}>
1036+
<TestComponent flagKey="flag-b" />
1037+
</OpenFeatureProvider>
1038+
);
1039+
1040+
await waitFor(() => {
1041+
expect(screen.getByTestId('flag-value')).toHaveTextContent('value-b');
1042+
});
1043+
});
1044+
});
9381045
});
9391046

9401047
describe('context, hooks and options', () => {

packages/react/test/provider.spec.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ describe('OpenFeatureProvider', () => {
288288
{ timeout: DELAY * 4 },
289289
);
290290

291-
expect(screen.getByText('Will says hi')).toBeInTheDocument();
291+
expect(screen.getByText('Will says aloha')).toBeInTheDocument();
292292
});
293293
});
294294
});

0 commit comments

Comments
 (0)