meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
574 stars 159 forks source link

Additional unnecessary tracker recalculation on dependancy change #319

Closed robwheatley closed 3 years ago

robwheatley commented 3 years ago

Sorry, me again. On v2.2.2

I've noticed that useTracker will recalculate 2x on a dependancy change, but I assume it should only do it once?

Most basic thing I can do to reproduce (we're not even tracking anything here)...

This tracker is in a component that has a projectId passed into it as a prop. In real life, this tracker would be tracking changes in that projectId's collection and I would have the projectId as a dep.

Let's start off with no dependancies:

  const something = useTracker(()=> {
        console.log('tracker calculated');
        return null
    }
  )

Result: Every time I change the projectId prop, 'tracker calculated' is logged to the console. Just as you would expect.

If we now add an empty deps array:

  const something = useTracker(()=> {
        console.log('tracker calculated');
        return null
    }, []
  )

Result: Other that the 1st time the component is rendered, 'tracker calculated' is not logged on projectId prop change. Again, just as you would expect.

BUT, if I add the projectId as a dep:

  const something = useTracker(()=> {
        console.log('tracker calculated');
        return null
    }, [props.projectId]
  )

Result: Every time I change the projectId prop, 'tracker calculated' is logged twice, which in turn renders the component twice.

CaptainN commented 3 years ago

It's doing that on purpose. Basically, the way React and useEffect works, you really shouldn't create side-effects in render. That creates a problem for reactive patterns like the one Meteor/Tracker and others like Mobx use, where they both access data and set up subscriptions (or computations) on first render. React really wants subscriptions and data access decoupled.

So in order to support React properly, useTracker does its thing in 2 stages. In first-render (or first-render after deps change), it runs the query and then stops whatever computation immediately (and in concurrent mode might do so 4 times). Then once the component mounts, it runs useEffect where side effects are allowed, and sets up the permanent reactivity (until unmount or deps change).

In 2.0 and 2.1 we used to jump through copious hoops to retain the initial renders "side-effect" its active computation, but it's was super hard to get right, and super hard to maintain - and it had edge cases where the behavior would be quite different, and a few bugs with how Subscriptions work. So it was changed to be much more of a clean React implementation, for better or for worse.

It might be possible to recycle computations that are created in render after deps changes, but we'll still have the issue on first-render. Actually, useTracker without deps already works this way! The tradeoff is that it always runs your reactive function in render in every render. I'm not sure it's a good idea though. Concurrent mode only seems to do the concurrency thing on first render, not re-renders, even when deps change. But Who knows what the React team may do in the future.

robwheatley commented 3 years ago

Roger that. Thanks for the detailed feedback, much appreciated.