Calling flush in an event handler leads to bad ordering of reactive reruns

[tmeasday]

Uggggghhhh.

Repro: https://github.com/tmeasday/react-flush-problem

1. Click the button.
   • Notice that the error is logged to the console.
   • This means the flush cycle finished without getMeteorData() rerunning (yet, it runs later)
   • This is bad -- usually it means subscriptions needlessly re-run, thanks to the default subscription re-use behaviour not happening.
2. Try again, running .go() in the browser console.
   • Notice there is no problem.

Note that FlowRouter calls Tracker.flush() at a certain point in it's .go() method, so this is far from a strange situation.

I don't have an explanation as to what's going on at this point but I'm sure it's something to do with the slightly zany way we make the computation re-create itself.

[tmeasday]

So the fundamental reason for this is quite simple. When you call forceUpdate it doesn't execute immediately, instead it adds it to the end of the batched update queue and ensures the queue is running.

1. In the console case, there is no queue, which means that update runs right away.
2. In the event handler case, there is (the onClick is in it), so it waits for the onClick to finish before running the update.

So this seems like a fundamental problem with the current approach. I think to summarise, the aim of the current approach is to make sure that when the getMeteorData computation invalidates, rather than it acting like an autorun, instead the component re-renders so that we get a componentWillUpdate followed by getMeteorData running.

If instead we simply allowed the computation to re-run, we'd see getMeteorData re-running, then after we would need to force-update the component with the new data somehow (without running getMeteorData again I suppose) -- which would change the semantics of the mixin; for instance componentWillUpdate would run after the getMeteorData call.

Fundamentally I can't really see a good solution to the problem. If someone calls Tracker.flush() in an event handler (not an unusual pattern, we do it in an example app after all!), then we are guaranteed that the flush will happen before any React updates occur.

[dgreensp]

Interesting.

It's possible to mess with React's built-in batching (see e.g.), but I think the fundamental issue is expecting React to complete its rendering synchronously. Tracker.flush() is probably a bad idea, even though it's been a Meteor pattern since the beginning. A UI framework should always be able to defer rendering, for example doing it in a requestAnimationFrame, and if logic needs to run afterwards, it can be done with some kind of callback.

It originally seemed reasonable that Tracker.flush() would flush the UI because we only had our own UI library to think of, and we may have even thought that other reactive UI libraries would naturally build on top of Tracker. Since React has its own queue, the app would have to flush that too, with something like ReactDOM.flush(), which is (probably) not going to be a thing that exists.

[tmeasday]

So is the conclusion here "Tracker.flush() is actually not a good idea"?

If so, I think we should stop doing it in our example app (certainly we can use Tracker.afterFlush() instead). And we should prod @arunoda to use afterFlush in flow router also (etc).

[arunoda]

Thanks Tim. There is a case where I can't use afyerFlish and need to do it immediately. I did it because there is no option for me. I will remove it in the next version as I stop using a computation inside.

[dgreensp]

Sounds good to me

[filipenevola]

I'm closing this just because it's too old. We can open new issues for items that are still valid.