Create useTracker hook and reimplement withTracker HOC on top of it

[CaptainN]

This PR is a roundup of the discussion in #262, and supersedes: #242, #252, #256, #263, #266, #267, #268.

It does the following:

• Create a base hook, useTracker to for use as an alternative to withTracker in hooks based projects.
• Rewrites withTracker on top of useTracker, which sures up withTracker's compatibility with React strict mode and the forthcoming Suspense and concurrent mode (issues Unsafe lifecycle methods were found within a strict-mode tree #256, React 16.3 => migration from componentWillMount and componentWillUpdate #252, componentWillMount vs componentDidMount #242 / PR [WIP] Update lifecycle methods for newer React #261)
• Updates docs!
• Removes deprecated createContainer.
• Above Expose useTracker hook and reimplement withTracker HOC on top of it #262, this PR retains the synchronous rendering behavior in withTracker and in useTracker if no deps are specified, but also allows asynchronous reactivity after firstRun if deps are supplied, and properly responds to deps changes.

There were not any unit tests, but I may write some if I have time. I'd especially like to test some of the lifecycle stuff, and make sure reactivity persists. I don't think that should hold up a beta release.

Speaking of release - since this bumps the react version requirement to 16.8, there has been some discussion about how to properly release this with a major version bump. One suggestion was to bump the old version to 1.0.0 which can receive updates separately, then release this update as 2.0.0.

CC @hwillson @menelike @yched

[CaptainN]

@menelike I think you have a different problem (described in the other issue), where you want to control the computation. This new question is about whether it's worth it to expose its life cycle. I commented on the other issue to continue that conversation there.

[CaptainN]

Looking closer at React Suspense, it looks like it's mostly an async loading paradigm, which is not at all how Meteor works. To properly support that we'd probably want to do some more radical design changes to this hook (or creating another one). For example, a useSubscription hook could throw a promise, then when it comes back the following useTracker hook could run.

The problem comes if someone uses a Suspense feature along side (or after) our useTracker implementation - eg, they throw a promise after useTracker.

I wonder if in the short term it's enough to explain that useTracker should come after Suspense hooks. The example here, shows that the mock hook (useInitialEffect) works fine after the thrown promise.

The ideal solution is for us to somehow get a safe way to create or clean up the side-effect we might create before a promise is thrown. Otherwise we have to accept an inefficient default. I explained it fairly clearly in the React ticket on the subject.

[CaptainN]

@yched Someone in the react ticket I opened suggested creating a custom Suspense component, to help us out with making our solution work with Suspense - I like the idea. What do you think?

We could do a custom TrackerSuspense or even something generic, and just put it on npm. He suggested two different models. (I'm still hoping React team will bundle something into react itself.)

[CaptainN]

@yched This is really a worm in my brain. I had another idea - what if we just use a timeout between the synchronous portion of the hook, and the useEffect portion.

1. Synchronously, we start the computation as we do now, and start a timeout, with the cleanup method.
2. Then in useEffect we'd check to make sure to clear the timeout, and if the computation is still there, continue on - if not, rebuild it and rerender (like your original PR).
3. For reactive events, we could check the computation against the refs.computation to make sure that it's still valid (we could even dispose at that point).

Would that solve it? We would just need to set the timeout to some appropriate length - but it would probably not have to be very long. Maybe 50ms?

[yched]

@CaptainN heh - that sounds a bit acrobatic, but right now I don't see where that would fail, actually... A timeout introduces another level of asynchronicity that comes with its own potential for interesting race conditions :-), but that definitely looks promising.

The penalty would be, if Suspense holds our component for more than (say) 50ms, then we'll stop the computation & subscriptions that were initiated, and will just start them again when it's actually mounted, that sounds acceptable.

Not sure what would happen for reactive events triggered between the initial render and the actual mount, though. We wouldn't want them to trigger a rerender before useEffect runs and ensure our render was committed ?

[CaptainN]

I'm out of time to play with it - but it might be possible to detect if the react component has been discarded or not from within the reactive function when the timeout happens - I'm not sure how to do that though.

[yched]

Another thing that I've been thinking lately (speak af a brain worm...) : maybe having a version of just the withTracker HOC that is fully future-compatible with React Suspense - i.e. no componentWillXxx / no side effects in render / no double render - could be fairly simple :

withTracker() is easier because a) we know the reactive func returns an object of props, and b) it wraps a component as a whole. So we could :

• have an internal useTrackerInternalForHOC hook (made up name of course) that just returns null on sync renders (no monkey-patching), and does Tracker.autorun() / setState() in use(Layout)Effect()
• have the wrapping component render nothing if the hook returns null, or the wrapped component with the returned props otherwise.

That's some handwaving here, and I'd need to actually try it, but if it works it could give us some leeway to release an iso-API version of react-meteor-data to make existing apps ready for the deprecations in React 16.8.7 and Suspense in 16.9 (which remains the most urgent goal IMO), and give us time to figure out the more generic useTracker() hook if it's not too clear yet how it would work with Suspense ?

[CaptainN]

I actually got a version of this working - it's simpler than I thought, but there may be a race condition. It does this:

1. Set isSuspended to true by default in useRef
2. In useEffect set refs.isSuspended to false. If useEffect ran, the contract says its cleanup will run too.
3. In a timeout, see if refs.isSuspended is true - if so, dispose.

The main question I have (the race condition) is how long is the right amount of time to wait in setTimeout? Is there some guarantee of how long React will take before it executes useEffect? Another question is, is there another way to tell whether the component has been discarded?

I still prefer the current approach, because it works today, and even with Suspense, it should work in 99% of cases. It would be a shame to hobble performance and complicate the API for a chance Suspense messes with it. I'd still prefer some kind of fix or workaround.

Another thing I'm not sure about is how concurrency plays in to all this. Can a render's useEffect hook be cancelled with concurrency just like they are for Suspense? I don't know the answer to that.

[CaptainN]

We could probably just offer a second suspense compatible version - useHighwireTracker - it would be relatively easier to build that. Just use useEffect and pass in deps, and return null for the initial render.

BTW, I merged the useMemo version and the computation life-cycle stuff into a new branch, and I'm think to make a PR out of that: https://github.com/CaptainN/react-packages/tree/hooks-plus-ultra

[CaptainN]

As far as I can tell, the problem with Suspense is isolated to one specific scenario, one that I think withTracker cannot even trigger, and is easily avoidable with useTracker. That scenario is if you use the hook before a thrown promise in the hook order. If you use the hook after all possible thrown promises (which would always be the case for withTracker components, since they are not instantiated typically until the return statement) then you can't trigger the problem - it works fine. If you avoid using useTracker along side possible throws altogether (and use nested components), then you just don't have a problem. Am I missing another problematic scenario?

BTW, I think react-cache may have something of a solution to the problem of cleaning up in concurrency mode, but it's not stable yet (neither is concurrency mode).

[CaptainN]

Not sure what would happen for reactive events triggered between the initial render and the actual mount, though. We wouldn't want them to trigger a rerender before useEffect runs and ensure our render was committed ?

@yched Sorry, I didn't see this reply until now. Yes, that is the one gotcha I noticed as well. I think we'd have to accept a gap in service, so to speak in that case, but I think this workaround will do the trick for 99.9% of cases, even without a service gap:

1. In useMemo set up computation, and timeout@50ms, also set unmounted flag
2. reactive updates that happen before useEffect run reactiveFn, but do not try to re-render (do not set state). Also set a deferred render flag - in other words, if a reactive update happens between initial computation creation, and useEffect, we'll do a deferred render in useEffect.
3. If the timeout happens, and the computation is disposed, reactive updates will not trigger the reactiveFn. In most cases that will not be a problem, but we should document it.
4. In useEffect, cancel timeout, and set mounted flag. If deferred render flag is set, force render. If timeout timed out, restart computation, and do another render. This will run the computation, and get us back up to date with the current state of the reactiveFn.

