From c80278e156c5ff6ffc48f4da0bac776caadff431 Mon Sep 17 00:00:00 2001 From: Md Junaed Hossain <169046794+junaed-optimizely@users.noreply.github.com> Date: Tue, 13 Aug 2024 20:35:08 +0600 Subject: [PATCH 1/6] [FSSDK-10544] hook init subscription code refactor --- src/hooks.spec.tsx | 2 +- src/hooks.ts | 65 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/hooks.spec.tsx b/src/hooks.spec.tsx index 97f7273..ffe0c19 100644 --- a/src/hooks.spec.tsx +++ b/src/hooks.spec.tsx @@ -62,7 +62,7 @@ const mockFeatureVariables: VariableValuesObject = { foo: 'bar', }; -describe('hooks', () => { +describe.skip('hooks', () => { let activateMock: jest.Mock; let featureVariables: VariableValuesObject; let getOnReadyPromise: any; diff --git a/src/hooks.ts b/src/hooks.ts index 8a2d928..a1ed121 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -148,10 +148,18 @@ function subscribeToInitialization( clientReady: false, didTimeout: false, }); - res.dataReadyPromise?.then(() => { - hooksLogger.info('Client became ready.'); + res.dataReadyPromise?.then((readyResult?: OnReadyResult) => { + if (!readyResult) { + return; + } + const { success, message } = readyResult; + if (success) { + hooksLogger.info('Client became ready.'); + } else { + hooksLogger.warn(`Client not ready, reason="${message}"`); + } onInitStateChange({ - clientReady: true, + clientReady: success, didTimeout: false, }); }); @@ -162,10 +170,18 @@ function subscribeToInitialization( clientReady: false, didTimeout: false, }); - res.dataReadyPromise?.then(() => { - hooksLogger.info('User became ready later.'); + res.dataReadyPromise?.then((readyResult?: OnReadyResult) => { + if (!readyResult) { + return; + } + const { success, message } = readyResult; + if (success) { + hooksLogger.info('User became ready later.'); + } else { + hooksLogger.warn(`Client not ready, reason="${message}"`); + } onInitStateChange({ - clientReady: true, + clientReady: success, didTimeout: false, }); }); @@ -176,10 +192,21 @@ function subscribeToInitialization( clientReady: false, didTimeout: true, }); - res.dataReadyPromise?.then(() => { - hooksLogger.info('Client became ready after timeout already elapsed'); + res.dataReadyPromise?.then((readyResult?: OnReadyResult) => { + if (!readyResult) { + return; + } + + const { success, message } = readyResult; + + if (success) { + hooksLogger.info('Client became ready after timeout already elapsed'); + } else { + hooksLogger.warn(`Client not ready, reason="${message}"`); + } + onInitStateChange({ - clientReady: true, + clientReady: success, didTimeout: true, }); }); @@ -188,13 +215,23 @@ function subscribeToInitialization( hooksLogger.warn(`Other reason client not ready, reason="${res.message}"`); onInitStateChange({ clientReady: false, - didTimeout: true, // assume timeout + didTimeout: false, }); - res.dataReadyPromise?.then(() => { - hooksLogger.info('Client became ready later'); + res.dataReadyPromise?.then((readyResult?: OnReadyResult) => { + if (!readyResult) { + return; + } + + const { success, message } = readyResult; + + if (success) { + hooksLogger.info('Client became ready later'); + } else { + hooksLogger.warn(`Client not ready, reason="${message}"`); + } onInitStateChange({ - clientReady: true, - didTimeout: true, // assume timeout + clientReady: success, + didTimeout: false, }); }); } From 0f19e7fe4655a100de12791ade470c62bdb49940 Mon Sep 17 00:00:00 2001 From: Md Junaed Hossain <169046794+junaed-optimizely@users.noreply.github.com> Date: Wed, 14 Aug 2024 17:35:23 +0600 Subject: [PATCH 2/6] [FSSDK-10544] useExperiment code + test refactor --- src/hooks.spec.tsx | 74 +++++++++++++++++++++++++++++----------------- src/hooks.ts | 34 +++++++++++---------- 2 files changed, 66 insertions(+), 42 deletions(-) diff --git a/src/hooks.spec.tsx b/src/hooks.spec.tsx index ffe0c19..339d504 100644 --- a/src/hooks.spec.tsx +++ b/src/hooks.spec.tsx @@ -22,7 +22,7 @@ import { render, renderHook, screen, waitFor } from '@testing-library/react'; import '@testing-library/jest-dom'; import { OptimizelyProvider } from './Provider'; -import { OnReadyResult, ReactSDKClient, VariableValuesObject } from './client'; +import { NotReadyReason, OnReadyResult, ReactSDKClient, VariableValuesObject } from './client'; import { useExperiment, useFeature, useDecision, useTrackEvent, hooksLogger } from './hooks'; import { OptimizelyDecision } from './utils'; const defaultDecision: OptimizelyDecision = { @@ -62,7 +62,7 @@ const mockFeatureVariables: VariableValuesObject = { foo: 'bar', }; -describe.skip('hooks', () => { +describe('hooks', () => { let activateMock: jest.Mock; let featureVariables: VariableValuesObject; let getOnReadyPromise: any; @@ -176,52 +176,79 @@ describe.skip('hooks', () => { it('should return a variation when activate returns a variation', async () => { activateMock.mockReturnValue('12345'); - const { container } = render( + render( ); - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('12345|true|false')); }); it('should return null when activate returns null', async () => { activateMock.mockReturnValue(null); featureVariables = {}; - const { container } = render( + render( ); - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|true|false')); }); it('should respect the timeout option passed', async () => { activateMock.mockReturnValue(null); + mockDelay = 100; readySuccess = false; + // Todo: getOnReadyPromise might be moved in the top later + getOnReadyPromise = ({ timeout = 0 }: any): Promise => { + const timeoutPromise = new Promise((resolve) => { + setTimeout( + () => { + resolve({ + success: false, + reason: NotReadyReason.TIMEOUT, + dataReadyPromise: new Promise((r) => + setTimeout( + () => + r({ + success: readySuccess, + }), + mockDelay + ) + ), + }); + }, + timeout || mockDelay + 1 + ); + }); + + const clientAndUserReadyPromise = new Promise((resolve) => { + setTimeout(() => { + resolve({ + success: readySuccess, + }); + }, mockDelay); + }); + + return Promise.race([clientAndUserReadyPromise, timeoutPromise]); + }; render( - + ); - - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|false|false')); // initial render - - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|false|true')); // when didTimeout - - // Simulate datafile fetch completing after timeout has already passed - // Activate now returns a variation + expect(screen.getByTestId('result')).toHaveTextContent('null|false|false'); // initial render + readySuccess = true; activateMock.mockReturnValue('12345'); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('12345|true|true')); // when clientReady + // When timeout is reached, but dataReadyPromise is resolved later with the variation + await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('12345|true|true')); }); it('should gracefully handle the client promise rejecting after timeout', async () => { - jest.useFakeTimers(); - readySuccess = false; activateMock.mockReturnValue('12345'); + getOnReadyPromise = (): Promise => new Promise((_, rej) => setTimeout(() => rej(REJECTION_REASON), mockDelay)); @@ -231,11 +258,7 @@ describe.skip('hooks', () => { ); - jest.advanceTimersByTime(mockDelay + 1); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|false|false')); - - jest.useRealTimers(); }); it('should re-render when the user attributes change using autoUpdate', async () => { @@ -249,7 +272,6 @@ describe.skip('hooks', () => { // TODO - Wrap this with async act() once we upgrade to React 16.9 // See https://github.com/facebook/react/issues/15379 - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|true|false')); activateMock.mockReturnValue('12345'); @@ -257,8 +279,7 @@ describe.skip('hooks', () => { await act(async () => { userUpdateCallbacks.forEach((fn) => fn()); }); - // component.update(); - // await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('12345|true|false'); + await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('12345|true|false')); }); @@ -272,7 +293,6 @@ describe.skip('hooks', () => { // // TODO - Wrap this with async act() once we upgrade to React 16.9 // // See https://github.com/facebook/react/issues/15379 - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|true|false')); activateMock.mockReturnValue('12345'); @@ -425,7 +445,7 @@ describe.skip('hooks', () => { }); }); - describe('useFeature', () => { + describe.skip('useFeature', () => { it('should render true when the feature is enabled', async () => { isFeatureEnabledMock.mockReturnValue(true); @@ -676,7 +696,7 @@ describe.skip('hooks', () => { }); }); - describe('useDecision', () => { + describe.skip('useDecision', () => { it('should render true when the flag is enabled', async () => { decideMock.mockReturnValue({ ...defaultDecision, diff --git a/src/hooks.ts b/src/hooks.ts index a1ed121..627a1c1 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -1,3 +1,4 @@ +/* eslint-disable react-hooks/rules-of-hooks */ /** * Copyright 2018-2019, 2022-2024, Optimizely * @@ -14,7 +15,6 @@ * limitations under the License. */ -/* eslint-disable react-hooks/rules-of-hooks */ import { useCallback, useContext, useEffect, useState, useRef } from 'react'; import { UserAttributes, OptimizelyDecideOption, getLogger } from '@optimizely/optimizely-sdk'; @@ -259,22 +259,18 @@ function useCompareAttrsMemoize(value: UserAttributes | undefined): UserAttribut export const useExperiment: UseExperiment = (experimentKey, options = {}, overrides = {}) => { const { optimizely, isServerSide, timeout } = useContext(OptimizelyContext); - if (!optimizely) { - hooksLogger.error( - `Unable to use experiment ${experimentKey}. optimizely prop must be supplied via a parent ` - ); - return [null, false, false]; - } - const overrideAttrs = useCompareAttrsMemoize(overrides.overrideAttributes); + const getCurrentDecision: () => ExperimentDecisionValues = useCallback( () => ({ - variation: optimizely.activate(experimentKey, overrides.overrideUserId, overrideAttrs), + variation: optimizely?.activate(experimentKey, overrides.overrideUserId, overrideAttrs) || null, }), [optimizely, experimentKey, overrides.overrideUserId, overrideAttrs] ); - const isClientReady = isServerSide || optimizely.isReady(); + const isClientReady = isServerSide || !!optimizely?.isReady(); + const isReadyPromiseFulfilled = !!optimizely?.getIsReadyPromiseFulfilled(); + const [state, setState] = useState(() => { const decisionState = isClientReady ? getCurrentDecision() : { variation: null }; return { @@ -293,6 +289,7 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri overrideUserId: overrides.overrideUserId, overrideAttributes: overrideAttrs, }; + const [prevDecisionInputs, setPrevDecisionInputs] = useState(currentDecisionInputs); if (!areDecisionInputsEqual(prevDecisionInputs, currentDecisionInputs)) { setPrevDecisionInputs(currentDecisionInputs); @@ -309,7 +306,7 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri // and we need to wait for the promise and update decision. // 2. When client is using datafile only but client is not ready yet which means user // was provided as a promise and we need to subscribe and wait for user to become available. - if ((optimizely.getIsUsingSdkKey() && !optimizely.getIsReadyPromiseFulfilled()) || !isClientReady) { + if (optimizely && ((optimizely.getIsUsingSdkKey() && !isReadyPromiseFulfilled) || !isClientReady)) { subscribeToInitialization(optimizely, finalReadyTimeout, (initState) => { setState({ ...getCurrentDecision(), @@ -317,11 +314,11 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri }); }); } - }, []); + }, [finalReadyTimeout, getCurrentDecision, isClientReady, isReadyPromiseFulfilled, optimizely]); useEffect(() => { // Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering. - if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) { + if (optimizely && isReadyPromiseFulfilled && options.autoUpdate) { return setupAutoUpdateListeners(optimizely, HookType.EXPERIMENT, experimentKey, hooksLogger, () => { setState((prevState) => ({ ...prevState, @@ -330,11 +327,11 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri }); } return (): void => {}; - }, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, experimentKey, getCurrentDecision]); + }, [isReadyPromiseFulfilled, options.autoUpdate, optimizely, experimentKey, getCurrentDecision]); useEffect( () => - optimizely.onForcedVariationsUpdate(() => { + optimizely?.onForcedVariationsUpdate(() => { setState((prevState) => ({ ...prevState, ...getCurrentDecision(), @@ -342,6 +339,13 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri }), [getCurrentDecision, optimizely] ); + + if (!optimizely) { + hooksLogger.error( + `Unable to use experiment ${experimentKey}. optimizely prop must be supplied via a parent ` + ); + } + return [state.variation, state.clientReady, state.didTimeout]; }; From 8fc45b034c2163b1274e332f408065a773a474c7 Mon Sep 17 00:00:00 2001 From: Md Junaed Hossain <169046794+junaed-optimizely@users.noreply.github.com> Date: Wed, 14 Aug 2024 18:04:23 +0600 Subject: [PATCH 3/6] [FSSDK-10544] useFeature code + test refactor --- src/hooks.spec.tsx | 101 ++++++++++++++++++--------------------------- src/hooks.ts | 36 +++++++++------- 2 files changed, 61 insertions(+), 76 deletions(-) diff --git a/src/hooks.spec.tsx b/src/hooks.spec.tsx index 339d504..83f2b2d 100644 --- a/src/hooks.spec.tsx +++ b/src/hooks.spec.tsx @@ -82,7 +82,38 @@ describe('hooks', () => { let setForcedDecisionMock: jest.Mock; let hooksLoggerErrorSpy: jest.SpyInstance; const REJECTION_REASON = 'A rejection reason you should never see in the test runner'; + const getOnReadyTimeoutPromise = ({ timeout = 0 }: any): Promise => { + const timeoutPromise = new Promise((resolve) => { + setTimeout( + () => { + resolve({ + success: false, + reason: NotReadyReason.TIMEOUT, + dataReadyPromise: new Promise((r) => + setTimeout( + () => + r({ + success: readySuccess, + }), + mockDelay + ) + ), + }); + }, + timeout || mockDelay + 1 + ); + }); + + const clientAndUserReadyPromise = new Promise((resolve) => { + setTimeout(() => { + resolve({ + success: readySuccess, + }); + }, mockDelay); + }); + return Promise.race([clientAndUserReadyPromise, timeoutPromise]); + }; beforeEach(() => { getOnReadyPromise = ({ timeout = 0 }: any): Promise => new Promise((resolve) => { @@ -200,39 +231,7 @@ describe('hooks', () => { mockDelay = 100; readySuccess = false; // Todo: getOnReadyPromise might be moved in the top later - getOnReadyPromise = ({ timeout = 0 }: any): Promise => { - const timeoutPromise = new Promise((resolve) => { - setTimeout( - () => { - resolve({ - success: false, - reason: NotReadyReason.TIMEOUT, - dataReadyPromise: new Promise((r) => - setTimeout( - () => - r({ - success: readySuccess, - }), - mockDelay - ) - ), - }); - }, - timeout || mockDelay + 1 - ); - }); - - const clientAndUserReadyPromise = new Promise((resolve) => { - setTimeout(() => { - resolve({ - success: readySuccess, - }); - }, mockDelay); - }); - - return Promise.race([clientAndUserReadyPromise, timeoutPromise]); - }; - + getOnReadyPromise = getOnReadyTimeoutPromise; render( @@ -445,7 +444,7 @@ describe('hooks', () => { }); }); - describe.skip('useFeature', () => { + describe('useFeature', () => { it('should render true when the feature is enabled', async () => { isFeatureEnabledMock.mockReturnValue(true); @@ -454,7 +453,6 @@ describe('hooks', () => { ); - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('true|{"foo":"bar"}|true|false')); }); @@ -467,44 +465,33 @@ describe('hooks', () => { ); - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); }); it('should respect the timeout option passed', async () => { + mockDelay = 100; isFeatureEnabledMock.mockReturnValue(false); featureVariables = {}; readySuccess = false; - + getOnReadyPromise = getOnReadyTimeoutPromise; render( - + ); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|false')); // initial render - - await optimizelyMock.onReady(); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|true')); // when didTimeout + // Initial render + expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|false'); - // Simulate datafile fetch completing after timeout has already passed - // isFeatureEnabled now returns true, getFeatureVariables returns variable values + readySuccess = true; isFeatureEnabledMock.mockReturnValue(true); featureVariables = mockFeatureVariables; - // Wait for completion of dataReadyPromise - await optimizelyMock.onReady().then((res) => res.dataReadyPromise); - // Simulate datafile fetch completing after timeout has already passed - // Activate now returns a variation - activateMock.mockReturnValue('12345'); - // Wait for completion of dataReadyPromise - await optimizelyMock.onReady().then((res) => res.dataReadyPromise); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('true|{"foo":"bar"}|true|true')); // when clientReady + // When timeout is reached, but dataReadyPromise is resolved later with the feature enabled + await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('true|{"foo":"bar"}|true|true')); }); it('should gracefully handle the client promise rejecting after timeout', async () => { - jest.useFakeTimers(); - readySuccess = false; isFeatureEnabledMock.mockReturnValue(true); getOnReadyPromise = (): Promise => @@ -516,11 +503,7 @@ describe('hooks', () => { ); - jest.advanceTimersByTime(mockDelay + 1); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|false')); - - jest.useRealTimers(); }); it('should re-render when the user attributes change using autoUpdate', async () => { @@ -535,7 +518,6 @@ describe('hooks', () => { // TODO - Wrap this with async act() once we upgrade to React 16.9 // See https://github.com/facebook/react/issues/15379 - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); isFeatureEnabledMock.mockReturnValue(true); @@ -559,7 +541,6 @@ describe('hooks', () => { // TODO - Wrap this with async act() once we upgrade to React 16.9 // See https://github.com/facebook/react/issues/15379 - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); isFeatureEnabledMock.mockReturnValue(true); diff --git a/src/hooks.ts b/src/hooks.ts index 627a1c1..e151a56 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -14,7 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - import { useCallback, useContext, useEffect, useState, useRef } from 'react'; import { UserAttributes, OptimizelyDecideOption, getLogger } from '@optimizely/optimizely-sdk'; @@ -358,24 +357,19 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri */ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {}) => { const { optimizely, isServerSide, timeout } = useContext(OptimizelyContext); - - if (!optimizely) { - hooksLogger.error( - `Unable to properly use feature ${featureKey}. optimizely prop must be supplied via a parent ` - ); - return [false, {}, false, false]; - } - const overrideAttrs = useCompareAttrsMemoize(overrides.overrideAttributes); + const getCurrentDecision: () => FeatureDecisionValues = useCallback( () => ({ - isEnabled: optimizely.isFeatureEnabled(featureKey, overrides.overrideUserId, overrideAttrs), - variables: optimizely.getFeatureVariables(featureKey, overrides.overrideUserId, overrideAttrs), + isEnabled: !!optimizely?.isFeatureEnabled(featureKey, overrides.overrideUserId, overrideAttrs), + variables: optimizely?.getFeatureVariables(featureKey, overrides.overrideUserId, overrideAttrs) || {}, }), [optimizely, featureKey, overrides.overrideUserId, overrideAttrs] ); - const isClientReady = isServerSide || optimizely.isReady(); + const isClientReady = isServerSide || !!optimizely?.isReady(); + const isReadyPromiseFulfilled = !!optimizely?.getIsReadyPromiseFulfilled(); + const [state, setState] = useState(() => { const decisionState = isClientReady ? getCurrentDecision() : { isEnabled: false, variables: {} }; return { @@ -393,7 +387,9 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {}) overrideUserId: overrides.overrideUserId, overrideAttributes: overrides.overrideAttributes, }; + const [prevDecisionInputs, setPrevDecisionInputs] = useState(currentDecisionInputs); + if (!areDecisionInputsEqual(prevDecisionInputs, currentDecisionInputs)) { setPrevDecisionInputs(currentDecisionInputs); setState((prevState) => ({ @@ -403,13 +399,14 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {}) } const finalReadyTimeout = options.timeout !== undefined ? options.timeout : timeout; + useEffect(() => { // Subscribe to initialzation promise only // 1. When client is using Sdk Key, which means the initialization will be asynchronous // and we need to wait for the promise and update decision. // 2. When client is using datafile only but client is not ready yet which means user // was provided as a promise and we need to subscribe and wait for user to become available. - if (optimizely.getIsUsingSdkKey() || !isClientReady) { + if (optimizely && (optimizely.getIsUsingSdkKey() || !isClientReady)) { subscribeToInitialization(optimizely, finalReadyTimeout, (initState) => { setState({ ...getCurrentDecision(), @@ -417,11 +414,12 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {}) }); }); } - }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [finalReadyTimeout, getCurrentDecision, optimizely]); useEffect(() => { // Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering. - if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) { + if (optimizely && isReadyPromiseFulfilled && options.autoUpdate) { return setupAutoUpdateListeners(optimizely, HookType.FEATURE, featureKey, hooksLogger, () => { setState((prevState) => ({ ...prevState, @@ -430,7 +428,13 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {}) }); } return (): void => {}; - }, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, featureKey, getCurrentDecision]); + }, [isReadyPromiseFulfilled, options.autoUpdate, optimizely, featureKey, getCurrentDecision]); + + if (!optimizely) { + hooksLogger.error( + `Unable to properly use feature ${featureKey}. optimizely prop must be supplied via a parent ` + ); + } return [state.isEnabled, state.variables, state.clientReady, state.didTimeout]; }; From 064bb1cafd8e226813af4fb5b5d0f7c84c25de9d Mon Sep 17 00:00:00 2001 From: Md Junaed Hossain <169046794+junaed-optimizely@users.noreply.github.com> Date: Wed, 14 Aug 2024 19:41:51 +0600 Subject: [PATCH 4/6] [FSSDK-10544] useDecision + useTrackEvent update --- src/hooks.spec.tsx | 122 ++++++++++++++++----------------------------- src/hooks.ts | 71 +++++++++++++------------- 2 files changed, 81 insertions(+), 112 deletions(-) diff --git a/src/hooks.spec.tsx b/src/hooks.spec.tsx index 83f2b2d..995fc07 100644 --- a/src/hooks.spec.tsx +++ b/src/hooks.spec.tsx @@ -82,54 +82,40 @@ describe('hooks', () => { let setForcedDecisionMock: jest.Mock; let hooksLoggerErrorSpy: jest.SpyInstance; const REJECTION_REASON = 'A rejection reason you should never see in the test runner'; - const getOnReadyTimeoutPromise = ({ timeout = 0 }: any): Promise => { - const timeoutPromise = new Promise((resolve) => { - setTimeout( - () => { - resolve({ - success: false, - reason: NotReadyReason.TIMEOUT, - dataReadyPromise: new Promise((r) => - setTimeout( - () => - r({ - success: readySuccess, - }), - mockDelay - ) - ), - }); - }, - timeout || mockDelay + 1 - ); - }); - const clientAndUserReadyPromise = new Promise((resolve) => { - setTimeout(() => { - resolve({ - success: readySuccess, - }); - }, mockDelay); - }); - - return Promise.race([clientAndUserReadyPromise, timeoutPromise]); - }; beforeEach(() => { - getOnReadyPromise = ({ timeout = 0 }: any): Promise => - new Promise((resolve) => { - setTimeout(function () { - resolve( - Object.assign( - { - success: readySuccess, - }, - !readySuccess && { - dataReadyPromise: new Promise((r) => setTimeout(r, mockDelay)), - } - ) - ); - }, timeout || mockDelay); + getOnReadyPromise = ({ timeout = 0 }: any): Promise => { + const timeoutPromise = new Promise((resolve) => { + setTimeout( + () => { + resolve({ + success: false, + reason: NotReadyReason.TIMEOUT, + dataReadyPromise: new Promise((r) => + setTimeout( + () => + r({ + success: readySuccess, + }), + mockDelay + ) + ), + }); + }, + timeout || mockDelay + 1 + ); }); + + const clientAndUserReadyPromise = new Promise((resolve) => { + setTimeout(() => { + resolve({ + success: readySuccess, + }); + }, mockDelay); + }); + + return Promise.race([clientAndUserReadyPromise, timeoutPromise]); + }; activateMock = jest.fn(); isFeatureEnabledMock = jest.fn(); featureVariables = mockFeatureVariables; @@ -230,13 +216,13 @@ describe('hooks', () => { activateMock.mockReturnValue(null); mockDelay = 100; readySuccess = false; - // Todo: getOnReadyPromise might be moved in the top later - getOnReadyPromise = getOnReadyTimeoutPromise; + render( ); + expect(screen.getByTestId('result')).toHaveTextContent('null|false|false'); // initial render readySuccess = true; activateMock.mockReturnValue('12345'); @@ -473,7 +459,7 @@ describe('hooks', () => { isFeatureEnabledMock.mockReturnValue(false); featureVariables = {}; readySuccess = false; - getOnReadyPromise = getOnReadyTimeoutPromise; + render( @@ -677,7 +663,7 @@ describe('hooks', () => { }); }); - describe.skip('useDecision', () => { + describe('useDecision', () => { it('should render true when the flag is enabled', async () => { decideMock.mockReturnValue({ ...defaultDecision, @@ -689,8 +675,6 @@ describe('hooks', () => { ); - await optimizelyMock.onReady(); - // component.update(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('true|{"foo":"bar"}|true|false')); }); @@ -700,50 +684,41 @@ describe('hooks', () => { enabled: false, variables: { foo: 'bar' }, }); + render( ); - await optimizelyMock.onReady(); + await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{"foo":"bar"}|true|false')); }); it('should respect the timeout option passed', async () => { decideMock.mockReturnValue({ ...defaultDecision }); readySuccess = false; + mockDelay = 100; render( - + ); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|false')); - await optimizelyMock.onReady(); - // component.update(); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|true')); + // Initial render + expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|false'); - // Simulate datafile fetch completing after timeout has already passed - // flag is now true and decision contains variables decideMock.mockReturnValue({ ...defaultDecision, enabled: true, variables: { foo: 'bar' }, }); - - await optimizelyMock.onReady().then((res) => res.dataReadyPromise); - - // Simulate datafile fetch completing after timeout has already passed - // Wait for completion of dataReadyPromise - await optimizelyMock.onReady().then((res) => res.dataReadyPromise); - - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('true|{"foo":"bar"}|true|true')); // when clientReady + readySuccess = true; + // When timeout is reached, but dataReadyPromise is resolved later with the decision value + await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('true|{"foo":"bar"}|true|true')); }); it('should gracefully handle the client promise rejecting after timeout', async () => { - jest.useFakeTimers(); - readySuccess = false; decideMock.mockReturnValue({ ...defaultDecision }); getOnReadyPromise = (): Promise => @@ -755,11 +730,7 @@ describe('hooks', () => { ); - jest.advanceTimersByTime(mockDelay + 1); - await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|false|false')); - - jest.useRealTimers(); }); it('should re-render when the user attributes change using autoUpdate', async () => { @@ -772,7 +743,6 @@ describe('hooks', () => { // TODO - Wrap this with async act() once we upgrade to React 16.9 // See https://github.com/facebook/react/issues/15379 - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); decideMock.mockReturnValue({ @@ -797,7 +767,6 @@ describe('hooks', () => { // TODO - Wrap this with async act() once we upgrade to React 16.9 // See https://github.com/facebook/react/issues/15379 - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); decideMock.mockReturnValue({ @@ -991,7 +960,6 @@ describe('hooks', () => { ); decideMock.mockReturnValue({ ...defaultDecision, variables: { foo: 'bar' } }); - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{"foo":"bar"}|true|false')); }); @@ -1017,7 +985,6 @@ describe('hooks', () => { { variationKey: 'var2' } ); - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); }); @@ -1045,7 +1012,6 @@ describe('hooks', () => { { variationKey: 'var2' } ); - await optimizelyMock.onReady(); await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); }); }); diff --git a/src/hooks.ts b/src/hooks.ts index e151a56..1001e6a 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -1,4 +1,3 @@ -/* eslint-disable react-hooks/rules-of-hooks */ /** * Copyright 2018-2019, 2022-2024, Optimizely * @@ -14,7 +13,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { useCallback, useContext, useEffect, useState, useRef } from 'react'; + +import { useCallback, useContext, useEffect, useState, useRef, useMemo } from 'react'; import { UserAttributes, OptimizelyDecideOption, getLogger } from '@optimizely/optimizely-sdk'; @@ -449,35 +449,33 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {}) export const useDecision: UseDecision = (flagKey, options = {}, overrides = {}) => { const { optimizely, isServerSide, timeout } = useContext(OptimizelyContext); - if (!optimizely) { - hooksLogger.error( - `Unable to use decision ${flagKey}. optimizely prop must be supplied via a parent ` - ); - return [ + const overrideAttrs = useCompareAttrsMemoize(overrides.overrideAttributes); + + const defaultDecision = useMemo( + () => createFailedDecision(flagKey, 'Optimizely SDK not configured properly yet.', { - id: null, - attributes: {}, + id: overrides.overrideUserId || null, + attributes: overrideAttrs || {}, }), - false, - false, - ]; - } + [flagKey, overrideAttrs, overrides.overrideUserId] + ); - const overrideAttrs = useCompareAttrsMemoize(overrides.overrideAttributes); + const getCurrentDecision: () => { decision: OptimizelyDecision } = useCallback( + () => ({ + decision: + optimizely?.decide(flagKey, options.decideOptions, overrides.overrideUserId, overrideAttrs) || defaultDecision, + }), + [flagKey, defaultDecision, optimizely, options.decideOptions, overrideAttrs, overrides.overrideUserId] + ); - const getCurrentDecision: () => { decision: OptimizelyDecision } = () => ({ - decision: optimizely.decide(flagKey, options.decideOptions, overrides.overrideUserId, overrideAttrs), - }); + const isClientReady = isServerSide || !!optimizely?.isReady(); + const isReadyPromiseFulfilled = !!optimizely?.getIsReadyPromiseFulfilled(); - const isClientReady = isServerSide || optimizely.isReady(); const [state, setState] = useState<{ decision: OptimizelyDecision } & InitializationState>(() => { const decisionState = isClientReady ? getCurrentDecision() : { - decision: createFailedDecision(flagKey, 'Optimizely SDK not configured properly yet.', { - id: overrides.overrideUserId || null, - attributes: overrideAttrs, - }), + decision: defaultDecision, }; return { ...decisionState, @@ -504,13 +502,14 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {}) } const finalReadyTimeout = options.timeout !== undefined ? options.timeout : timeout; + useEffect(() => { // Subscribe to initialzation promise only // 1. When client is using Sdk Key, which means the initialization will be asynchronous // and we need to wait for the promise and update decision. // 2. When client is using datafile only but client is not ready yet which means user // was provided as a promise and we need to subscribe and wait for user to become available. - if (optimizely.getIsUsingSdkKey() || !isClientReady) { + if (optimizely && (optimizely.getIsUsingSdkKey() || !isClientReady)) { subscribeToInitialization(optimizely, finalReadyTimeout, (initState) => { setState({ ...getCurrentDecision(), @@ -518,7 +517,8 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {}) }); }); } - }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [finalReadyTimeout, getCurrentDecision, optimizely]); useEffect(() => { if (overrides.overrideUserId || overrides.overrideAttributes || !options.autoUpdate) { @@ -532,11 +532,11 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {}) ...getCurrentDecision(), })); }); - }, [overrides.overrideUserId, overrides.overrideAttributes, options.autoUpdate]); + }, [overrides.overrideUserId, overrides.overrideAttributes, options.autoUpdate, flagKey, getCurrentDecision]); useEffect(() => { // Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering. - if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) { + if (optimizely && isReadyPromiseFulfilled && options.autoUpdate) { return setupAutoUpdateListeners(optimizely, HookType.FEATURE, flagKey, hooksLogger, () => { setState((prevState) => ({ ...prevState, @@ -545,14 +545,20 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {}) }); } return (): void => {}; - }, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, flagKey, getCurrentDecision]); + }, [isReadyPromiseFulfilled, options.autoUpdate, optimizely, flagKey, getCurrentDecision]); + + if (!optimizely) { + hooksLogger.error( + `Unable to use decision ${flagKey}. optimizely prop must be supplied via a parent ` + ); + } return [state.decision, state.clientReady, state.didTimeout]; }; export const useTrackEvent: UseTrackEvent = () => { const { optimizely, isServerSide, timeout } = useContext(OptimizelyContext); - const isClientReady = !!(isServerSide || optimizely?.isReady()); + const isClientReady = isServerSide || !!optimizely?.isReady(); const track = useCallback( (...rest: Parameters): void => { @@ -569,10 +575,6 @@ export const useTrackEvent: UseTrackEvent = () => { [optimizely, isClientReady] ); - if (!optimizely) { - return [track, false, false]; - } - const [state, setState] = useState<{ clientReady: boolean; didTimeout: DidTimeout; @@ -589,12 +591,13 @@ export const useTrackEvent: UseTrackEvent = () => { // and we need to wait for the promise and update decision. // 2. When client is using datafile only but client is not ready yet which means user // was provided as a promise and we need to subscribe and wait for user to become available. - if (optimizely.getIsUsingSdkKey() || !isClientReady) { + if (optimizely && (optimizely.getIsUsingSdkKey() || !isClientReady)) { subscribeToInitialization(optimizely, timeout, (initState) => { setState(initState); }); } - }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [optimizely, timeout]); return [track, state.clientReady, state.didTimeout]; }; From f87cbf4336df63e85ed72f7c0d21484109c5c56e Mon Sep 17 00:00:00 2001 From: Md Junaed Hossain <169046794+junaed-optimizely@users.noreply.github.com> Date: Thu, 15 Aug 2024 17:39:04 +0600 Subject: [PATCH 5/6] [FSSDK-10544] hook log improvement --- src/hooks.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/hooks.ts b/src/hooks.ts index 1001e6a..a84b266 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -25,6 +25,7 @@ import { OptimizelyContext } from './Context'; import { areAttributesEqual, OptimizelyDecision, createFailedDecision } from './utils'; export const hooksLogger = getLogger('ReactSDK'); +const optimizelyPropError = "The 'optimizely' prop must be supplied via a parent "; enum HookType { EXPERIMENT = 'Experiment', @@ -340,9 +341,7 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri ); if (!optimizely) { - hooksLogger.error( - `Unable to use experiment ${experimentKey}. optimizely prop must be supplied via a parent ` - ); + hooksLogger.error(`Unable to use experiment ${experimentKey}. ${optimizelyPropError}`); } return [state.variation, state.clientReady, state.didTimeout]; @@ -431,9 +430,7 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {}) }, [isReadyPromiseFulfilled, options.autoUpdate, optimizely, featureKey, getCurrentDecision]); if (!optimizely) { - hooksLogger.error( - `Unable to properly use feature ${featureKey}. optimizely prop must be supplied via a parent ` - ); + hooksLogger.error(`Unable to properly use feature ${featureKey}. ${optimizelyPropError}`); } return [state.isEnabled, state.variables, state.clientReady, state.didTimeout]; @@ -548,9 +545,7 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {}) }, [isReadyPromiseFulfilled, options.autoUpdate, optimizely, flagKey, getCurrentDecision]); if (!optimizely) { - hooksLogger.error( - `Unable to use decision ${flagKey}. optimizely prop must be supplied via a parent ` - ); + hooksLogger.error(`Unable to use decision ${flagKey}. ${optimizelyPropError}`); } return [state.decision, state.clientReady, state.didTimeout]; @@ -563,7 +558,7 @@ export const useTrackEvent: UseTrackEvent = () => { const track = useCallback( (...rest: Parameters): void => { if (!optimizely) { - hooksLogger.error(`Unable to track events. optimizely prop must be supplied via a parent `); + hooksLogger.error(`Unable to track events. ${optimizelyPropError}`); return; } if (!isClientReady) { From e1356180a06b9806b623c239f75ab19d73fd1e76 Mon Sep 17 00:00:00 2001 From: Md Junaed Hossain <169046794+junaed-optimizely@users.noreply.github.com> Date: Thu, 15 Aug 2024 17:45:10 +0600 Subject: [PATCH 6/6] [FSSDK-10544] test comments cleanup --- src/hooks.spec.tsx | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/src/hooks.spec.tsx b/src/hooks.spec.tsx index 995fc07..ff150d1 100644 --- a/src/hooks.spec.tsx +++ b/src/hooks.spec.tsx @@ -71,7 +71,6 @@ describe('hooks', () => { let notificationListenerCallbacks: Array<() => void>; let optimizelyMock: ReactSDKClient; let readySuccess: boolean; - // let reason: NotReadyReason; let userUpdateCallbacks: Array<() => void>; let UseExperimentLoggingComponent: React.FunctionComponent; let UseFeatureLoggingComponent: React.FunctionComponent; @@ -214,7 +213,7 @@ describe('hooks', () => { it('should respect the timeout option passed', async () => { activateMock.mockReturnValue(null); - mockDelay = 100; + mockDelay = 20; readySuccess = false; render( @@ -255,8 +254,6 @@ describe('hooks', () => { ); - // TODO - Wrap this with async act() once we upgrade to React 16.9 - // See https://github.com/facebook/react/issues/15379 await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|true|false')); activateMock.mockReturnValue('12345'); @@ -276,8 +273,6 @@ describe('hooks', () => { ); - // // TODO - Wrap this with async act() once we upgrade to React 16.9 - // // See https://github.com/facebook/react/issues/15379 await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('null|true|false')); activateMock.mockReturnValue('12345'); @@ -455,7 +450,7 @@ describe('hooks', () => { }); it('should respect the timeout option passed', async () => { - mockDelay = 100; + mockDelay = 20; isFeatureEnabledMock.mockReturnValue(false); featureVariables = {}; readySuccess = false; @@ -502,8 +497,6 @@ describe('hooks', () => { ); - // TODO - Wrap this with async act() once we upgrade to React 16.9 - // See https://github.com/facebook/react/issues/15379 await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); isFeatureEnabledMock.mockReturnValue(true); @@ -525,8 +518,6 @@ describe('hooks', () => { ); - // TODO - Wrap this with async act() once we upgrade to React 16.9 - // See https://github.com/facebook/react/issues/15379 await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); isFeatureEnabledMock.mockReturnValue(true); @@ -535,7 +526,7 @@ describe('hooks', () => { act(() => { userUpdateCallbacks.forEach((fn) => fn()); }); - // component.update(); + await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); }); @@ -697,7 +688,7 @@ describe('hooks', () => { it('should respect the timeout option passed', async () => { decideMock.mockReturnValue({ ...defaultDecision }); readySuccess = false; - mockDelay = 100; + mockDelay = 20; render( @@ -741,8 +732,6 @@ describe('hooks', () => { ); - // TODO - Wrap this with async act() once we upgrade to React 16.9 - // See https://github.com/facebook/react/issues/15379 await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); decideMock.mockReturnValue({ @@ -765,8 +754,6 @@ describe('hooks', () => { ); - // TODO - Wrap this with async act() once we upgrade to React 16.9 - // See https://github.com/facebook/react/issues/15379 await waitFor(() => expect(screen.getByTestId('result')).toHaveTextContent('false|{}|true|false')); decideMock.mockReturnValue({ @@ -789,7 +776,7 @@ describe('hooks', () => { ); - // component.update(); + expect(mockLog).toHaveBeenCalledTimes(1); expect(mockLog).toHaveBeenCalledWith(false); });