Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression leads to missing autoUpdate hook updates. #197

Open
epatey opened this issue Apr 19, 2023 · 1 comment
Open

Regression leads to missing autoUpdate hook updates. #197

epatey opened this issue Apr 19, 2023 · 1 comment

Comments

@epatey
Copy link

epatey commented Apr 19, 2023

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants