Skip to content

Commit

Permalink
Fix useTrackerNoDeps for React 18
Browse files Browse the repository at this point in the history 
Also re-adds an attempt to retain the "side-effect"
we are creating during first render, when we can,
to both versions of the hook
  • Loading branch information
@CaptainN
CaptainN committed Apr 29, 2022
1 parent b8eb187 commit 384da2f
Showing 1 changed file with 46 additions and 37 deletions.
83 changes: 46 additions & 37 deletions packages/react-meteor-data/useTracker.ts
View file
Expand Up @@ -66,48 +66,54 @@ const useTrackerNoDeps = <T = any>(reactiveFn: IReactiveFn<T>, skipUpdate: ISkip
// it stops the inner one.
Tracker.nonreactive(() => Tracker.autorun((c: Tracker.Computation) => {
refs.computation = c;
const data = reactiveFn(c);
if (c.firstRun) {
// Always run the reactiveFn on firstRun
refs.trackerData = reactiveFn(c);
} else if (!skipUpdate || !skipUpdate(refs.trackerData, reactiveFn(c))) {
refs.trackerData = data;
} else if (!skipUpdate || !skipUpdate(refs.trackerData, data)) {
// For any reactive change, forceUpdate and let the next render rebuild the computation.
forceUpdate();
}
}));

// To avoid creating side effects in render with Tracker when not using deps
// create the computation, run the user's reactive function in a computation synchronously,
// then immediately dispose of it. It'll be recreated again after the render is committed.
// To clean up side effects in render, stop the computation immediately
if (!refs.isMounted) {
// We want to forceUpdate in useEffect to support StrictMode.
// See: https://github.com/meteor/react-packages/issues/278
if (refs.computation) {
refs.computation.stop();
delete refs.computation;
}
Meteor.defer(() => {
if (!refs.isMounted && refs.computation) {
refs.computation.stop();

This comment has been minimized.

Sign in to view

Copy link
@make-github-pseudonymous-again

make-github-pseudonymous-again Feb 22, 2023 •
edited

Shouldn't this only be called on the computation created just above? Could refs.computation potentially be something else than the computation created just above?

This comment has been minimized.

Sign in to view

Copy link
@Grubba27

Grubba27 Feb 23, 2023

Contributor

Are you having an issue with this refs.computation.stop ?
We had discussed that it might be an issue.
If you are having with this, we will need to adjust( make a const and stop this const in the defer)

This comment has been minimized.

Sign in to view

Copy link
@make-github-pseudonymous-again

make-github-pseudonymous-again Feb 23, 2023

I think it might be a source of bugs. The reffed computation is already stopped at the top of the function. The goal of this conditional is to clean up the side effect of this particular render, so it should only apply to the computation created during that render, and nothing else.

This comment has been minimized.

Sign in to view

Copy link
@CaptainN

CaptainN Feb 25, 2023

Author Collaborator

I remember having various issues with trying to stop the initial computation in the same time slice as where it is first run. The strange thing is, this works just fine, and always did, in the NoDeps implementation. It just gave me a lot of trouble in the deps implementation. That's the reason the Meteor.defer is there. Meteor.defer runs very very soon after the current execution time slice is complete, and always before useEffect (at least, on modern browsers, can't speak to IE or older Safari).

It's possible that it's not a problem any more with React 18, I haven't tested it. If not, we should not bother trying to stop the computation in defer. In this case, there's also no need even to store the initial computation in refs, just use a local var, run the firstRun, then stop it immediately after. Tracker.autorun should run the computation inline so that should be safe to run right away (and like I said, it currently works this way in noDeps).

This comment has been minimized.

Sign in to view

Copy link
@CaptainN

CaptainN Feb 25, 2023

Author Collaborator

Also worth noting, this should ONLY run during the very first render, before mount. That means a couple of things:

  • When deps change, this will never re-run.
  • This will actually run a few times in concurrent mode, and most of the results will be tossed away. If a user implements a side-effect in their own tracker function, that can cause unforeseen, and difficult to diagnose problems.
  • When deps do change, we still need to make sure the new data is available immediately, and not delayed by one render - that is one of the tricky aspects of writing this hook.

This comment has been minimized.

Sign in to view

Copy link
@make-github-pseudonymous-again

make-github-pseudonymous-again Feb 25, 2023

@CaptainN Not sure if we are talking about the same thing. I am suggesting keeping the defer but referring only to the computation instantiated above. For instance:

  const localComputation = Tracker.nonreactive(() => Tracker.autorun((c: Tracker.Computation) => {
    refs.computation = c;
    const data = reactiveFn(c);
    if (c.firstRun) {
      // Always run the reactiveFn on firstRun
      refs.trackerData = data;
    } else if (!skipUpdate || !skipUpdate(refs.trackerData, data)) {
      // For any reactive change, forceUpdate and let the next render rebuild the computation.
      forceUpdate();
    }
  }));

  // To clean up side effects in render, stop the computation immediately
  if (!refs.isMounted) {
    Meteor.defer(() => {
      if (!refs.isMounted) {
        localComputation.stop();
...

This comment has been minimized.

Sign in to view

Copy link
@CaptainN

CaptainN Feb 27, 2023 •
edited

Author Collaborator

I'm not sure what's going on to cause these problems, but I'll say that the current implementation is muddled. Here it is (from master):

  useMemo(() => {
    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
        const data = refs.reactiveFn();
        if (c.firstRun) {
          refs.data = data;
        } else if (!skipUpdate || !skipUpdate(refs.data, data)) {
          refs.data = data;
          forceUpdate();
        }
      })
    );

    // Stop the computation immediately to avoid creating side effects in render.
    // refers to this issues:
    // https://github.com/meteor/react-packages/issues/382
    // https://github.com/meteor/react-packages/issues/381
    if (refs.comp) refs.comp.stop();

    // In some cases, the useEffect hook will run before Meteor.defer, such as
    // when React.lazy is used. This will allow it to be stopped earlier in
    // useEffect if needed.
    refs.comp = comp;
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => {
      if (!refs.isMounted && refs.comp) {
        refs.comp.stop();
        delete refs.comp;
      }
    });
  }, deps);

(Note: BUG: I just spotted a bug in the above - the intent seems (I think?) to be to clean up the current computation, but it's actually lagging behind - that line if (refs.comp) refs.comp.stop(); will always clean up the previous computation (the one still stored in refs), because the current one is set a couple of lines after this invocation. The comments seem to suggest that it's trying to clean up the current computation right away, not trying to clean up an artifact from another time slice - that should not be necessary, but it wouldn't hurt to have a check there (maybe it's possible the useMemo runs again after deps change, but before useEffect takes over? I don't really know for sure.)

If we do intend to stop the computation immediately, and if we determine that it is safe to do so, then we don't need the defer block at all in this case. The computation is already stopped before the defer block is created (or it seems to be trying to stop it anyway). There's no reason to keep the defer in this case. I will say though, that I recall having trouble getting things to work correctly when stopping it in the same time slice like this. It needs to be tested.

If we find that computations CAN be stopped immediately (in the same timeslice) then it should look more like this:

  useMemo(() => {
    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
        const data = refs.reactiveFn();
        if (c.firstRun) {
          refs.data = data;
        } else if (!skipUpdate || !skipUpdate(refs.data, data)) {
          refs.data = data;
          forceUpdate();
        }
      })
    );

    // Stop the computation immediately to avoid creating side effects in render.
    // refers to this issues:
    // https://github.com/meteor/react-packages/issues/382
    // https://github.com/meteor/react-packages/issues/381
    comp.stop();
  }, deps);

Another Note: I don't know if I left it like this, but there used to be a bit of precaution in place to make sure that in the rare case when a useEffect hooks runs before Meteor.defer, the computation would get cleaned up in useEffect (there's a note about that happening when React.lazy is used). This code seems to have been removed. It's important to put that back inside of useEffect. There should just be a check to make sure if there is an existing computation on refs.comp, that it is stopped there. This being missing could definitely cause problems and memory leaks.

This comment has been minimized.

Sign in to view

Copy link
@make-github-pseudonymous-again

make-github-pseudonymous-again Feb 27, 2023

Ah I hadn't seen that one. It's another variation on the same theme. Anyway, if you look at the NoDeps implementation you should find a similar problem where the Meteor.defer call indirectly refers to the local scope computation (if everything goes according to plan and nothing runs before defer...), instead of just directly referring to the local scope computation.

This comment has been minimized.

Sign in to view

Copy link
@CaptainN

CaptainN Feb 27, 2023 •
edited

Author Collaborator

Oh! Garsh this code is tricky...

I changed things to try and keep the original computation alive in some cases, which is why it no longer cleans up the old computation in useEffect. Basically, if the useEffect beats Meteor.defer to the punch, it should not clean up the computation. I wonder if this inadvertently created a race condition.

Maybe it's worth removing that, and going back to the old way - but that would add the double render that everyone complains about. Tricky tricky.

BTW, if we determine that we CANNOT stop computation immediately (which I suspect is the case), and we do want the additional check, useMemo block would look like this:

  useMemo(() => {
    // Stop a potential previous computation to avoid leaks:
    if (refs.comp) {
      // I have not verified that this is even possible
      refs.comp.stop();
      delete refs.comp;
    }

    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
        const data = refs.reactiveFn();
        if (c.firstRun) {
          refs.data = data;
        } else if (!skipUpdate || !skipUpdate(refs.data, data)) {
          refs.data = data;
          forceUpdate();
        }
      })
    );

    // In some cases, the useEffect hook will run before Meteor.defer, such as
    // when React.lazy is used. This will allow it to be stopped earlier in
    // useEffect if needed.
    refs.comp = comp;
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => {
      if (!refs.isMounted && refs.comp) {
        refs.comp.stop();
        delete refs.comp;
      }
    });
  }, deps);

