fix(hooks): Return latest decision values on first call, and re-render in response to decision input changes #64

mjc1283 · 2020-08-20T00:18:09Z

Summary

This PR addresses two issues with the useExperiment and useFeature hooks:

Changes to the overrides argument do not trigger a re-render
Even after the client instance is ready, the first call returns false (for feature enabled status from useFeature) or null (for variation assignment from useExperiment).

A boolean-returning isReady method is added to ReactSDKClient. When isReady returns true, useFeature and useExperiment don't wait for the fulfillment of the onReady promise before calling their respective decision methods.

Arguments that affect decision results (experiment/feature key + overrides) are tracked in state, so we can compare prior and current values. When values change, the decision is recomputed. I followed the pattern described here, treating the decision state as being derived from the arguments.

The effect that subscribes to updates is also changed to depend on the decision inputs. Since override user attributes are an object, I used a ref combined with a custom equality function to ensure that the dependency array receives a new object reference only when the contents of the attributes are different.

Test Plan

Manually tested using an example React app. Checked useFeature and useExperiment return values both before & after client became ready. Checked that useFeature and useExperiment trigger re-renders when entity key or override arguments change.
Unit tests: updated, and added more

mikeproeng37

At I high level, this makes sense. I did an initial pass and left some feedback. Will attempt a second, lengthier pass tomorrow.

mikeproeng37 · 2020-08-21T04:27:08Z

src/hooks.ts

+ * @param {UserAttributes|undefined} newAttrs
+ * @returns boolean
+ */
+function areAttributesEqual(oldAttrs: UserAttributes | undefined, newAttrs: UserAttributes | undefined): boolean {


This feels like it should go in our util/fns.

Agree, although I don't think it'd be used anywhere else.

mikeproeng37 · 2020-08-21T04:29:42Z

src/hooks.ts

+  initialDecision: DecisionType,
+  options: HookOptions = {},
+  overrides: HookOverrides = {}
+): DecisionType {


This return type doesn't match the docstring

mikeproeng37 · 2020-08-21T05:57:41Z

src/hooks.ts

+  const { isServerSide } = useContext(OptimizelyContext);
+  const isClientReady = isServerSide || optimizely.isReady();
+
+  const decisionStateAndSetter = useState<DecisionType>(() => {


Any reason not to destruct this as const [decisionState, setDecisionState] = useState...?

decisionState has to be declared with let, it may be assigned to below.

mikeproeng37 · 2020-08-21T05:59:48Z

src/hooks.ts

+      });
+    }
+    return (): void => {};
+  }, [optimizely, entityKey, overrides.overrideUserId, overrideAttrsRef.current]);


Can we just subscribe to isClientReady instead of the entire optimizely object? Not sure if that will make much of a diff in terms of perf.

If optimizely actually changes, it's a whole new client instance. In that case the effect definitely should rerun. But I think this would be a very rare case - usually it is set once on the provider and never changed.

mikeproeng37 · 2020-08-21T06:00:40Z

src/hooks.ts

      });
+  }, [optimizely, finalReadyTimeout]);


Same here about subscribing to optimizely

Same answer, that effect needs to rerun if we got a new optimizely.

approach for detecting changes in user attributes. Add more tests.

is true

circAssimilate

Hey, @mjc1283, thanks for pinging me about this. Have a few questions on your approach. We could pair on this review and try and knock it out without a lot of back and forth if you prefer. If so, just find some time on my calendar and ping me!

circAssimilate · 2020-08-24T15:13:35Z

src/hooks.ts

  });
+  let decisionState = decisionStateAndSetter[0];


Is it safe to mutate the state value from useState?

Not sure, I will try removing this.

circAssimilate · 2020-08-24T15:16:19Z

src/hooks.ts

+        setDecisionState(getCurrentDecision());
+      });
+    }
+    return (): void => {};


Will we have a memory leak issue on unmount? Do we need to remove auto update listeners here?

I think it's ok because if control reaches this line, no auto update listeners were added.

Oh, rightttt, my misread on this. That's clear to me now.

circAssimilate · 2020-08-24T15:23:15Z

src/hooks.ts

+  const [prevDecisionInputs, setPrevDecisionInputs] = useState<DecisionInputs>(currentDecisionInputs);
+  if (!areDecisionInputsEqual(prevDecisionInputs, currentDecisionInputs)) {
+    setPrevDecisionInputs(currentDecisionInputs);
+    decisionState = getCurrentDecision();


This feels like it's unsafe - like it would update the variable but React wouldn't necessarily know. I'm guessing you're doing it to avoid a re-render? Can you point to any docs that talk about this approach? I'm seeing https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops but it doesn't look like anything strange is happening other than a conditional state update which is expected to cause one more render. Doesn't this automatically happen on the next line when setDecisionState() is called?

I can try taking this out. I just figured since I am updating decision state, I should return the latest value that I'm updating it to. But yea, it's not done that way in the example - even though they call setIsScrollingdown, they don't assign that new value to isScrollingDown before using it.

circAssimilate · 2020-08-24T15:28:07Z

src/hooks.ts

+  if (!areDecisionInputsEqual(prevDecisionInputs, currentDecisionInputs)) {
+    setPrevDecisionInputs(currentDecisionInputs);
+    decisionState = getCurrentDecision();
+    setDecisionState(decisionState);


I might be missing something obvious but I can't see what setDecisionState does with decisionState. It looks like a function without any params.

setDecisionState is a function, it's the 2nd argument returned from useState on line 219. I am trying to update the state value here.

circAssimilate · 2020-08-24T15:30:38Z

src/hooks.ts

+        setDecisionState(getCurrentDecision());
+      });
+    }
+    return (): void => {};


