Expose useTracker hook and reimplement withTracker HOC on top of it

[yched]

As initially posted on forums.meteor, this is a first attempt at providing a useTracker hook to get reactive data in functionnal React components.

Example usage :

import { useTracker } from 'Meteor/react-meteor-data';
function MyComponent({ docId }) {
  const doc = useTracker(() => Documents.findOne(docId), [docId]);
  return doc ? <div><h1>{doc.title}</h1>{doc.body}</div> : <Spinner/>
}

The PR also re-implements the existing withTracker HOC on top of it, which:

• allows testing the hook in existing applications and class-based components
• solves withTracker's compatibility with React strict mode and the forthcoming Suspense and concurrent mode (issues #256, #252, #242 / PR #261)

A couple questions remain open (more details in comments below, to follow soon), but this already seems to work fine in our real-world app heavily based on withTracker.

[yched]

Also, as a reminder : besides allowing easier access to reactive Tracker data in React components through the new useTracker() syntax, the PR makes existing apps, using the existing withTracker() HOC, compatible with the forthcoming React releases.

From what was announced a couple weeks ago, the next React minor release should output deprecation warnings on the lifecycle methods used by the current withTracker() implementation. Would be nice not having to rush the new implementation out when this happens, and have at least a beta under our belt ?

[CaptainN]

I've been using this in production for months (both withTracker and useTracker) and it's been rock solid.

[jchristman]

Same here from @CaptainN

[whoknowsb]

Guys this is just amazing! Any news?

[menelike]

There might be a noticeable breaking change which is a result of how effects in react work.

Given the following simplified example used with React-Router

<Switch>
  <RoutePermission whenIsUser path="/foo"> // has access because logged In
    <Foo />
  </RoutePermission>
  <RoutePermission permission="canAccessBar" path="/bar"> // has access because has permission
    <Bar />
  </RoutePermission>
</Switch>

and

// either whenIsUser or permission must be set
const RoutePermission = ({ whenIsUser, permission, children }) => {
  const hasPermission = useTracker(checkPermFunc, [permission]);
  const isUser = useTracker(() => Meteor.userId());

 if (whenIsUser && isUser) return children;  
 if (permission && hasPermission) return children

  return <Redirect path="/signin"/>;
}

When the user initially navigates to "/foo" (whenIsUser && isUser) succeeds because he is a logged in user, but on a route change to "/bar" (permission && hasPermission) will fail even if checkPermFunc() returns true if run manually. This happens because

1. Even if declared multiple times, RoutePermission will be the same component on each Switch.
2. Hence, permission was undefined in RoutePermission on /foo, but when navigating to /bar permission will be canAccessBar BUT useTracker(() => trueOrFalse, [permission]) will return stale data (with permission is still undefined) on render. useEffect within useTracker will fire AFTER the render, but until then <Redirect path="/signin"/>; has already fired.

The only solution I could come up with is setting a key on every RoutePermission so that a unmount/mount happens where the initialData coming from useTracker is in sync.

This might be a very rare edge case It might others still help to figure out what's wrong. Unfortunately I've not found a backward compatible solution to solve this, it's just how react hooks work.

Hope this helps.

[CaptainN]

Why is RoutePermission the same component instance even when it's declared twice? That doesn't seem right - is this something specific to React Router? Can you add a unique key to each one to force them as separate instances?

<Switch>
  <RoutePermission whenIsUser path="/foo" key="/foo"> // has access because logged In
    <Foo />
  </RoutePermission>
  <RoutePermission permission="canAccessBar" path="/bar" key="/bar"> // has access because has permission
    <Bar />
  </RoutePermission>
</Switch>

(Update: duh, you said as much about the key... I'm still not sure why this is happening without the key - but I'm not sure its specific to this PR.)

[CaptainN]

Here is at least one issue on react-router which may be relevant: https://github.com/ReactTraining/react-router/issues/4578

[menelike]

@CaptainN

Thanks a lot for your feedback, highly appreciated.

Why is RoutePermission the same component instance even when it's declared twice?

AFAIK this is how the reconciliation in react has always worked. With the current withTracker a React.(Pure)Component wraps your target component, since components are instances, two instances won't be the same hence they are un/mounted. When using functional components this does not apply as instances don't exist like with components, as a result the reconciliation will reuse the existing react node. Btw. I use this as a neat feature to improve performance ;) (Be aware, I am not an expert on react's reconciliation)

Having said that, IMHO this is a breaking change, I don't think that this is only related to react router and affects specific design patterns in general (though I haven't had the time to analyze react router's source code, be aware)

TL;DR

If you rely on data from a reactive function in a react render call, you need to be able to deal with stale data otherwise you'll face async related issues.

[CaptainN]

I can't believe the react team would ship hooks with this kind of glaring problem. That would lead to a class of difficult to reason through errors... a leaky abstraction.

If it's really just a matter of having a component in there, we could wrap the internals of withTracker inside a Component, but I'd suggest simply documenting the issue, and accepting the breaking change with the version bump to 2.0.

[jchristman]

Wrapping the inside of withTracker with a Component completely negates one of the main gains of using hooks: you don’t pollute your React tree and make it 7000 layers deep. I vote (as much as my vote matters 😂) on document and release as a breaking change.

[CaptainN]

It's true that it would negate the benefits of using hooks - but if you are using withTracker, you are not using hooks anyway. The point is that it may be an option for users who need a perfectly backward compatible version. The hooks implementation would be unaffected.

[CaptainN]

The more I think about this, the more I think it's a React/React Router issue worth knowing about, but really has nothing to do with Meteor, or this implementation. Any type of component which does this type of if-ing or switching, would be subject to the same issue, with our without Meteor. The solution is to simply document it. The workaround is reasonable enough - just add a unique key to each component inside the (or whatever other flow statement framework you might use).

[menelike]

AFAIK this is how the reconciliation in react has always worked. With the current withTracker a React.(Pure)Component wraps your target component, since components are instances, two instances won't be the same hence they are un/mounted. When using functional components this does not apply as instances don't exist like with components, as a result the reconciliation will reuse the existing react node. Btw. I use this as a neat feature to improve performance ;) (Be aware, I am not an expert on react's reconciliation)

I have to correct myself, it doesn't matter if the component is a function or a class in context of reconciliation

I just set up https://github.com/menelike/useTrackerTest to demonstrate this issue.

The issue can be observed on the console when switching between hookFoo and hookBar or newWithTrackerFoo and newWithTrackerBar.

When on newWithTrackerFoo and then navigating to newWithTrackerBar: Note the 2 renders

When on withTrackerFoo and then navigating to withTrackerBar:

be aware, needs to be verified: My assumption is that updates with useState are done like componentDidUpdate = () => setState() hence two render phases and possible stale data on the first render.

[menelike]

Well, useEffect gets triggered by the dependency change, but after the render:

https://reactjs.org/docs/hooks-effect.html

The only way to solve this would be to compare pastDeps with currentDeps manually (=== is enough) and use the result of the reactive function instead of the state. Unfortunately there is no pastDeps (for a good reason), so AFAIK there is no solution in sight.

Since those bugs are really hard to debug I'd recommend to first use the old withTracker code for backwards compatibility and set it to deprecation for 3.x. Second useTracker should document this specific scenario.

The more I think about this, the more I think it's a React/React Router issue worth knowing about, but really has nothing to do with Meteor, or this implementation.

I must disagree, IMHO this issue happens in conjunction with specific design patterns (which are also used in Switch in React.Router)

[CaptainN]

@menelike After thinking about it - a lot more - I think you are right, we should deal with this (for cases even when the dep values change in other ways) - and it might be easy to fix.

Here is the current hook implementation:

// Forgetting the deps parameter would cause an infinite rerender loop, so we default to [].
function useTracker(reactiveFn, deps = []) {
  // Note : we always run the reactiveFn in Tracker.nonreactive in case
  // we are already inside a Tracker Computation. This can happen if someone calls
  // `ReactDOM.render` inside a Computation. In that case, we want to opt out
  // of the normal behavior of nested Computations, where if the outer one is
  // invalidated or stopped, it stops the inner one too.

  const [trackerData, setTrackerData] = useState(() => {
    // No side-effects are allowed when computing the initial value.
    // To get the initial return value for the 1st render on mount,
    // we run reactiveFn without autorun or subscriptions.
    // Note: maybe when React Suspense is officially available we could
    // throw a Promise instead to skip the 1st render altogether ?
    const realSubscribe = Meteor.subscribe;
    Meteor.subscribe = () => ({ stop: () => {}, ready: () => false });
    const initialData = Tracker.nonreactive(reactiveFn);
    Meteor.subscribe = realSubscribe;
    return initialData;
  });

  useEffect(() => {
    // Set up the reactive computation.
    const computation = Tracker.nonreactive(() =>
      Tracker.autorun(() => {
        const data = reactiveFn();
        Meteor.isDevelopment && checkCursor(data);
        setTrackerData(data);
      })
    );
    // On effect cleanup, stop the computation.
    return () => computation.stop();
  }, deps);

  return trackerData;
}

Might be as easy as resetting the state in the cleanup function:

      // ...
      // On effect cleanup, stop the computation, and reset the data.
      return () => {
        setTrackerData(reactiveFn());
        computation.stop();
      }
    }, deps);

We are already tracking deps internally, so if they change, we simply rerun the reactive method when stopping the old computation.

If you have a local fork, give this a try, and see if it solves the issue.

Update: I updated the code above from the correct branch...

[yched]

I'm on a vacation at the moment, and just had a cursory read of the email notifications for now. Will take a closer look in 10-ish days :-)

[CaptainN]

@yched I was looking at an old commit - it's fine in the current PR. Have a great time on vacation!

[menelike]

@CaptainN

What do you think about this approach? (not heavily tested, far from complete, still have some missing update calls)

This approach makes sure that in a render call no stale data is returned, the effect happens immediately.

I also could remove the monkey patching in the state initialization.:

    // No side-effects are allowed when computing the initial value.
    // To get the initial return value for the 1st render on mount,
    // we run reactiveFn without autorun or subscriptions.
    // Note: maybe when React Suspense is officially available we could
    // throw a Promise instead to skip the 1st render altogether ?
    const realSubscribe = Meteor.subscribe;
    Meteor.subscribe = () => ({ stop: () => {}, ready: () => false });
    const initialData = Tracker.nonreactive(reactiveFn);
    Meteor.subscribe = realSubscribe;
    return initialData;

and it seems that deps can be void now:

// Forgetting the deps parameter would cause an infinite rerender loop, so we default to [].
function useTracker(reactiveFn, deps = []) {

https://github.com/menelike/useTrackerTest/tree/trackerMap

const shallowEqualArray = (arrA, arrB) => {
  if (arrA === arrB) {
    return true;
  }

  if (!arrA || !arrB) {
    return false;
  }

  const len = arrA.length;

  if (arrB.length !== len) {
    return false;
  }

  for (let i = 0; i < len; i++) {
    if (arrA[i] !== arrB[i]) {
      return false;
    }
  }

  return true;
};

class DependencyMemoize {
  constructor() {
    this._instances = {};
    this._memoizedData = {};
    this._idCount = 0;
  }

  call(instanceId, deps, func) {
    if (deps && instanceId in this._memoizedData && shallowEqualArray(this._instances[instanceId], deps)) {
      // always update deps to avoid duplicates preventing garbage collection
      this._instances[instanceId] = deps;

      return this._memoizedData[instanceId];
    }

    const data = func();
    Meteor.isDevelopment && checkCursor(data);
    this._instances[instanceId] = deps;
    this._memoizedData[instanceId] = data;
    return data
  }

  update(instanceId, deps, data) {
    this._instances[instanceId] = deps;
    this._memoizedData[instanceId] = data;
  }

  createId() {
    this._idCount += 1;
    return this._idCount;
  }

  clear(instanceId) {
    delete this._instances[instanceId];
    delete this._memoizedData[instanceId];
  }
}

const memoize = new DependencyMemoize();

const useTracker = (reactiveFn, deps) => {
  // Note : we always run the reactiveFn in Tracker.nonreactive in case
  // we are already inside a Tracker Computation. This can happen if someone calls
  // `ReactDOM.render` inside a Computation. In that case, we want to opt out
  // of the normal behavior of nested Computations, where if the outer one is
  // invalidated or stopped, it stops the inner one too.

  const [[instanceId], forceUpdate] = useState(() => {
    // use an Array to enforce an update when forceUpdating the same Id
    // it seems the state is compared to prevState which prevents a forceUpdate?
    // could not find the specs for this
    return [memoize.createId()];
  });

  useEffect(() => {
    // Set up the reactive computation.
    const computation = Tracker.nonreactive(() =>
      Tracker.autorun((c) => {
        // trigger reactivity
        const data = reactiveFn(); // this is a wasted call when run initially, can this be avoided?
        Meteor.isDevelopment && checkCursor(data);
        if (!c.firstRun) {
          // reuse the reactive result and update the memoization
          // this avoids a call to reactiveFn() after forceUpdate triggers a re-render
          memoize.update(instanceId, deps, data);
          forceUpdate([instanceId]);
        }
      })
    );

    // On effect cleanup, stop the computation.
    return () => {
      computation.stop();
      memoize.clear(instanceId);
    }
  }, deps);

  return Tracker.nonreactive(() => memoize.call(instanceId, deps, reactiveFn));
};

[CaptainN]

@menelike My simple hack didn't work?

Update: Actually, my hack should probably use Tracker.nonreactive:

      // ...
      // On effect cleanup, stop the computation, and reset the data.
      return () => {
        setTrackerData(Tracker.nonreactive(() => reactiveFn()));
        computation.stop();
      }
    }, deps);

[CaptainN]

@menelike I like that you tried to remove the monkey patching in @yched's implementation of the initial value. I thought about other ways to remove that, or at least to make it monkey patch less often, such as checking Tracker.active, instead of just always monkey patching. I'm not actually sure why it monkey patches subscribe at all. Why does it do that?

I also wonder if there is a functional (or hooks based) way to write that class which could simplify things, making it so you don't have keep track of instances, and don't have to have shallowEqualArray at all.

[CaptainN]

It looks like we simply want the tracked functions to respond to changes in deps immediately in the current render pass, instead of waiting until the useEffect hook's "after render" cycle.

At first glance, the React hook useMemo is ideal for this! Sadly, it does not make a semantic guarantee, so we can't use it. grumble

I looked around for what React uses under the hood for their shallow compare. They have old add-on but at the top it says not to use it, and to use useMemo instead, which is not helpful.

I looked around the net, and this is a generally interesting problem, with dozens of different solutions to basically using a side effect within the render method instead of waiting until after render. Most of those can be solved easily with (and some of them use, however ill-advised) useMemo...

Since we can't, we can use useRef and a custom shallow compare to solve the problem. Here is my solution, using hooks, and your shallow compare function:

const shallowEqualArray = (arrA, arrB) => {
  if (arrA === arrB) {
    return true;
  }

  if (!arrA || !arrB) {
    return false;
  }

  const len = arrA.length;

  if (arrB.length !== len) {
    return false;
  }

  for (let i = 0; i < len; i++) {
    if (arrA[i] !== arrB[i]) {
      return false;
    }
  }

  return true;
};

// Similar to useState, except it will reset the state whenever deps is changed
const useStateWithDeps = (factory, deps) => {
  let [state, setState] = useState({});
  const previousDeps = useRef();
  if (!shallowEqualArray(previousDeps.current, deps)) {
    previousDeps.current = deps;
    state = factory();
    setState(state);
  }
  return [state, setState]
}
function useTracker(reactiveFn, deps = []) {
  const [computation, setComputation] = useState(null);
  const [trackerData, setTrackerData] = useStateWithDeps(() => {
    // Note : we always run the reactiveFn in Tracker.nonreactive in case
    // we are already inside a Tracker Computation. This can happen if someone calls
    // `ReactDOM.render` inside a Computation. In that case, we want to opt out
    // of the normal behavior of nested Computations, where if the outer one is
    // invalidated or stopped, it stops the inner one too.
    let returnData = null;
    Tracker.nonreactive(() => {
      setComputation(Tracker.autorun((c) => {
        const data = reactiveFn();
        Meteor.isDevelopment && checkCursor(data);
        if (c.firstRun) {
          returnData = data;
        } else {
          setTrackerData(data);
        }
      }));
    });
    return returnData;
  }, deps);

  // On unmount, stop the computation.
  useEffect(() => () => computation.stop(), [computation]);

  return trackerData;
}

So this basically does the same thing your code did, except doesn't need to keep track of all those instances, and such. This is working in my local general testing, but I didn't test the specific use case you are looking to address. Give it a go?