This comment has been minimized.

Sign in to view

Copy link
@CaptainN

CaptainN Feb 27, 2023

Author Collaborator

Okay, one more note - are we seeing errors of the type "Cannot set state on unmounted component"?

If so, I know why that is, and can fix it! But it's a bit tricky, if we want to keep the initial computation alive (and reduce instances of double render on mount).

Essentially, I wrote the computation handler that runs from within useMemo, to be able to forceUpdate in case it's recycled. But if an update comes through the computation before the component is "mounted" (really, before the render is committed, in modern terms), then the forceUpdate call may not work, and may throw that error. Please try this (I'm sorry, I'm not set up for testing right now):

const useTrackerWithDeps = <T = any>(reactiveFn: IReactiveFn<T>, deps: DependencyList, skipUpdate: ISkipUpdate<T> = null): T => {
  const forceUpdate = useForceUpdate();

  const { current: refs } = useRef<{
    reactiveFn: IReactiveFn<T>;
    data?: T;
    comp?: Tracker.Computation;
    isMounted?: boolean;
    forceUpdateOnMount?: boolean;
  }>({ reactiveFn });

  // keep reactiveFn ref fresh
  refs.reactiveFn = reactiveFn;

  useMemo(() => {
    // Stop a potential previous computation to avoid leaks:
    if (refs.comp) {
      // I have not verified that this is even possible, but this should be safe
      refs.comp.stop();
      delete refs.comp;
    }

    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
        const data = refs.reactiveFn();
        if (c.firstRun) {
          refs.data = data;
        } else if (!skipUpdate || !skipUpdate(refs.data, data)) {
          refs.data = data;
          if (!refs.isMounted) {
            // If we are not mounted, we will forceUpdate on mount.
            refs.forceUpdateOnMount = true;
          } else {
            forceUpdate();
          }
        }
      })
    );

    // In some cases, the useEffect hook will run before Meteor.defer, such as
    // when React.lazy is used. This will allow it to be stopped earlier in
    // useEffect if needed.
    refs.comp = comp;
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => {
      if (!refs.isMounted && refs.comp) {
        refs.comp.stop();
        delete refs.comp;
      }
    });
  }, deps);

  useEffect(() => {
    // Let subsequent renders know we are mounted (render is committed).
    refs.isMounted = true;

    // In some cases, the useEffect hook will run before Meteor.defer,
    // in this case, we can recycle the computation, and avoid a double render.
    if (!refs.comp) {
      refs.comp = Tracker.nonreactive(
        () => Tracker.autorun((c) => {
          const data: T = refs.reactiveFn(c);
          if (!skipUpdate || !skipUpdate(refs.data, data)) {
            refs.data = data;
            forceUpdate();
          }
        })
      );
    } else if (refs.forceUpdateOnMount) {
      forceUpdate();
    }

    delete refs.forceUpdateOnMount;

    return () => {
      refs.comp.stop();
      delete refs.comp;
      refs.isMounted = false;
    };
  }, deps);

  return refs.data as T;
};

This comment has been minimized.

Sign in to view

Copy link
@CaptainN

CaptainN Feb 27, 2023 •
edited

Author Collaborator

Okay, last one (probably). This is probably a safer way to do the above. Basically, it just always destroys the current computation if there is an update while the component is not mounted (I also added another isMounted check inside of the useEffect handler, for defensive coding). This is safer, but it will mean that we are not recycling the computation on mount in those cases - that shouldn't be a problem in like 99% of cases. This is probably the best mix of safety with trying to recycle the initial computation (though most of the time that's still going to fail).

const useTrackerWithDeps = <T = any>(
  reactiveFn: IReactiveFn<T>,
  deps: DependencyList,
  skipUpdate: ISkipUpdate<T> | null = null,
): T => {
  const forceUpdate = useForceUpdate();

  const { current: refs } = useRef<{
    reactiveFn: IReactiveFn<T>;
    data?: T;
    comp?: Tracker.Computation;
    isMounted?: boolean;
  }>({ reactiveFn });

  // keep reactiveFn ref fresh
  refs.reactiveFn = reactiveFn;

  useMemo(() => {
    // Stop a potential previous computation to avoid leaks:
    if (refs.comp) {
      // I have not verified that this is even possible, but this should be safe
      refs.comp.stop();
      delete refs.comp;
    }

    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(() =>
      Tracker.autorun((c: Tracker.Computation) => {
        const data = refs.reactiveFn();
        if (c.firstRun) {
          refs.data = data;
          return;
        }

        // If not mounted (and not firstRun), delete the computation and let
        // useEffect recreate it on mount.
        if (!refs.isMounted) {
          refs.comp?.stop();
          delete refs.comp;
          return;
        }

        if (!skipUpdate || !skipUpdate(refs.data, data)) {
          refs.data = data;
          forceUpdate();
        }
      })
    );

    // In some cases, the useEffect hook will run before Meteor.defer, such as
    // when React.lazy is used. This will allow it to be stopped earlier in
    // useEffect if needed.
    refs.comp = comp;
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => {
      if (!refs.isMounted && refs.comp) {
        refs.comp.stop();
        delete refs.comp;
      }
    });
  }, deps);

  useEffect(() => {
    // Let subsequent renders know we are mounted (render is committed).
    refs.isMounted = true;

    // In some cases, the useEffect hook will run before Meteor.defer,
    // in this case, we can recycle the computation, and avoid a double render.
    if (!refs.comp) {
      refs.comp = Tracker.nonreactive(() =>
        Tracker.autorun((c) => {
          // If not mounted, stop and delete the computation.
          if (!refs.isMounted) {
            refs.comp?.stop();
            delete refs.comp;
          }
          const data: T = refs.reactiveFn(c);
          if (!skipUpdate || !skipUpdate(refs.data, data)) {
            refs.data = data;
            forceUpdate();
          }
        })
      );
    }

    return () => {
      refs.comp.stop();
      delete refs.comp;
      refs.isMounted = false;
    };
  }, deps);

  return refs.data as T;
};
delete refs.computation;
}
});
}

useEffect(() => {
// Let subsequent renders know we are mounted (render is committed).
refs.isMounted = true;

// Render is committed. Since useTracker without deps always runs synchronously,
// forceUpdate and let the next render recreate the computation.
if (!skipUpdate) {
forceUpdate();
} else {
Tracker.nonreactive(() => Tracker.autorun((c: Tracker.Computation) => {
refs.computation = c;
if (!skipUpdate(refs.trackerData, reactiveFn(c))) {
// For any reactive change, forceUpdate and let the next render rebuild the computation.
forceUpdate();
}
}));
// In some cases, the useEffect hook will run before Meteor.defer, such as
// when React.lazy is used. In those cases, we might as well leave the
// computation alone!
if (!refs.computation) {
// Render is committed, but we no longer have a computation. Invoke
// forceUpdate and let the next render recreate the computation.
if (!skipUpdate) {
forceUpdate();
} else {
Tracker.nonreactive(() => Tracker.autorun((c: Tracker.Computation) => {
const data = reactiveFn(c);
refs.computation = c;
if (!skipUpdate(refs.trackerData, data)) {
// For any reactive change, forceUpdate and let the next render rebuild the computation.
forceUpdate();
}
}));
}
}

// stop the computation on unmount
return () =>{
refs.computation?.stop();
delete refs.computation;
}
}, []);

Expand All @@ -121,6 +127,7 @@ const useTrackerWithDeps = <T = any>(reactiveFn: IReactiveFn<T>, deps: Dependenc
reactiveFn: IReactiveFn<T>;
data?: T;
comp?: Tracker.Computation;
isMounted?: boolean;
}>({ reactiveFn });

// keep reactiveFn ref fresh
Expand All @@ -140,29 +147,31 @@ const useTrackerWithDeps = <T = any>(reactiveFn: IReactiveFn<T>, deps: Dependenc
refs.comp = comp;
// To avoid creating side effects in render, stop the computation immediately
Meteor.defer(() => {
if (refs.comp) {
if (!refs.isMounted && refs.comp) {
refs.comp.stop();
delete refs.comp;
}
});
}, deps);

useEffect(() => {
if (refs.comp) {
refs.comp.stop();
delete refs.comp;
// Let subsequent renders know we are mounted (render is committed).
refs.isMounted = true;

if (!refs.comp) {
refs.comp = Tracker.nonreactive(
() => Tracker.autorun((c) => {
const data: T = refs.reactiveFn(c);
if (!skipUpdate || !skipUpdate(refs.data, data)) {
refs.data = data;
forceUpdate();
}
})
);
}
const computation = Tracker.nonreactive(
() => Tracker.autorun((c) => {
const data: T = refs.reactiveFn(c);
if (!skipUpdate || !skipUpdate(refs.data, data)) {
refs.data = data;
forceUpdate();
}
})
);

return () => {
computation.stop();
refs.comp.stop();
};
}, deps);

Expand Down

3 comments on commit 384da2f

@yched
Copy link
Contributor

@yched yched commented on 384da2f Feb 1, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to cause useTrackerWithDeps to re-render way too much (2.4.0 is fine, bug appears in 2.5.0)
#382

@CaptainN
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yched I wonder whether this is true with every version of React, or just with a specific range of versions.

@yched
Copy link
Contributor

@yched yched commented on 384da2f Feb 25, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CaptainN I could reproduce the 2.5.0 bug described in #382 using React 17.0.2 and 18.2.0. I didn't try earlier versions though.

Please sign in to comment.