Double subscriptions when using useTracker in lazy components

[ksinas]

When using useTracker in lazy components I noticed that sometimes data get subscribed, unsubscribed and subscribed again.

This happens because useMemo and useEffect sometimes gets called synchronously, thus comp.stop() gets called too late, resulting in second subscription.

[CaptainN]

useMemo and useEffect (given the same set of deps) should not ever be called synchronously. Can you point me to some information about cases where it might?

A reproduction or unit test would be even better.

I wonder if the useEffect hook is getting called before Meteor.defer gets a chance to run for some other reason in some cases.

[CaptainN]

A simple rewrite could help (would need testing):

  const memoizedComp = 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) => {
        if (c.firstRun) data = refs.reactiveFn();
      })
    );
    if (Meteor.isDevelopment) {
      checkCursor(data);
    }
    return comp;
  }, deps);
  
  // This won't run in concurrency mode - leaving more errant subscriptions laying around...
  useLayoutEffect(() => {
    // To avoid creating side effects in render, stop the computation immediately
    memoizedComp.stop();
    // We might also need to set data here, not ideal
  }, [memoizedComp]);

  useEffect(() => {
    const computation =  Tracker.nonreactive(
      () => Tracker.autorun((c) => {
        setData(refs.reactiveFn(c));
      })
    );
    return () => {
      computation.stop();
    }
  }, deps);

In addition to adding another hook, it also might have a bug where the data state gets lost after deps change (would need to test that). It might be possible to put the clean-up in the useEffect block, but then we might have to defer the creation of the next computation - things are getting complicated again.

Oh! Right, this won't work, because it would leave the computation laying around in concurrency mode...

Update: Actually, that data state loss issue is already addressed by another change, which you can find on the comparator branch:

react-packages/packages/react-meteor-data/useTracker.ts

Line 111 in e30e290

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

[CaptainN]

Anyway, I can fix this if needed, but I'll need a reproduction.

[ksinas]

Actually, the problem is with Meteor.defer. I replicated it and wrote a simple app which shows the sequence of actions.
You can see that they are called in different sequence when using a lazy component
https://codesandbox.io/s/delicate-hooks-61067?file=/src/App.js

[ksinas]

What if something like this? If we avoid stopping initial computation, then we would also preserve the same output from the first computation. Is there any negative consequence to this approach?

const useTracker = (reactiveFn, deps) => {
  let [data, setData] = useState();
  const { current: refs } = useRef({ reactiveFn, mounted: false, computation: null });
  refs.reactiveFn = reactiveFn;
  refs.data = data;

  useMemo(() => {
    refs.computation = Tracker.nonreactive(
      () => Tracker.autorun((c: Tracker.Computation) => {
       // keep the outorun alive, but do not create side effects in render
        refs.intermediateData = refs.reactiveFn();
        if (Meteor.isDevelopment) {
          checkCursor(refs.intermediateData);
        }
        if (c.firstRun) {
          refs.data = refs.intermediateData
          delete refs.intermediateData;
        } else if (refs.mounted) {
          setData(refs.intermediateData)
          delete refs.intermediateData;
        }
      })
    );
  }, deps);

  useEffect(() => {
    refs.mounted = true;
    if (refs.intermediateData) {
      setData(refs.intermediateData)
      delete refs.intermediateData;
    } else {
      setData(refs.data)
    }
    return () => {
      refs.mounted = false;
      refs.computation.stop()
      refs.computation = null;
    }
  }, deps);
  return refs.data;
}

[CaptainN]

Interesting. Replacing meteorDefer with setTimeout fixes your test case. But I don't know if that's a workable solution in production, because if I understand it correctly, browser limit the smallest setTimeout interval to 4ms, and I don't know how long it takes in minimum for useEffect to run after render.

I can come up with a solution without using setTimeout, but it's a bummer that Meteor.defer doesn't work in this case. Maybe this is something that can be fixed on that side?

[ksinas]

I wouldn't change it to setTimeout. You could just always stop the computation on the start of the useEffect. But how about my previous suggestion? It solves 2 issues instead.

[CaptainN]

Your suggestion makes 100% sense, except when concurrent mode is used, which is the real problem with all this. Your computation.stop() would never be called for 3 out of 4 renders created when using concurrent mode, because in that mode the render body is called multiple times, but useEffect is only called once on the instance that gets "committed" (sometimes we say "mounted").

Meteor.subscribe actually has a ton of internal details that make it super hard to choreograph all this too. It has it's own internal lifecycle, and if it's not catered to just right, you get churn in your pub/subs.

We used to do a ton of work to keep the initial computation from first render alive, using timeouts, and checking things in useEffect - but it was horrendously difficult to maintain, and there were still many edge cases where it'd fail (in some cases causing subscriptions to die, and other to churn in a subscribe/unsubscribe death loop).

I'm actually thinking this is a bug in Meteor.defer - can you confirm whether it's ever run? I think it's not actually getting run in this case - that could be big bug in Meteor.defer that should be fixed on that side of this.

[CaptainN]

Actually, maybe we don't even need that defer? We might be able to take a page out of useTrackerNoDeps and simply call stop inline when the component hasn't yet been "mounted". I'll play around with that later.

[ksinas]

I'm not exactly sure how concurrent mode works under the hood, but I would be surprised that it would evaluate useMemo multiple times with the same deps. That sounds like a potential performance issue.

[CaptainN]

It definitely does - useMemo is just run inline with the rest of the render, and if the render is suspended, it's values are discarded. (I've seen some assertions that useRef is shared between renders in concurrent mode - but I don't think it is.) The individual "concurrently" rendered passes do not share any state between them.

[ksinas]

It definitely does - useMemo is just run inline with the rest of the render, and if the render is suspended, it's values are discarded. (I've seen some assertions that useRef is shared between renders in concurrent mode - but I don't think it is.) The individual "concurrently" rendered passes do not share any state between them.

I played a bit around and can confirm that indeed, useMemo is called multiple times and refs are not shared. So that means that in concurrent mode might be multiple subscribe / unsubscribe happening. I guess it's safer to do subscriptions purely in useEffect