optimizely / react-sdk

React SDK for Optimizely Feature Experimentation and Optimizely Full Stack (legacy)
https://docs.developers.optimizely.com/experimentation/v4.0.0-full-stack/docs/javascript-react-sdk
Apache License 2.0
89 stars 36 forks source link

Regression leads to missing `autoUpdate` hook updates. #197

Open epatey opened 1 year ago

epatey commented 1 year ago

There is a timing bug in all three hooks' (useFeature, useExperiment, and useDecision) that leads to autoUpdate: true render triggers getting lost.

The code in #125 introduced a window of time between the mounting of the hook and the registering a listener for config or user changes. If the user (or presumably the config) is changed in that window of time, an update will not be triggered.

This pattern is used in all of the hooks.

  useEffect(() => {
    // Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering.
    if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) {
      return setupAutoUpdateListeners(optimizely, HookType.FEATURE, featureKey, hooksLogger, () => {
        setState(prevState => ({
          ...prevState,
          ...getCurrentDecision(),
        }));
      });
    }
    return (): void => { };
  }, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, featureKey, getCurrentDecision]);

If the user is changed too soon, the listener will still be set up, but it will be too late. The change has already occurred and the decision has not be reassessed.

I would propose that, immediately before setting up the listener, the code must determine if the user or config has changed since the hook was mounted. If so, it should execute the setState code prior to setting up the listener. For example:

  useEffect(() => {
    // Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering.
    if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) {
      if (user or config have changed since mounting) {
        setState(prevState => ({
          ...prevState,
          ...getCurrentDecision(),
        }));
      }
      return setupAutoUpdateListeners(optimizely, HookType.FEATURE, featureKey, hooksLogger, () => {
      // remainder is the same as before
mikechu-optimizely commented 1 year ago

@epatey Thanks for the detailed filing. I've created a ticket (FSSDK-9625) to review and implement a solution.

junaed-optimizely commented 1 week ago

@epatey, there have been some updates in #273 that addresses some of the rendering issues. Could you please update to the latest version and recheck if this issue still persists!

Thanks!