There may be further optimizations - since we are accepting the cost of setting up the computation in the render pass, do we even need to set it up again in useEffect? We can probably set up the main computation in useStateWithRef, and then only use useEffect for cleanup, or wire up cleanup some other way. (Update: I went ahead and changed it as described. I'm sure it needs more testing, and additional eyes looking at it.)

[CaptainN]

I think these changes actually improved my app performance (the updated one, which is probably not visible in email for those following along there). I guess this makes sense, since it's now only setting up a computation once, on render, instead of on render, and then again after render. It is also not rendering twice anymore, which it would have been doing before since a state is set in the withEffect hook, which of course runs after the first render. (Lighthouse seems to confirm this, shaving a second off time to idle in my app).

So we got a more responsive implementation (re-calculates immediately when deps are changed, instead of waiting), and also improved performance!

I think this is still compatible with upcoming Suspense API, but I'm not sure. I wrapped the app in <StrictMode> and got no errors or warnings from that. I also wrapped it in <React.unstable_ConcurrentMode> and it all works beautifully (though, I do get a bunch of warnings from other components like Helmet, Material-UI and React Router - I'm not sure if that's effecting anything)

[menelike]

@CaptainN

I think we're heading into the right direction. Your code results in two render calls on mount (in my example) which I can't observe with the current withTracker implementation.

Today I tried to replicate the current withTracker code (https://github.com/meteor/react-packages/blob/796a075a4bbb36212e6a74c9f3576b119d31c474/packages/react-meteor-data/ReactMeteorData.jsx) without reinventing the wheel:

function useTracker(reactiveFn, deps) {
  const previousDeps = useRef();
  const computation = useRef();
  const trackerData = useRef();

  const [, forceUpdate] = useState();

  const dispose = () => {
    if (computation.current) {
      computation.current.stop();
      computation.current = null;
    }
  };

  // this is called at componentWillMount and componentWillUpdate equally
  // simulates a synchronous useEffect, as a replacement for calculateData()
  // if prevDeps or deps are not set shallowEqualArray always returns false
  if (!shallowEqualArray(previousDeps.current, deps)) {
    dispose();

    // Todo
    // if (Meteor.isServer) {
    //   return component.getMeteorData();
    // }

    // Use Tracker.nonreactive in case we are inside a Tracker Computation.
    // This can happen if someone calls `ReactDOM.render` inside a Computation.
    // In that case, we want to opt out of the normal behavior of nested
    // Computations, where if the outer one is invalidated or stopped,
    // it stops the inner one.
    computation.current = Tracker.nonreactive(() => (
      Tracker.autorun((c) => {
        if (c.firstRun) {
          // Todo do we need a try finally block?
          const data = reactiveFn();
          Meteor.isDevelopment && checkCursor(data);

          // don't recreate the computation if no deps have changed
          previousDeps.current = deps;
          trackerData.current = data;
        } else {
          // make sure that shallowEqualArray returns false
          previousDeps.current = Math.random();
          // Stop this computation instead of using the re-run.
          // We use a brand-new autorun for each call to getMeteorData
          // to capture dependencies on any reactive data sources that
          // are accessed.  The reason we can't use a single autorun
          // for the lifetime of the component is that Tracker only
          // re-runs autoruns at flush time, while we need to be able to
          // re-call getMeteorData synchronously whenever we want, e.g.
          // from componentWillUpdate.
          c.stop();
          // trigger a re-render
          // Calling forceUpdate() triggers componentWillUpdate which
          // recalculates getMeteorData() and re-renders the component.
          forceUpdate(Math.random());
        }
      })
    ));
  }

  // replaces this._meteorDataManager.dispose(); on componentWillUnmount
  useEffect(() => dispose, []);

  return trackerData.current;
}

I only added shallowEqualArray in order to stay consistent with the effect API.

In withTracker I added props as Object.values() as it enhances the shallow comparison. const data = useTracker(() => getMeteorData(props) || {}, props ? Object.values(props) : props);

Update

Maybe it's even better to pass no deps at all in conjunction with withTracker so that a re-render always triggers a recomputation. memo wraps useTracker anyway and it's much closer to the current implementation.

---

From my observations (this stuff is hard to debug) this code works 100% the same as withTracker now (also tested in my large meteor project).

After all I think that the current code from this PR introduces some performance issues as you've mentioned earlier (it's like managing state only with componentDidUpdate while getDerivedStateFromProps is needed). Those should be gone now.

What do you think?

[menelike]

Note: One thing I still need to determine is if useEffect uses Object.is() and/or shallow comparison.

Update I found it

https://github.com/facebook/react/blob/master/packages/shared/objectIs.js

I don't understand all reasons behind reacts implementation (for example, why continue in the for loop and no early exit!?) https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberHooks.js#L307-L354

I'd propose:

function is(x, y) {
  return (
    (x === y && (x !== 0 || 1 / x === 1 / y)) || (x !== x && y !== y) // eslint-disable-line no-self-compare
  );
}

function shallowEqualArray(nextDeps, prevDeps) {
  if (!prevDeps || !nextDeps) {
    return false;
  }

  const len = nextDeps.length;

  if (prevDeps.length !== len) {
    return false;
  }

  for (let i = 0; i < len; i++) {
    if (!is(nextDeps[i], prevDeps[i])) {
      return false;
    }
  }

  return true;
}

[menelike]

@yched @CaptainN

I just submitted my proposal to https://github.com/yched/react-packages/pull/1

[CaptainN]

Oh right! I added a useState at the last minute to preserve a reference to computation for cleanup - but of course setting that triggers a second update. Actually, setting state as a backup the way I do in useStateWithDeps probably also triggers a re-render, so replacing that with useRef also makes sense. (I agree, this stuff is hard to debug/test. We should probably have unit tests.)

I'd like to re-invent the wheel a little bit. The original withTracker implementation throws away the computation and rebuilds it for every single props change. This makes sense, because they where limited to the information provided in props, and there's no way to know what effect those changes would have on the computation. In useTracker we are providing a way to say precisely what values will have an impact, so it makes sense to avoid recalculation if we have that data. We only need to create a new Tracker instance when the deps change. But we should also have a backup plan in cases where the user has not supplied deps - in that case, it can work like the old withTracker and always rebuild the computation. In this way, useTracker would never be worse than the old withTracker, but if we supply deps, it can be much better! All upside.

Here's what I'm thinking (very close to your last implementation):

function is(x, y) {
  return (
    (x === y && (x !== 0 || 1 / x === 1 / y)) || (x !== x && y !== y) // eslint-disable-line no-self-compare
  );
}

function shallowEqualArray(nextDeps, prevDeps) {
  if (!prevDeps || !nextDeps) {
    return false;
  }

  const len = nextDeps.length;

  if (prevDeps.length !== len) {
    return false;
  }

  for (let i = 0; i < len; i++) {
    if (!is(nextDeps[i], prevDeps[i])) {
      return false;
    }
  }

  return true;
}

function useTracker(reactiveFn, deps) {
  const computation = useRef();
  const previousDeps = useRef();
  const trackerData = useRef();

  // if prevDeps or deps are not set or falsy shallowEqualArray always returns false
  if (!shallowEqualArray(previousDeps.current, deps)) {
    previousDeps.current = deps;

    // Stop the previous computation
    if (computation.current) {
      computation.current.stop();
    }

    // Note : we always run the reactiveFn in Tracker.nonreactive in case
    // we are already inside a Tracker Computation. This can happen if someone calls
    // `ReactDOM.render` inside a Computation. In that case, we want to opt out
    // of the normal behavior of nested Computations, where if the outer one is
    // invalidated or stopped, it stops the inner one too.
    computation.current = Tracker.nonreactive(() => (
      Tracker.autorun((c) => {
        trackerData.current = reactiveFn();
        if (Meteor.isDevelopment) checkCursor(trackerData.current);
      })
    ));
  }

  // Stop the last computation on unmount
  useEffect(() => () => computation.current.stop(), []);

  return trackerData.current;
}

And then withTracker just needs a small update to remove usage of deps:

export const withTracker = options => Component => {
  const expandedOptions = typeof options === 'function' ? { getMeteorData: options } : options
  const { getMeteorData, pure = true } = expandedOptions

  function WithTracker(props) {
    const data = useTracker(() => getMeteorData(props) || {})
    return data ? <Component {...props} {...data} /> : null
  }

  return pure ? memo(WithTracker) : WithTracker
}

This should get us the benefits of the hooks lifecycle (or whatever it's called) while retaining the old behavior for withTracker.

[menelike]

I agree that this small piece of code should be as performant as it can get, it will be used several times in projects and might make the difference between a fast and slow meteor/react app.

I must miss something but I don't see any crucial changes to your example code, but I agree that useTrackershould have a dependency check and should minimize the need for wasted recomputations. But this is how I actually think that I implemented it?

In short:

• In conjunction with withTrackerevery re-render recreates the computation. A PureComponent is used to block unnecessary calls.
• useTracker gives you the freedom to pass deps, if you don't, the computation will be recreated on every render, if you do it checks for (shallow)equality and recreates only if a change is detected.

So the only place where a recomputation is wasted is when a reactive value changes over time and after the first run of the computation (!computation.firstRun). All my attempts to get rid of this failed, and I am not really sure if there is any performance gain to expect (after all, if deps change we MUST recreate the computation anyway?).

[CaptainN]

Actually, I'm questioning why this is here:

        } else {
          // make sure that shallowEqualArray returns false
          previousDeps.current = Math.random();
          // Stop this computation instead of using the re-run.
          // We use a brand-new autorun for each call to getMeteorData
          // to capture dependencies on any reactive data sources that
          // are accessed.  The reason we can't use a single autorun
          // for the lifetime of the component is that Tracker only
          // re-runs autoruns at flush time, while we need to be able to
          // re-call getMeteorData synchronously whenever we want, e.g.
          // from componentWillUpdate.
          c.stop();
          // trigger a re-render
          // Calling forceUpdate() triggers componentWillUpdate which
          // recalculates getMeteorData() and re-renders the component.
          forceUpdate(Math.random());
        }

It says something about "re-runs" but what does that actually mean? Is it not enough to use the memoized value from the last reactive run? Why does it spend an entire additional render cycle each time the computation is re-built (which in the case of withTracker is every time any prop changes or the component mounts)?

@menelike Do you know why it does this? It seems like we can get rid of that entire extra cycle, no?

[menelike]

@CaptainN Yes I agree, this is exactly the part I wanted to get rid of in the past but failed. I faced async related issues. My idea was to call the reactive function, save it to ref.current and force an update.

On the other hand I also think that re-creating a computation is not a heavy task, the most impact is on Meteor.subscribe of which arguments are checked per EJSON.equals(sub.params, params). So it should reuse the Subscriptions, and if it doesn't, this means that some deps have changed (which means the computation needs to be recreated anyway). I'll give it a second shot over the next days, but I don't expect a success.

The comment and logic you mentioned is actually taken from https://github.com/meteor/react-packages/blob/devel/packages/react-meteor-data/ReactMeteorData.jsx#L68-L79

[CaptainN]

Oh! I got it. That's actually the part that causes a re-render when the tracker data changes from outside the render method (duh). My solution would not work, given that. Using the old code is perfect (now that I understand it.)

Then here is some feedback for your PR - there is a duplicated effort - you don't need this, because it's already been handled at the bottom of the js file in the export statement.

  // When rendering on the server, we don't want to use the Tracker.
  // We only do the first rendering on the server so we can get the data right away
  if (Meteor.isServer) {
    return reactiveFn();
  }

I'd probably use an incrementing value instead of a random value for force update - it's entirely possible, if improbable, that you get the same value from two separate Math.random() invocations.

Other than that, it looks golden! Nice job.

[menelike]

Then here is some feedback for your PR - there is a duplicated effort - you don't need this, because it's already been handled at the bottom of the js file in the export statement.

fixed by https://github.com/yched/react-packages/pull/1/commits/365584f02b0a94dd6552a55827e2dc99596a6634

I'd probably use an incrementing value instead of a random value for force update - it's entirely possible, if improbable, that you get the same value from two separate Math.random() invocations.

fixed by https://github.com/yched/react-packages/pull/1/commits/c3803b96cf2eb843c8df549753586d398 fixed by https://github.com/yched/react-packages/pull/1/commits/ddfb7cd61b1e564d9a5144ee67797399ccc08055 This wasn't needed at all since null will always resolve into a re-render, it was an old piece of code I missed removing.

Other than that, it looks golden! Nice job.

Thank you very much and thanks a lot for this very productive collaboration. Actually this also helped me understand react hooks much better.

[CaptainN]

Yeah, this was fun - I learned a ton about Hooks, and even some of the upcoming stuff like Suspense, and StrictMode, ConcurrentMode (which makes my app SO SMOOTH). Super great learning time, and I think it's a real improvement to the product.

[CaptainN]

@menelike After sitting with this for a while, and doing some performance testing in my app, there is still one thing I don't quite understand. Why do we need to run the computation synchronously for every single change - what does this comment mean?

// We use a brand-new autorun for each call
// to capture dependencies on any reactive data sources that
// are accessed.  The reason we can't use a single autorun
// for the lifetime of the component is that Tracker only
// re-runs autoruns at flush time, while we need to be able to
// re-call the reactive function synchronously whenever we want, e.g.
// from next render.

It would seem we should only have to re-build the autorun when the deps change, not every time, but I don't really understand what this comment means when it talks about re-runs and "flush time". I get why it should run synchronously for the first run, but why do we need it to run synchronously for every additional render?

In my app, my computation is slow compared with my react render - almost 4 times slower (this probably indicates a problem with my queries, but still). It would be great to allow that to run asynchronously most of the time.

[CaptainN]

Actually, I think I was reading that wrong. When I tried to verify that, being more careful to start from empty offline storage, etc., the graph looked a bit different. Reading it correctly showed me an error outside this component, which is pretty useful. I don't think my queries are that slow, but I did run a bunch more tests, and I can at least confirm that the synchronous version of useTracker is faster than the asynchronous (useEffect based) version - in my specific case, it's an average of around 30-50% faster. Pretty sweet!

I would still like to know why its important to run the reactive function synchronously, as described in that comment.

[CaptainN]

I have a couple of other thoughts, we might as well figure out before any of this ships (I guess it will at some point).

1. It'd be nice to have a cleanup method in the interface - something users can use to clean up something, like stopping a subscription or clearing out a session var. I added this, along with a couple of examples in this PR.

2. In many cases, I wonder whether the react version of deps compare (shallow direct compare) is enough. This question applies both @yched's and @menelike's branches. In this example:

const myQueryFromProps =  { _id: props.myDocumentId, public: true }
const myData = useTracker(() => {
  const handle = Meteor.subscribe('myPublication', myQueryFromProps)
  return myCollection.find(myQueryFromProps).fetch()
}, [myQueryFromProps])

myQueryFromProps will be a new object every render. This makes the deps check kind of useless. I suppose this is standard "hooks" behavior, since a useEffect hook would have the same problem. Maybe that means it's not worth our time to change - but it might be worth documenting. The way around rerendering is to be careful about what you put in deps, or to use a ref to hold a single instance of the object and always modify that:

const myQueryFromProps = useRef({ _id: null, public: true })
myQueryFromProps.current._id = props.myDocumentId
const myData = useTracker(() => {
  const handle = Meteor.subscribe('myPublication', myQueryFromProps.current)
  return myCollection.find(myQueryFromProps.current).fetch()
}, Object.values(myQueryFromProps.current))

I guess this is more of a React thing than a Meteor thing, but is it reasonable to ask a Meteor user to do all this? In the React space, there is a react eslint "exhaustive-deps" package which would explicitly prohibit this kind of optimization, because they say it would lead to bugs. But I can't think of how you'd achieve the same level of of efficiency without it. There is a lengthy issue on this.

(This example is contrived - if a user creates that myQueryFromProps object inside the useTracker handler, instead of outside, it would also solve the problem. But what if a user grabs the object from elsewhere, and passes it in to useTracker? Its something worth knowing about.)

I think the good news on this is, I don't think this is any less efficient than the old withTracker, while it does at least offer a way to optimize if you want to.

[CaptainN]

@boomfly Would you mind testing out the PR here? It's a fork of this PR with some additional work to make the initial render synchronous. The result is more immediate availability from the reactive function and 1 fewer re-renders (the current version here always renders at least twice).

The PR @menelike spearheaded actually does a few nice things. If makes deps optional, and if you leave them out then useTracker performs essentially the same as withTracker has, with every reactive invocation running synchronously during render, and a new computation created with every change (this was standard for withTracker). If you add deps then the initial reactive function run (c.firstRun) still runs synchronously, but additional reactive runs will run outside of the react render cycle, asynchronously (like @yched's approach), and will re-use the computation instead of tearing down and rebuilding every time.

It's the best of both worlds!

[CaptainN]

How do we get this (and this) merged and deployed?

[CaptainN]

I just produced this neat package for maintaining state which survives HCP on top of useTracker - I had to include the hook implementation in my project to make it work. Will this ever get merged?

I also figured out how to do unit tests for react hooks in there. Maybe we should add some of those for useTracker.

[hwillson]

There has been a lot of conversation around this PR, so just to confirm - is this fully ready for review?

[CaptainN]

@hwillson The PR we have over on @yched's fork (spearheaded by @menelike) is ready for review. Would it be simpler to create a new PR directly from that fork?

That fork solves two important issues with the current PR:

1. It only renders once, instead of twice like the current implementation in this PR does for every mount. It runs the firstRun synchronously with React render, and only causes rerender when the reactive func's reactive stuff changes, or deps change. For reactive updates after firstRun, with deps the reactive func runs asynchronously, without deps it runs synchronously.
2. It corrects the response to deps changes to work as you would expect. Each time the deps change, it stops the computation immediately, then on the next render, it rebuilds the computation, and runs that firstRun synchronously.
3. As a bonus, it also makes deps optional - and if you don't specify them, then the hook works almost exactly like withTracker has worked, in that it will always discard and rebuild the computation, and every invocation runs synchronously with render. This gives a perfectly backward compatible behavior to withTracker.

[hwillson]

@CaptainN Creating a new PR from that fork would be great - thanks!

[yched]

This is now superceded by #273, I should have closed this a while ago.