Fix issue with useTracker and Subscriptions when using deps

[CaptainN]

• Fixes an edge case bug where Subscriptions could get lost or thrash, when using deps, and if the render is not mounted quickly enough. All of that timeout workaround stuff has been removed and replaced with a much more clean "react" implementation.
• Cleans up typescript defs
• Add resubscribe tests for with deps variant, and reorg test groupings
• Simplifies implementation
• Removes undocumented computation handler

The runtime performance characteristics of the hook changed enough to warrant a minor version bump.

[CaptainN]

@rijk @yched I actually think this is a bit important to get out, because I think the issues I uncovered are probably effecting some apps in ways that have gone either unnoticed, or have been transitory enough to create mysterious blips in folks apps.

[dburles]

Hey @CaptainN this looks great. Did you intend to bump the version in this PR too?

[CaptainN]

@dburles I typically leave that for the release engineer. The minor version number should at least be bumped. A bunch of internals changed. It's more than a bug fix release.

[dburles]

Minor version bump makes sense to me. I'll leave it to @filipenevola to handle the release.

[rj-david]

Getting a console error when running a page with withTracker

Warning: useTracker expected an array in it's second argument (dependency), but got type of undefined.

Not a problem with react-meteor-data 2.1.0

[Jerry360-bit]

Using useTracker single subscription. Running fine on [email protected] with react-meteor-data 2.1.4.
After updating to Meteor 2.0 with react-meteor-data 2.2.1, find and findOne methods mostly return undefined with :

Warning: unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering.

[filipenevola]

Hi @Jerry360-bit could you open an issue with a reproduction? Read more here

[Jerry360-bit]

Hi @filipenevola,

Thanks for your reply. Meteor and React newbie here trying to get back into coding after being gone for about 15 years....
Still getting used to a changed development world.....

I was using useTracker(fn ,[]). This worked with react-meteor-data 2.1.4 but not in 2.2.1.
Switched to useTracker(fn) and all is fine again.

Understand react-meteor-data is undergoing changes. I will just try to keep up and learn more about it.....

[msgfxeg]

I am having a problem with that fix after updating from meteor 2. I always getting React: “Maximum update depth exceeded” and I am stuck and my system is down for 3 days now. even I do not use any useEffect() in my code! after debugging an researching for 3 very log days I found this 'fix' and I believe it is not a fix it has broken my code so please help here is my useTracker() code which was working fine before upgrading

  const ready = useTracker(() => {
    return (
      Meteor.subscribe('countriesPub').ready() &&
      Meteor.subscribe('citiesPub').ready() &&
      Meteor.subscribe('customersPub').ready() &&
      Meteor.subscribe('districtsPub').ready() &&
      Meteor.subscribe('naturesPub').ready() &&
      Meteor.subscribe('finishesPub').ready() &&
      Meteor.subscribe('typesPub').ready() &&
      Meteor.subscribe('facilitiesPub').ready() &&
      Meteor.subscribe('amenitiesPub').ready() &&
      Meteor.subscribe('tagsPub').ready() &&
      Meteor.subscribe('brokersPub').ready() &&
      Meteor.subscribe('projectsPub').ready() &&
      Meteor.subscribe('unitsPub').ready() &&
      Meteor.subscribe('settingsPub').ready() &&
      Meteor.subscribe('notilogsPub').ready &&
      Meteor.subscribe('imagesPub').ready() &&
      Meteor.subscribe('documentsPub').ready() &&
      Meteor.subscribe('documentsNamesPub').ready()
    );
  });

  const {
    countries,
    cities,
    districts,
    natures,
    finishes,
    types,
    facilities,
    amenities,
    tags,
    brokers,
    projects,
    units,
    serial,
    systemSettings,
    systemSettingsId,
    customers,
    admins,
  } = useTracker(() => {
    if (ready) {
      const units = Units.find({}, { sort: { deleted: 1, serial: 1 } }).fetch();

      units.map((unit) => {
        unit.investors.map((investor) => {
          investor.customer = Customers.findOne({ _id: investor.customer });
        });
      });

      const setting = Settings.findOne({ systemSettings: { $exists: true } });
      const systemSettings = setting.systemSettings;
      const systemSettingsId = setting._id;
      const serialPrefix = systemSettings.unitSerialPrefix;
      const serialLeadingZeros = '0'.repeat(Number(systemSettings.unitSerialLeadingZeros));
      const serialSeed = Number(systemSettings.unitSerialSeed);
      const serialCounter = Number(systemSettings.unitSerialCounter);
      const serialNewValue = Number(serialSeed + serialCounter + 1);
      const serial = `${serialPrefix}${serialLeadingZeros}${serialNewValue}`;
      const customers = Customers.find({ 'profile.isAdmin': false }).fetch();
      const countries = Countries.find({}, { sort: { name: 1 } }).fetch();
      const cities = Cities.find({}, { sort: { name: 1 } }).fetch();
      const districts = Districts.find({}, { sort: { name: 1 } }).fetch();
      // const states= States.find({}, { sort: { name: 1 } }).fetch();
      const natures = Natures.find({}, { sort: { name: 1 } }).fetch();
      const finishes = Finishes.find({}, { sort: { name: 1 } }).fetch();
      const types = Types.find({}, { sort: { name: 1 } }).fetch();
      const facilities = Facilities.find({}, { sort: { name: 1 } }).fetch();
      const amenities = Amenities.find({}, { sort: { name: 1 } }).fetch();
      const tags = Tags.find({}, { sort: { name: 1 } }).fetch();
      const brokers = Brokers.find({}, { sort: { name: 1 } }).fetch();
      const projects = Projects.find({}, { sort: { name: 1 } }).fetch();
      const admins = Customers.find({ 'profile.isAdmin': true }).fetch();

      return {
        countries,
        cities,
        districts,
        natures,
        finishes,
        types,
        facilities,
        amenities,
        tags,
        brokers,
        projects,
        units,
        serial,
        systemSettings,
        systemSettingsId,
        customers,
        admins,
      };
    }
    return {
      countries: null,
      cities: null,
      districts: null,
      natures: null,
      finishes: null,
      types: null,
      facilities: null,
      amenities: null,
      tags: null,
      brokers: null,
      projects: null,
      units: null,
      serial: null,
      systemSettings: null,
      systemSettingsId: null,
      customers: null,
      admins: null,
    };
  }, [ready]);

