Half duplex

[CaptainN]

This PR retains the initial synchronous behavior of @menelike's branch (including the first time after deps change), while allowing subsequent activity to run asynchronously, and reuses the existing computation. I think this is in line with the goal of retaining backward compatibility, and more efficient than rebuilding the computation for every run through the computation.

[CaptainN]

This also includes a patch for cleaning up eslint errors

[menelike]

Doesn't this cancel reactivity? if the computation is stopped directly on firstRun a second run will never happen.

[CaptainN]

The dispose method cancels the one currently in the computation ref:

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

But then we record the new one on the line following:

        if (c.firstRun) {
          // if we are re-creating the computation, we need to stop the old one.
          dispose();

          // store the new computation
          computation.current = c

          // store the deps for comparison on next render
          previousDeps.current = deps;
        } else {
          // use a uniqueCounter to trigger a state change to force a re-render
          forceUpdate(++uniqueCounter);
        }

[menelike]

I completely missed that, thanks!

[menelike]

First of all, I love the cleanup!

I don't have much time now, but I will definitely be able to make a proper review at the end of this week.

I did not have the chance to test your code, but reading it it seems that you cancel the computation straight on the first run, as a result, a reactive change won't trigger anything and forceUpdate is never called. Am I missing something?

[CaptainN]

Maybe it would make sense to disambiguate that:

    Tracker.nonreactive(() => (
      Tracker.autorun((c) => {
        // This will capture data synchronously on first run (and after deps change).
        // Additional cycles will follow the normal computation behavior.
        const data = reactiveFn();
        Meteor.isDevelopment && checkCursor(data);
        trackerData.current = data;

        if (c.firstRun) {
          // if we are re-creating the computation, we need to stop the old one.
          if (computation.current) {
            computation.current.stop();
          }

          // store the new computation
          computation.current = c

          // store the deps for comparison on next render
          previousDeps.current = deps;
        } else {
          // use a uniqueCounter to trigger a state change to force a re-render
          forceUpdate(++uniqueCounter);
        }
      })
    ));

[menelike]

It looks much cleaner and the idea is really neat!

Unfortunately, I get loops in my large project if I use this code. I guess that the handover between two reactive computations is causing this (dispose() is called like crazy). Before your change, only one active computation existed at a time, now a computation is canceled within the firstRun of the newly created one, as a result, those two computations might trigger each other. But this is just a guess, it's hard to recreate a reproducible test case.

[CaptainN]

Hmm, actually that makes sense, based on some of the digging I"ve been doing to try and understand why the original implementation was the way it was. Meteor has all sorts of stuff in it to reuse computations, and stack them up, to sort of allow old unused computations to expire. I'm still not entirely sure how that works, and would rather simply correctly address the issue. I have one other idea, and I think it'll work:

  // this is called like at componentWillMount and componentWillUpdate equally
  // in order to support render calls with synchronous data from the reactive computation
  // if prevDeps or deps are not set areHookInputsEqual always returns false
  // and the reactive functions is always called
  if (!areHookInputsEqual(deps, previousDeps.current)) {
    // If we already have a computation, we need to stop that one first
    dispose();

    // 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.
    Tracker.nonreactive(() => (
      Tracker.autorun((c) => {
        // This will capture data synchronously on first run (and after deps change).
        // Additional cycles will follow the normal computation behavior.
        const data = reactiveFn();
        Meteor.isDevelopment && checkCursor(data);
        trackerData.current = data;

        if (c.firstRun) {
          // store the new computation
          computation.current = c

          // store the deps for comparison on next render
          previousDeps.current = deps;
        } else {
          // use a uniqueCounter to trigger a state change to force a re-render
          forceUpdate(++uniqueCounter);
        }
      })
    ));
  }

All I did was move the computation disposition out of the new computation's handler. I'll give this some tests.

Maybe it's not worth the effort for this minor improvement (or maybe there actually is a reason they had it the other way, and I just don't know what it is). I can submit the eslint fixes separately, or if you think the other PR #2 I made is useful, that contains those eslint fixes as well.

