fix(hooks): Always recompute decision after resolution of ready promise #91
Conversation
src/hooks.ts
Outdated
| }); | ||
| }); | ||
| } | ||
| }, [isClientReady, finalReadyTimeout, setState, getCurrentDecision, optimizely]); |
There was a problem hiding this comment.
Are we adding setState here because it was throwing a exhaustive dependency lint warning otherwise? If that function reference changes wondering if we could be over-eagerly running this effect.
There was a problem hiding this comment.
Good point. The lint rule seems to allow excluding it. I will remove it.
| setState(prevState => ({ | ||
| ...prevState, | ||
| ...getCurrentDecision(), | ||
| })); | ||
| }); | ||
| } | ||
| return (): void => {}; |
There was a problem hiding this comment.
more of a question on the previously existing functionality - does this do anything helpful or could it be left off? No need to change that here, jw. My understanding is it's optional and isn't helpful here.
There was a problem hiding this comment.
Ohh, I see - it's because we're returning the cancelation up above so probably have to always supply it.
There was a problem hiding this comment.
Thanks @circAssimilate, yea let's do some testing. I'll DM you.
src/hooks.ts
Outdated
| }); | ||
| }); | ||
| } | ||
| }, [isClientReady, finalReadyTimeout, setState, getCurrentDecision, optimizely]); |
There was a problem hiding this comment.
Good point. The lint rule seems to allow excluding it. I will remove it.
|
I was able to manually bash on this and things are looking good from my vantage point. Here's the old SDK: https://codesandbox.io/s/priceless-flower-2ip8d?file=/src/App.js And here's the new one: https://codesandbox.io/s/falling-flower-5yuwm?file=/src/App.js Everything seems to be working better in the new. |
Summary
Fix the following bug: when client is initialized with datafile, and no SDK key, OptimizelyExperiment and OptimizelyFeature components don't properly render their decisions.
The issue is in the implementation in hooks and the interaction with the client's ready state (via
isReady/onReady). When initialized with datafile, the client begins in a non-ready state, but immediately becomes ready on the next tick. But the hook does not recompute the decision when this occurs - it's only recomputing the decision when aCONFIG_UPDATEis fired.CONFIG_UPDATEis never fired when the instance was initialized with datafile only.To fix, add an effect in the
useFeature/useExperimenthooks that recomputes the decision after the client becomes ready.Test plan