[pinvooeg]

@Jerry360-bit thank you for your comment. It helped me a lot and my system is working again

[CaptainN]

@msgfxeg What specifically fixed the issue? What wasn't working, vs. what changed to make it work?

[msgfxeg]

@CaptainN Thank you for your concern and for asking. What was not working after upgrading from v2.1.4 to the latest version is useTracker(fn, [arrayOfDependencies]) even after weaving out the dependencies array still not working! what I have changed to make it work again is downgrading to v2.1.4 of react-meteor-data by editing the meteor packages file i.e. .meteor/packages from react-meteor-data to be react-meteor-data@=2.1.4 after that my system is up and running again.

[rijk]

I downgraded to 2.1.4 too, in my case the new version seemed to triggered an immediate rerender where the previous version did not, and this interrupted a framer-motion transition. I haven't done any more specific debugging yet, as downgrading fixed the issue for me.

[CaptainN]

I think I have an idea as to what's going on here. I think the reactiveFn reference is going stale. I need to add a way to keep that reference fresh without creating a ton of new renders. Probably that'll mean storing it in a ref on every render, and then using that ref in autoRun. The old hook did that for different reasons, and probably explains why that one doesn't exhibit this problem.

I'll patch this later tonight.

[CaptainN]

@rijk That FramerMotion problem is not likely to be solved by the fix I described above - but you should also know, that under some situations, the old hook will do that too... (You may be able to workaround that by using useMemo on the results of useTracker - yes, that feels hacky...) That uncertainty is one of the reasons I changed this (also to support StrictMode and ConcurrentMode cleanly).

For that type use case, you may need the experimental useFind hook, and/or we may need to add a comparator function extension to useTracker, which has been a desired feature for a long while. I think it may have become necessary to implement that for certain types of workflows.

For the issue of the error we have been discussing, if you remove the deps array altogether, does that fix the problem? (if you do that, it should use the useTrackerNoDeps internal variant, which is a completely different implementation from the deps variant)

[CaptainN]

Can you guys confirm that when removing the deps array (not just using an empty array) the problem persists?

[rijk]

Not sure if you were asking me as well, but I cannot remove the array as there are actual deps in it.

[Jerry360-bit]

For me the problem clears when not using any deps array. Using an empty array the problem persists.

[CaptainN]

@rijk You actually should be able to. Deps is optional in useTracker and I actually think the no-deps path should become the recommended default path for most users. You'll still get your double render though...

[make-github-pseudonymous-again]

I am running [email protected]. I have problems with things not loading depending on timing artefacts, for instance a hook will work only if some entries are already present in minimongo. I don't remember witnessing these problems before upgrading to Meteor 2.0.

Two examples of what I am experiencing:

