Greetings all, I have familiarized myself with the react-meteor-data package and its shortcomings. I just wanted to put forth a useTracker that I quickly developed, and perhaps someone could code review it and tell me why it wasn’t done this way to start with.
The point of this useTracker is to remove the “double” execution on first render. I noticed that when I console.log inside a useTracker, it happens twice, and upon reviewing the package I saw that it was because internally there was a useEffect and useMemo both calling the passed function, therefore the approach used necessitated two executions in succession upon the first render. So, I went ahead and made a useTracker that only executed once.
This useTracker is in production on my React Native app https://hotspotnyc.com (the web UI uses the official useTracker and the app uses useTracker2)
Have your run your version through the test suite at react-packages? The tests are pretty extensive and would likely turn up some different behaviours in your approach.
Off the cuff, your update() call inside the useMemo is breaking a React rule by mutating state during the render. See the React rules here.
I think the natural React way to do async state updates is to put them in useEffect. Doing only this would delay Meteor data updates until after other changes are rendered. So the useTracker hook includes an extra call of the reactive function to get the data synchronously during any render with new dependencies.
I also thought that there should be a useEffect, that is usually how all data fetching is triggered in react, after all. useEffect is used in conjunction with useState, the state being initially empty, then asynchronously filled once data is ready.
But Tracker.autorun is too smart. It has the data immediately. When placing a Tracker.autorun inside of a useEffect, you have missed the window of opportunity to get your data included in the first render. So to make use of the syncronous nature of Tracker.autorun, we must make sure the reactive function is called within the same event loop as the render, hence why I moved it to a useMemo.
The main problem with that approach is that the cleanup function in useEffect won’t run in concurrent mode for every useMemo - that’s why useMemo has no similar utility. If you use this hook with time slicing features (error boundaries, suspense and concurrent mode, now standard in React 18) it will leak like crazy.
There are also a lot of edge cases with timings you’ll run in to in cases where you’ve got deps changing, but the old computation is still running after useMemo, but before useEffect's cleanup runs, and things like that (I don’t think useEffect's cleanup is guaranteed to run before the next render when deps change, but I could be wrong).
Anyway, the rework I did recently to make useTracker work with React 18 actually does look a bit like that, with a few edge cases taken care of.
The biggest headache with all this is actually not Tracker, but Meteor.subscribe - that makes a bunch of assumptions about its own lifecycle which matches the old Blaze implementation. It’s been a total pain to support that, with the old tests. Honestly, it might be time to think about rewriting Meteor.subscribe, or creating a deeply integrated alternative. It’s getting extremely difficult to support it.
In general you cannot have any side effects inside a useMemo. Assigning refs.comp or stopping the computation is a side-effect.
As @captainn said, you may only start noticing problems with this implementation once time slicing features of React kick in.
PS: You cannot have any side effects inside a useMemo the same way you cannot have any side effects inside the render function of a component. In the current source of useTracker there are some cases where refs get assigned/unassigned in the body of a hook, for instance here, will this cause a problem in the future @captainn?
PPS: Calling Tracker.autorun is also a side-effect it seems as it consumes some global resources that need manual clean up.
PPPS: If not in the general case, you may think it is a good idea to workaround the rules of React because your special case looks like it would work without them. But in that special case I think you should consider whether the added maintenance burden is worth the feature. In the case of useTracker I have never had a problem due to this double-rendering behavior. Did you?
There is no real good way to avoid creating side effects in render when using tracker, so useTracker goes through a number of checks to make sure we are cleaning up whatever side effects are created immediately.
There may be some edge cases where the computations get lost actually, particularly with error boundaries (like, if the component is mounted, and then some error is throw in other hook before useTracker gets to run in subsequent renders - we should probably write a unit test for that case - I did just think of a way to solve for that, but in previous attempts it didn’t work - things may have changed in React 18). But generally, the hook does a good job of cleaning up it’s own in-render side effects, and 99% of the time, you really only have to worry about creating side-effects in the first render.
With the no-deps hook implementation, we are also taking advantage of the algorithm we are using - it only ever creates a computation during render (or immediately after the first render, and mount). When the computation detects an update, it triggers re-render, then invalidates the previous computation at that time (we should try invalidating the computation within the computation handler - this is the idea mentioned above. I do recall this didn’t work in previous iterations, but I need to make changes for React 18 anyway - I’ll give this a shot).
Anyway, it’s complicated, and annoying to make all this work. That’s one of the reasons we created useFind and useSubscribe, so we could iterate a little closer to the metal. We should consider reworking some of the core APIs to make this even simpler, especially with useSubscribe. useTracker is honestly, too hard to maintain (part of that is TinyTest - we really need a more modern, more reliable, and more documented, testing framework).
The challenges with TinyTest are actually why the coming React 18 patches are stalled. I need to find time to make them work again, as they are extremely important to protect against some edge case regression, particularly where subscribe is involved.
This has gotten so deep, thank you for your time and for the excellent discussion
The problem I experienced was strictly a performance issue. We had a long list and I found that speeding up useTracker was a quick win throughout the app, but after reading this it might appear that useTracker is not even the way of the future
A HUGE PROBLEM…
So, there is this hard rule that there shall be no side effect in a useMemo…And this is because the body of a useMemo function may look like a callback, but it is still executing in the same event loop as the render itself, at least the first execution. If you were to unwrap the body from the useMemo, you’d see the side effects clear as day. Would you stop a computation in a render function? Yeah, that looks ugly. Now I can see why react says no side effect in renders.
BUT THEN AGAIN…
Knowing what we know about meteor, the body of the Tracker.autorun is executed once in the same event loop as the render and this fact enables us to have a result in time for the first render. In a subsequent event loop, the body of the Tracker.autorun is called, but never the same one as the initial render. This prevents a render from triggering another render. Calling Tracker.autorun inside a render is akin to setting up a listener, which to me sounds a lot less sketchy than a full blown side effect, since it is strictly observing.
Now if we remove some assumptions about react like that it renders each component in the same event loop, synchronously, then we start to get some problems, but I am not following that development as closely as I should.
I realize it might sound like I am defending my useTracker, but I swear I just want to serve the community by getting a greater understanding. At the time my useTracker is the only choice for this project I am working on due to performance. Perhaps my useTracker is useful now and will be ruined when react makes its next big innovation. At the very least I would like to confirm that my solution works for the time being even if react is going to change and make this solution not work.
Oh, and none of this might matter, because by that time perhaps we won’t need useTracker at all — @captainn could a developer like me feasibly make use of useFind and useSubscription and avoid useTracker and its double execution altogether?
I personally think useFind is easier to use for higher performance scenarios than useTracker. It gets you a reference stable list of items from your find query, and you can use that to memoize the items in your rendered list. If you add a page of 10 items, to a list of 500, react will only have to render the new 10, the others will not rerender. If a single document updates, only that 1 item will need to rerender. The reconciler will be able to skip the rest, because of the reference stability.
useSubscription is currently just a thin wrapper around useTracker but that may change.
Not to hammer too hard, but adding an observer with Tracker.autorun is definitely a side effect - it’s the main thing the react team means when they say “no side effects”. Doing that without a reliable way to clean things up (and useEffect is not reliable, because it doesn’t always run with ever render) would produce multiple errant observers (listeners) that will never be cleaned up, in concurrent mode, or when other time slicing features are used.