Same question as below about the need to "unlisten" on unmount.

…since the last time

circAssimilate · 2020-08-24T21:46:37Z

src/hooks.ts

+  const [decisionState, setDecisionState] = useState<FeatureDecisionValues>(() => {
+    if (isClientReady) {
+      return getCurrentDecision();
    }
-    return initialState;
+    return { isEnabled: false, variables: {} };


@mjc1283, I am guessing there is something about this implementation of useState that I don’t understand. Can you help me understand what the anon function does here - does it only set initial state or is it used on every setDecisionState. I look at the React docs but couldn't find a clear example. The one I found seemed to insinuate that the provided function would be called every time.

@mjc1283 relayed this to me in Slack:

I think the answer is: when you pass a function to useState, it is just called once the first time. It’s like an initializer. It returns the initial value of the state. As described here under “Lazy Initial State”

circAssimilate

Thanks for the responses and for fixing this, Matt! All looks good to me then if the general functionality is tested and all that. Will we release a minor or patch version for this?

mjc1283

Thanks for reviewing @circAssimilate . I am thinking this'll be a patch version.

Matt Carroll added 11 commits August 18, 2020 21:30

WIP: issue 58

c020cbc

More WIP

d897288

More WIP

a4ccea8

Make it generic

4bec1e2

Further cleanup

93f73c5

Comment cleaning

91efea2

Update attr equality test

4324bd2

Remove scratch files

e708049

Undo unrelated changes

fda91cd

Remove more unrelated changes

de574c6

Separate client initialization state into its own hook

df820d3

mjc1283 added the wip label Aug 20, 2020

mjc1283 self-assigned this Aug 20, 2020

Matt Carroll added 3 commits August 19, 2020 17:19

Fix useDecision comment

d08d1b9

Fix existing hooks tests

32c06ae

New tests for useExperiment

0b8b65c

mikeproeng37 reviewed Aug 21, 2020

View reviewed changes

Matt Carroll added 5 commits August 21, 2020 15:46

Add official hooks lint plugin. Fix lint errors. Use a different

1214ceb

approach for detecting changes in user attributes. Add more tests.

Make attributes comparison safer

8e7386d

Fix initialization state - isReady is immediately true when isServerSide

ea029c4

is true

Fix Experiment component tests

1596603

Fix Feature component tests

acdf9f1

mjc1283 removed the wip label Aug 21, 2020

mjc1283 marked this pull request as ready for review August 21, 2020 23:55

mjc1283 removed their assignment Aug 22, 2020

optimizely-codereview-bot requested a review from mikeproeng37 August 22, 2020 00:00

circAssimilate suggested changes Aug 24, 2020

View reviewed changes

Matt Carroll added 2 commits August 24, 2020 09:34

Don't assign new value to decisionState when decision inputs changed …

5d6bd5e

…since the last time

Move attributes equality fn to utils

3a4c594

mjc1283 requested a review from circAssimilate August 24, 2020 17:49

circAssimilate reviewed Aug 24, 2020

View reviewed changes

circAssimilate approved these changes Aug 24, 2020

View reviewed changes

mjc1283 commented Aug 24, 2020

View reviewed changes

mjc1283 merged commit b387abb into master Aug 26, 2020

mjc1283 deleted the mcarroll/issue-58 branch August 26, 2020 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hooks): Return latest decision values on first call, and re-render in response to decision input changes #64

fix(hooks): Return latest decision values on first call, and re-render in response to decision input changes #64

mjc1283 commented Aug 20, 2020 •

edited

Loading

mikeproeng37 left a comment

mikeproeng37 Aug 21, 2020

mjc1283 Aug 21, 2020

mikeproeng37 Aug 21, 2020

mikeproeng37 Aug 21, 2020

mjc1283 Aug 21, 2020

mikeproeng37 Aug 21, 2020

mjc1283 Aug 21, 2020

mikeproeng37 Aug 21, 2020

mjc1283 Aug 21, 2020

circAssimilate left a comment

circAssimilate Aug 24, 2020

mjc1283 Aug 24, 2020

circAssimilate Aug 24, 2020

mjc1283 Aug 24, 2020

circAssimilate Aug 24, 2020

circAssimilate Aug 24, 2020

mjc1283 Aug 24, 2020

circAssimilate Aug 24, 2020

mjc1283 Aug 24, 2020

circAssimilate Aug 24, 2020

circAssimilate Aug 24, 2020

circAssimilate Aug 24, 2020

circAssimilate left a comment

mjc1283 left a comment

fix(hooks): Return latest decision values on first call, and re-render in response to decision input changes #64

fix(hooks): Return latest decision values on first call, and re-render in response to decision input changes #64

Conversation

mjc1283 commented Aug 20, 2020 • edited Loading

Summary

Test Plan

mikeproeng37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

circAssimilate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

circAssimilate left a comment

Choose a reason for hiding this comment

mjc1283 left a comment

Choose a reason for hiding this comment

mjc1283 commented Aug 20, 2020 •

edited

Loading