1. In an attachments system: I can attach uploads to both patients and consultations. A consultation is linked to a patient. If I want to list all attachments that can be reached from a patient directly (attached to the patient) or indirectly (attached to a consultation linked to the patient) I end up with a hook waterfall. This leads to problems when trying to load these attachments from scratch but no problem if I first open a page that loads the patient's consultations then navigate to the page that loads these attachments.
2. In a paginating system: if I want page 7 to be displayed reactively, I need to load pages 1 through 7 in minimongo if I want to use basic pub/sub without polluting other pub/sub results. This page loads consultations and each consultation loads the patient it is linked to. If I navigate forward to page 8, consultations get loaded but not their respective patients. If I navigate backwards however, both consultations and their patients are loaded properly. Note that navigating forward loads an additional page of results in minimongo while navigating backwards removes a page of results in minimongo.

In both cases the problem can be generalized to the following: On a fresh query, with a waterfall of hooks (X loading Y's loading Z's), I end up with the second hook waiting forever (Y's are loaded but not Z's).

• Example 1 is an instance of this problem with X = page, Y = consultations, Z = patients
• Example 2 is an instance of this problem with X = patient, Y = consultations, Z = attachments

Another problem I have with the pagination example is that, if multiple consultations of the same pages are linked to the same patient, sometimes, only a subset of the patients are loaded successfully, the others hang forever.

All these problem go away if I remove dependency arrays in useTracker calls. These dependency arrays are arrays of values that uniquely identify each query.

Are my problems related to what is being discussed here?

[CaptainN]

@aureooms Oh hey, you are here. PR #314 should fix these problems. I'd appreciate it if you could give that a test.

In case you haven't done that before, you can do test by downloading/checking out the bugfix branch, and copying the react-meteor-data folder to your local packages directory in your app.

[make-github-pseudonymous-again]

@CaptainN I confirm using deps in useTracker works as expected in my project with [email protected].

[make-github-pseudonymous-again]

Could a hook waterfall be used as regression test for this fix?

[CaptainN]

@aureooms I'm not sure what you mean, but maybe. I think there were up to 2 problems:

• There is a chance the computation in useEffect was getting stopped by a containing computation. I put in mitigation for that possibility - it's a bit blind, but we can know that the mitigation can work, where as I am uncertain that it'll work right without it. Easy win.
• I think the other problem is more likely what was causing this for users, which is that the reactive function was cached from a previous render, and was not receiving access to the latest values in it's own scope. We can definitely write tests to figure this out and prevent regression, however, I'm light on the time to do that. The mitigation for this was to keep a reference object in the hook, and always update the reactiveFn reference. The old version of the hook had that for a different reason.

Another thing we should write tests for is cases where deps are used incorrectly (like someone uses an object ref which will change on every render - easy to write a test for that one). This probably wouldn't have worked in 2.2.1 because of issue #2 above, but we should still not allow the hook to fail in these cases. It might even be possible to detect this in some way during development, and kick up a helpful warning, similar to that useEffect warning.

[make-github-pseudonymous-again]

I'm not sure what you mean

@CaptainN What utterance are you referring to?

[shadoath]

Why are we not using setData(reactiveFn()) here as it is done below?

[CaptainN]

Because that would cause an immediate rerender. The idea here is to temporarily override the value of data during the initial render (or immediately after deps change). Then when the component is mounted the next tracker instance set up in useEffect will update the state data.

There is an assumption made here, which I'm not confident in, that the initial render will never be run more than once before useEffect is run. I'm thinking that might not be true if a user invokes an immediate rerender themselves (by triggering some hook in render). I can actually write a test for this.

I already have a mitigation in place for this possibility, but I don't want to push it until I've confirmed it's needed.
https://github.com/meteor/react-packages/blob/better-handle-data-with-refs/packages/react-meteor-data/useTracker.ts

[afrokick]

Found an issue #327

As a solution we can store it in useRef with flag initialized, which turn to true after useEffect.
Something like:

function useTracker(){
  ...
  const tempData = useRef({result:undefined, initialized:false});
  ...
  if(comp.firstRun) {
    tempData.current.result = refs.reactiveFn();
    tempData.current.initialized = false;
  }
  ...
  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;
    };
  });
  ...
  return !tempData.current.initialized ? (tempData.current.result : data) as T;
}

[CaptainN]

One of the things I changed in #321 was to keep the value of the first render in a ref - can you try that branch and see if it solves your issue?

[CaptainN]

I'm not sure what you mean

@CaptainN What utterance are you referring to?

I was referring to this:

Could a hook waterfall be used as regression test for this fix?

[make-github-pseudonymous-again]

@CaptainN Something like the following

const useStuff = (stuff) => {
    const things = useThings(stuff);
    const foo = useFoo(things);
    const bar = useBar(foo);
    // ...
};