Skip to content

Commit 8133a4f

Browse files
authored
fix: compare full EvaluationDetails to prevent stale data (#1287)
## This PR - updated the `isEqual` function to compare any input - updated the compare to evaluate the whole EvaluationDetails object instead of just the value ### Related Issues Fixes #1286 ### Notes This may trigger slightly more re-renders but I feel that that's better than return stale EvaluationDetails data which has proven to be confusing to folks attempting to debug unexpected evaluations. --------- Signed-off-by: Michael Beemer <[email protected]>
1 parent 9b05be9 commit 8133a4f

File tree

3 files changed

+84
-12
lines changed

3 files changed

+84
-12
lines changed

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

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -307,9 +307,9 @@ function attachHandlersAndResolve<T extends FlagValue>(
307307
isFirstRender.current = false;
308308
return;
309309
}
310-
310+
311311
const newDetails = resolver(client).call(client, flagKey, defaultValue, options);
312-
if (!isEqual(newDetails.value, evaluationDetails.value)) {
312+
if (!isEqual(newDetails, evaluationDetails)) {
313313
setEvaluationDetails(newDetails);
314314
}
315315
}, [client, flagKey, defaultValue, options, resolver, evaluationDetails]);
@@ -324,11 +324,9 @@ function attachHandlersAndResolve<T extends FlagValue>(
324324
const updatedEvaluationDetails = resolver(client).call(client, flagKey, defaultValue, options);
325325

326326
/**
327-
* Avoid re-rendering if the value hasn't changed. We could expose a means
328-
* to define a custom comparison function if users require a more
329-
* sophisticated comparison in the future.
327+
* Avoid re-rendering if the evaluation details haven't changed.
330328
*/
331-
if (!isEqual(updatedEvaluationDetails.value, evaluationDetailsRef.current.value)) {
329+
if (!isEqual(updatedEvaluationDetails, evaluationDetailsRef.current)) {
332330
setEvaluationDetails(updatedEvaluationDetails);
333331
}
334332
}, [client, flagKey, defaultValue, options, resolver]);

packages/react/src/internal/is-equal.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
1-
import { type FlagValue } from '@openfeature/web-sdk';
2-
31
/**
42
* Deeply compare two values to determine if they are equal.
53
* Supports primitives and serializable objects.
6-
* @param {FlagValue} value First value to compare
7-
* @param {FlagValue} other Second value to compare
4+
*
5+
* Note: Does not handle Date, RegExp, Map, Set, or circular references.
6+
* Suitable for comparing EvaluationDetails and other JSON-serializable data.
7+
* @param {unknown} value First value to compare
8+
* @param {unknown} other Second value to compare
89
* @returns {boolean} True if the values are equal
910
*/
10-
export function isEqual(value: FlagValue, other: FlagValue): boolean {
11+
export function isEqual(value: unknown, other: unknown): boolean {
1112
if (value === other) {
1213
return true;
1314
}
@@ -16,7 +17,7 @@ export function isEqual(value: FlagValue, other: FlagValue): boolean {
1617
return false;
1718
}
1819

19-
if (typeof value === 'object' && value !== null && other !== null) {
20+
if (typeof value === 'object' && value !== null && typeof other === 'object' && other !== null) {
2021
const valueKeys = Object.keys(value);
2122
const otherKeys = Object.keys(other);
2223

packages/react/test/evaluation.spec.tsx

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,79 @@ describe('evaluation', () => {
585585
return new TestingProvider(CONFIG, DELAY); // delay init by 100ms
586586
};
587587

588+
describe('provider ready event updates', () => {
589+
it('should update EvaluationDetails when provider becomes ready (including reason)', async () => {
590+
const PROVIDER_READY_DOMAIN = 'provider-ready-test';
591+
const TEST_FLAG_KEY = 'test-flag';
592+
const FLAG_VALUE = true;
593+
594+
// Create a provider that will delay initialization
595+
const provider = new TestingProvider(
596+
{
597+
[TEST_FLAG_KEY]: {
598+
disabled: false,
599+
variants: {
600+
on: FLAG_VALUE,
601+
off: false,
602+
},
603+
defaultVariant: 'on',
604+
},
605+
},
606+
DELAY,
607+
);
608+
609+
OpenFeature.setProvider(PROVIDER_READY_DOMAIN, provider);
610+
611+
let capturedDetails: EvaluationDetails<boolean> | undefined;
612+
let renderCount = 0;
613+
614+
function TestComponent() {
615+
renderCount++;
616+
const details = useBooleanFlagDetails(TEST_FLAG_KEY, FLAG_VALUE);
617+
capturedDetails = details;
618+
619+
return (
620+
<div>
621+
<div data-testid="value">{String(details.value)}</div>
622+
<div data-testid="reason">{details.reason}</div>
623+
<div data-testid="render-count">{renderCount}</div>
624+
</div>
625+
);
626+
}
627+
628+
render(
629+
<OpenFeatureProvider domain={PROVIDER_READY_DOMAIN}>
630+
<TestComponent />
631+
</OpenFeatureProvider>,
632+
);
633+
634+
// Initial render - provider is NOT_READY, should use default value with ERROR reason
635+
expect(capturedDetails?.value).toBe(FLAG_VALUE);
636+
expect(capturedDetails?.reason).toBe(StandardResolutionReasons.ERROR);
637+
expect(capturedDetails?.errorCode).toBe(ErrorCode.PROVIDER_NOT_READY);
638+
expect(screen.getByTestId('reason')).toHaveTextContent(StandardResolutionReasons.ERROR);
639+
640+
const initialRenderCount = renderCount;
641+
642+
// Wait for provider to become ready and component to re-render
643+
await waitFor(
644+
() => {
645+
expect(capturedDetails?.reason).toBe(StandardResolutionReasons.STATIC);
646+
},
647+
{ timeout: DELAY * 2 },
648+
);
649+
650+
// After provider is ready, same value but different reason and no error
651+
expect(capturedDetails?.value).toBe(FLAG_VALUE);
652+
expect(capturedDetails?.errorCode).toBeUndefined();
653+
expect(screen.getByTestId('value')).toHaveTextContent(String(FLAG_VALUE));
654+
expect(screen.getByTestId('reason')).toHaveTextContent(StandardResolutionReasons.STATIC);
655+
656+
// Verify that a re-render occurred
657+
expect(renderCount).toBeGreaterThan(initialRenderCount);
658+
});
659+
});
660+
588661
describe('when using the noop provider', () => {
589662
function TestComponent() {
590663
const { value } = useSuspenseFlag(SUSPENSE_FLAG_KEY, DEFAULT);

0 commit comments

Comments
 (0)