[CaptainN]

BTW, this is a pretty important component - it's a bit odd that it has no unit tests.

[CaptainN]

This works correctly in my testing with no loops - with both versions. Here's a simple test:

const useSession = (name, defaultValue, cleanup) => {
  import { Session } from 'meteor/session'
  const value = useTracker(() => {
    Session.setDefault(name, defaultValue)
    return Session.get(name)
  }, [name, defaultValue], cleanup)
  return [
    value,
    (val) => Session.set(name, val)
  ]
}

let name = 'test'
function Test () {
  const [count] = useSession(name, 0)
  console.log(count)
  return <div>{count}</div>
}

Meteor.startup(() => {
  import { Session } from 'meteor/session'
  
  // increments the session value to test meteor reactive data changes
  Session.setDefault(name, 0)
  setInterval(() => {
    Session.set(name, Session.get(name) + 1)
  }, 500)

  // Also see what happens when the name changes, and invalidates the old computation
  setTimeout(() => {
    name = 'test2'
  }, 3000)
})

I also put logs out for the first-run and additional-run blocks inside the hook, and the same - one log per first-run and additional-run blocks as expected.

Update: I'm going an update to avoid blowing up your email further. In thinking through this, it became obvious that the code right after the deps check, and the code in c.firstRun block run in the same thread every time the deps change. So having it one place or the other doesn't matter much - but I created a patch to put it back the other way, because I think it's a bit easier to read that way. The main thing this patch does then is avoid stopping and recreating the computation every time a reactive value changes, and that seems worth while.

[menelike]

Loops are gone now, I am trying to determine if performance is increased, one of my very simple test is just counting calls to Meteor.subscribe() like

const originalSub = Meteor.subscribe;
let subcounter = 0;
Meteor.subscribe = function(name) {
  console.log('sub', subcounter++);
  return originalSub(...arguments)
};

I am getting more counts with this code, but I have no clue why this happens. Reading the code it should actually decrease the number of calls to Meteor.subscribe(). Do you have any idea why this happens or can you reproduce this on your side?

Keep in mind that 90% of my codebase still uses withTracker.

Update

This is driving me nuts, maybe your code is just too fast and is less blocking and therefore updates/re-rendering is a lot faster which may result in this?

Your changes are way more elegant, I absolutely like it and want to merge it, just need to understand what is causing the increased number of calls to Meteor.subscribe()

[menelike]

I think that Meteor.subscribe() is called more than before because stopping the computation was less reactive and more CPU intensive. The result of this PR is that the current computation just triggers more often - which is actually a good thing!

So if you don't have any remarks on this, we're good to merge! Fantastic job!

[CaptainN]

I can't think of anything that would cause an increase in the invocation rate. The only real difference I can think of is in the question of "does the reactive function get called more often?" would be the possibility that the non-synchronous computation could get called, then trigger a render, then during render, or even before, the computation could fire again, triggering another render. This is unlikely to be a problem 99% of the time, since we are talking about a very short period of time (milliseconds) between async invocation and react render, but it's is still a possibility. The only other thing I'm not certain of, is whether the computation will fire one last time after the .stop() method is invoked - I would not think so, but I don't know that for sure.

An aside: If you do a subscription in a reactive function, I would assume it would have to call the subscribe method every time anything inside the computation changes. Maybe it makes sense to create additional computations to separate the work (something hooks makes easier as compared to containers):

var params = { public: props.public }
var isReady = useTracker(() => Meteor.subscribe('subName', params).ready(), [params]);
var data = useTracker(() => isReady && MyCollection.find(params).fetch(), [params, isReady]);

Actually, the trouble with the above is that params is a new object every time, and would trigger a refresh of the computation anyway (this isn't any worse than the old behavior - so that's great!). I bet this is going to be a confusion for implementers. Folks will likely try to use useMemo and other tricks to avoid recomputation, but the best solution is probably to build complex objects from properties inside the computation. Example:

// Only reactively (meteor) updates when the subscription state changes.
var isReady = useTracker(() => Meteor.subscribe('subName', { public: props.public }).ready(), [props.public]);
// Reactively (react) updates when isReady, and reactively updates (meteor) when collection documents change.
var data = useTracker(() => !isReady ? [] : MyCollection.find({ public: props.public }).fetch(), [props.public, isReady]);
// both change in response to changes to props.public

What's nice about this separation is, the second computation will change any time any of the documents in MyCollection change, but won't continuously invoke Meteor.subscribe!

What's great about this pattern, is it's only possible because the firstRun is synchronous (isReady will contain the correct result immediately). It probably wouldn't have worked correctly without your initiative on this.

(BTW, the speed increase I noted in the other issue was comparing your synchronous work, before my patch, with the previous work based on useEffect/useState - your changes were fast already, we are just noodling around the edges at this point.)

[menelike]

@CaptainN

I think I found an issue with this PR (not a big thing, but still worth to discuss).

When using this with withTracker deps are always undefined for a good reason. When a second computation is done, it will always run the reactive function again, and then trigger an update with forceUpdate. Then on the update (since deps are undefined or null) the computation is stopped and it recreated a computation and calls the reactive function again.

As a result, the reactive function is called twice where only one call is actually needed.

[menelike]

If !c.firstRun then deps should be checked first before the reactiveFunction is called. If deps would lead to a recreation of the computation, it should just forceUpdate, otherwise it should call the reactiveFunction and forceUpdate

[CaptainN]

Hmm, you are right, in that case (or in a case where a user uses useTracker with no deps) it certainly will run the reactive function twice (once asyncronously, then again on render). That's an argument in favor of leaving it the way it was.

An alternative would be as you described:

  if (!areHookInputsEqual(deps, refs.previousDeps)) {
    // if we are re-creating the computation, we need to stop the old one.
    dispose();

    // store the deps for comparison on next render
    refs.previousDeps = deps;

    // 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.
    refs.computation = Tracker.nonreactive(() => (
      Tracker.autorun((c) => {
        if (c.firstRun) {
          // This will capture data synchronously on first run (and after deps change).
          // Additional cycles will follow the normal computation behavior.
          const data = reactiveFn();
          if (Meteor.isDevelopment) checkCursor(data);
          refs.trackerData = data;
        } else {
          // if the deps are `falsy` then we want to skip running the reactiveFn and
          // let it run synchronously on next render.
          if (!areHookInputsEqual(deps, refs.previousDeps)) {
            const data = reactiveFn();
            if (Meteor.isDevelopment) checkCursor(data);
            refs.trackerData = data;
          }
          // use a uniqueCounter to trigger a state change to force a re-render
          forceUpdate(++uniqueCounter);
        }
      })
    ));
  }

[menelike]

this is untested code

I just fixed your if clause in the 2+ run, this should now work as best as it can. WDYT?

I am not sure if the call of c.stop() is really needed as it will be disposed on re-run. But stopping the computation directly might cover async related issues.

  if (!areHookInputsEqual(deps, refs.previousDeps)) {
    // if we are re-creating the computation, we need to stop the old one.
    dispose();

    // store the deps for comparison on next render
    refs.previousDeps = deps;

    // 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.
    refs.computation = Tracker.nonreactive(() => (
      Tracker.autorun((c) => {

        const setData = () => {
          const data = reactiveFn();
          if (Meteor.isDevelopment) checkCursor(data);
          refs.trackerData = data;
        };

        if (c.firstRun) {
          // This will capture data synchronously on first run (and after deps change).
          // Additional cycles will follow the normal computation behavior.
          setData();
        } else {
          // if the deps are `falsy` (which is always the case if deps or previousDeps is null or undefined)
          // then we want to skip running the reactiveFn and let it run synchronously on next render.
          if (!areHookInputsEqual(deps, refs.previousDeps)) {
            c.stop();
          } else {
            setData();
          }
          // use a uniqueCounter to trigger a state change to force a re-render
          forceUpdate(++uniqueCounter);
        }
      })
    ));
  }