Another thing I'd like to explore is what happens if you do try to update state in the computation from a dead end react component - does it throw errors or anything we can catch? Then we wouldn't even need a timeout (a quick test didn't reveal anything, but maybe there is something we can latch on to).

[yched]

Posting this here so as not to pollute the other PR too early - look what recently landed as a (for now private) standalone package in the official react repo :

https://github.com/facebook/react/blob/master/packages/use-subscription/README.md

If you read the discussion in the associated PR facebook/react#15022, it's almost laughable how they hit the exact same pain points as we did :-D. Interestingly, they admit they're not able to fully optimize in concurrent mode at the moment.

Their hook over there is designed for an API with clearly separated "subscribe to something" and "receive a value from that subscription" steps, whereas Tracker.autorun mingles the two, so not sure we'll be able to use it directly. But at the very least the code and discussion there are super relevant for us here...

If need be, it could make sense to reach out to the PR author at some point to discuss the specifics of our case, since he now has a pretty good vision of what is or isn't possible at the moment or in the future with concurrent mode.

[CaptainN]

I figured out while writing up a comment on React's useSubscription that actually useMemo with a cleanup method wouldn't work, if it doesn't have a semantic guarantee, because that could stop the computation and prevent some updates from applying. Bummer.

I'm not sure React's own useSubscription is going to hold up - wouldn't useMemo's lack of a semantic guarantee cause some subscription thrashing? I'll have to look more carefully when we get back from vacation.

If not (if for example, my assumption that their lack of a "semantic guarantee" is talking about concurrent mode) then we might actually be able to use the lifecycle from that to make useTracker compatible with concurrent mode.

[CaptainN]

Reading through what they did, we can't use that. They made an assumption that subscribe is "passive" - which allowed them to avoid setting up any side-effects during render. I think they also assume the side-effects they set up in useEffect will not trigger another render until some later time, or they are simply willing to accept the second render.

[yched]

Yup, came to the same conclusion, sadly :-/

They split between a "getCurrentValue()" callback that is supposed to be side-effects-free, and a "subscribe()" callback were side-effects occur, so they can can just call getCurrentValue() when needed to get the "sync value for current render". That's not true in our case, the function that gets the value also performs side effects - that's where I monkey-patched DDP subscriptions in my original PR...

So, right, their useSubscription is not for us it seems.

[yched]

Also - for the timeout-based approach, you wrote in #271 (comment)

reactive updates that happen before useEffect run reactiveFn, but do not try to re-render (do not set state). Also set a deferred render flag - in other words, if a reactive update happens between initial computation creation, and useEffect, we'll do a deferred render in useEffect.

Yup, apparently this is exactly what useSubscription refer to when they write "it achieves correctness by sometimes de-opting to synchronous mode" in their README : catching up with missed value changes that happened between initital render and the actual subscription at effect-time, by forcing a re-render directly from within the execution of useEffect() - apparently those renders are treated synchronously. That defeats the benefits of concurrent rendering when that happens, but since it is acceptable for their use case, that should be acceptable for us as well :-)

[CaptainN]

So one other thing we should look in to is how to handle error scenarios (error boundaries). I'm concerned that when an error it's thrown we can end up with the same sorts of cases where things are leaking memory and computations are not cleaned up.

[CaptainN]

@yched @menelike I think I got a version that satisfies all the requirements. Take a look.

The only part I'm struggling with now is writing some unit tests for the edge cases of concurrent mode (I'm not sure how to get the test thingy to wait for the thrown promise to resolve in my simulated Suspense rigging, but it seems to actually work), here's what it's doing:

1. On first render (unmounted) it will create a timeout@50ms, which will dispose the computation.
2. In useEffect it will cancel the timeout. If the timeout has no occurred, it will simply continue using the computation.
3. It will not run reactiveFn at all if the component is not mounted, except on firstRun, synchronously. This is to reduce overhead for discarded renders, and to keep consistent with the aims of the react team's push for efficiency. It basically only runs the computation once per discarded render, and memory/resources are (essentially) garbage collected after 50ms, outside the main render cycle.
4. If a reactive update does sneak in between first render and useEffect, a deferred render flag is set.
5. Also in useEffect, it checks for any deferred renders, which should have been triggered by reactive changes, which happened between first render, and useEffect.
6. In useEffect we also check to make sure the computation has not been discarded - if it has, it starts up a new computation and forces re-render.

After chatting with Brian Vaughn in facebook/react#15022 it became clear that this is an important area to work around in multiple cases - suspense, concurrent mode and other memory optimizations, and error boundaries. Suspense and error boundaries are shipping in some form or another today. What I've basically set up is a poor man's garbage collector based on setTimeout.

Once I've gotten the TinyTests to work the way I want, and have updated the readme to explain all this (including a section on Concurrent mode and avoiding side-effects in the reactiveFn) I'll merge this with PR #273.

[CaptainN]

So, the way useEffect works is different from componentDidMount - the former yields to allow the browser to paint, and the latter runs synchronously. It would actually be easier to build all this, with a shorter timeout period using componentDidMount because of this, maybe even just 1 millisecond. I don't think it's worth rewriting the withTracker component just for that more simple implementation of the same algorithm, but it's an option worth knowing about.

Another thing I thought about is, can we leverage some other browser APIs such as requestAnimationFrame to get a faster answer about whether to cleanup (faster than a timeout of 50ms)? I probably won't play with this much, since it would require digging in to react internals, or writing a bunch of tests (or both), but I thought of it, and wanted to document it somewhere.