useTracker returns undefined for the second render

[afrokick]

The issue occurred in 2.2.2. The issue not exists in 2.1.4.

@CaptainN After #306 the function starts to return undefined value.

I had some investigation.

It appears after second render. Because useMemo only fires on first render, the value from useMemo didn't store on next render.

https://github.com/meteor/react-packages/blob/master/packages/react-meteor-data/useTracker.ts#L117

I found discussion in PR about this line: https://github.com/meteor/react-packages/pull/306#discussion_r568788170

So, we need a test for my test case?)

Simple component for reproducing:

import React, { useState } from 'react';
import { useTracker } from 'meteor/react-meteor-data';

export const App = () => {
  const [s, setS] = useState(false);
  useTracker(() => {
    //to trigger rerender
    setS(true);
  }, []);

  const links = useTracker(() => [], []);

  //BUG, because the 'links' must always be 'array'
  if (links === undefined) {
    console.error(`links === undefined`);//for 2d render it equals undefined
  }

  return (
    <div>
      <h2>Links count:{links.length}</h2>
    </div>
  );
};

[afrokick]

Duplicate my solution from https://github.com/meteor/react-packages/pull/306#discussion_r598607776

function useTracker(){
  ...
  const tempData = useRef({ result: undefined, initialized: false });
  ...
  useMemo(() => {
    ...
    if(comp.firstRun) {
      tempData.current.result = refs.reactiveFn(); //store result in temp variable.
      tempData.current.initialized = false; //reset it, because deps can be changed
    }
    ...
  }, deps);
  ...
  useEffect(() => {
    ...
    const res = refs.reactiveFn(c);
    setData(res);
    tempData.current.result = undefined;
    tempData.current.initialized = true;
    ...
    return () => {
      computation.stop();
      tempData.current.result = undefined;
      tempData.current.initialized = false;
    };
  }, deps);
  ...
  return !tempData.current.initialized ? (tempData.current.result : data) as T;
}

Should it works properly? @CaptainN

[CaptainN]

Your example might be a little too contrived - why are you using setState from within useTracker?

Still, the second item shouldn't return undefined on subsequent renders, so I can look in to that.

[afrokick]

I simplified the example to get a simple code which can reproduces the issue.

Our case is useSubscription hook:

  const [loading, setLoading] = useState(true);
  const unmounted = useUnmounted();

  useTracker(() => {
    const s = Meteor.subscribe(pubName, ...subOpts, {
      onStop(error?: Meteor.Error) {
        if (error) {
          log(pubName, `sub stop with error:`, error.toString());
        }
      },
    });

    if (unmounted.current) return;

    setLoading(!s.ready());
  }, [pubName, ...subOpts]);

  return loading;

[CaptainN]

You still don't need to call a state setter from within useTracker:

  const loading = useTracker(() => {
    const s = Meteor.subscribe(pubName, ...subOpts, {
      onStop(error?: Meteor.Error) {
        if (error) {
          log(pubName, `sub stop with error:`, error.toString());
        }
      },
    });

    return !s.ready();
  }, [pubName, ...subOpts]);

  return loading;

I wonder if that would correct the problem - but I still think there may be an issue, and I will look in to it.

[afrokick]

Thanks for the tip!

But anyway, there is a lot of cases where one of the hooks can use setState which cause rerender

[CaptainN]

@afrokick did you see my comment in the PR? I think I may have mitigated for this exact problem in another branch.

Basically, the only reason I can see for this to be an issue is if the render runs a second time before it's committed (before useEffect runs). That's not supposed to happen in React - but I think there may be a case where if you call a state setter during first render, it might actually do that (which is what your snippets are doing).

The add-comparator-argument branch (PR #321) has a change to the way it stores the value on first render (it stores data in a ref, like you suggested), and that might already mitigate the issue. Can you test that branch out?

[afrokick]

@CaptainN it doesn't work https://github.com/meteor/react-packages/pull/321/files#r599728699 Lets discuss your PR in branch PR

[CaptainN]

That's an old settled PR - it's probably better to discuss here (I keep losing it in there).

That said, I think I see the problem, and will try to get a solution together this week.

[crapthings]

version is 2.2.2

it looks we have extra 1 rerun for a simple example

function App () {
  console.log('rerun')

  const ready = useTracker(() => {
    return Meteor.subscribe('something').ready()
  }, [])

  console.log(ready)

  if (!ready) {
    return <div>loading</div>
  }

  return (
    <div></div>
  )
}

i think it should only rerun 2 times

[crapthings]

test with 2.1.4 doesn't has the prob

[afrokick]

@crapthings it is "expected" behaviour, because first comes from useMemo(initial render), the second - from useEffect(re-render)

[CaptainN]

It's just the way React works - but the react team is adamant that renders aught to be cheap, and side effect free, and Tracker has always been pretty fast. In order to properly support suspense, error bounds, concurrent rendering, and strict mode properly, we just need to accept the second run on mount. It's still fast, and in 99% of cases should not be a problem.

I really need to write that performance article, but here are some highlights:

• Don't succumb to premature optimizations! Write your app so it runs correctly, then measure, and then optimize where needed.
• Supplying deps is an optimization. In most cases you don't need them. useTracker is fast either way. I've seen too many bugs from incorrect use of deps even in useEffect to recommend that as a standard. If you don't need it, just skip it. You'll you'll save yourself a lot of headaches.
• The upcoming shouldUpdate comparator argument should not be abused. I've been hesitant to even release that, because unless you really know what you are doing, many uses are going to make an app slower, not faster. I fear folks are going to over use it, or worse, use something like fast-deep-equals. That's not a good idea, for many reasons.

[StorytellerCZ]

@CaptainN so I guess we can close this issue?

[grossbart]

It was still an issue for us in some situations, https://github.com/meteor/react-packages/pull/329 would fix this problem.

[CaptainN]

The solution in #329 is very similar to the solution in #321 - and I added some tests for further edge cases (there is still one left to work out, I'll write a test and fix soon).

Please check out #321 and see if it solves your issue. That is very close to shipping.

[CaptainN]

I just added doc updates for skipUpdate function which finishes #321. This should let you effectively control re-renders in performance sensitive situations. Please measure before using! This feature should not be abused!

This release will also fix the various bugs mentioned here and elsewhere with lost data.

[CaptainN]

PR #321 was released as version react-meteor-data@2.3.1 - that should have fixed this up. Let us know.