Expose useTracker hook and reimplement withTracker HOC on top of it #262
Conversation
|
@yched: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
|
Hi there, just now as I submitted my version of react meteor hooks (#263) I saw yours! I separated two hooks one for subscription and one for data. I would love to get some feedback on your and mine solution. How do you handle stopping a subscription when a component is unmounted? As far as I understand, the returned function of Your solution for the first render looks promising. Maybe one could do the same but cache the handles in an array to stop all subscriptions in a Cheers 👍 |
I think offering various focused hooks like useSubscription(name, ...args), useDocument(collection, selector, options), useReactiveVar(var)... could be worthwhile, especially as it encourages splitting into granular reactive functions that only rerun when their dep gets invalidated, rather than a single big one which reruns as a whole when any of its deps gets invalidated. But as I outlined in the OP, I do see a lot of value in providing a useTracker hook that acts like the current withTracker HOC does, i.e. accepting a function that can mix subscriptions and data : it lets us reimplement withTracker on top of it (as the PR does), which instantly makes all existing Meteor/React apps compatible with React's soon-to-be-released "concurrent mode" (Fiber, Async, Suspense, the name has changed a few times), Otherwise, you have to rewrite your whole app to use "functional components + the new hooks" instead of "classes + withTracker" to be compatible, which would be a huge pain. Then the more specialized ones (useSubscription, useDocument, useReactiveVar...), are just a couple one-line wrappers around the generic useTracker. (Also, hooks can't be inside if()s, meaning a hook like
Yes, stopping the reactive computation on cleanup automatically stops the subscriptions that it made.
Yep, I have written just that actually, didn't find the time to push yet :-) Will try do so soon. |
|
So yes, the challenge here is :
Last commit solves that by having the Meteor.subscribe stub collect the subscriptions attempted at mount time. We then actually do those subscriptions manually in useEffect (and manually register them to stop when the Tracker.autorun computation is stopped). When the dependencies change, useEffect recreates a new Tracker.autorun computation and no special handling of subscriptions is needed there. |
|
Adjusted the "defer subscriptions from mount time to didMount time" part :
In short, I guess I need feedback from Tracker / DDP experts here :-) |
|
Well, come to think of it: the thing about "not firing Meteor.subscribe() at mount time because we wouldn't be able to stop the subscriptions if the mount was cancelled / restarted later by React concurrent mode", also applies to Tracker.autorun computations : we wouldn't be able to stop them if the mount is canceled. A Tracker computation is a kind of subscription (the general notion of subscription, not Meteor.subscribe specifically) : something that you setup at some point and need to stop/cleanup to avoid memory leaks. React concurrent mode says "no subscription at mount time, only at didMoiunt / didUpdate with useEffect()", that applies to Tracker.autorun too. So I guess the approach before the last two commits was the right one :
AFAICT, that's also the approach other libraries that do "return the results of a function and update them over time as they change" (like react-redux with its Reverting to 46c243a, then. Simpler code, less awkward messing around with subscriptions... |
|
Whoever is in charge of this project - what are your thoughts on this method? I really love the syntax and am wondering if we would be better off spinning of a new package for this so that we can take advantage of it now (as opposed to just doing it as a “polyfill” duke in each project)? Tl; dr - package maintainers should chime in so we can stop wasting time on this pull request and have someone step up to create a new package. |
|
To be clear, I believe this is the right spot for the code to live (with documentation on meteor guide already). Maybe we create a package that can be the standin until hooks are out of RFC stage? |
|
Side note : I'll be away for the next two weeks and won't be working on this in the meantime. As far as I can tell, the current code works fine and looks stable to me. Still TBD : how exactly the package should deal with "stick with current implementation for React < [the 1st React version that officially ships hooks], use the hook-based implementation otherwise". Maybe just a new major release that requires a minimal React version ? But yeah, on that question and on the code itself, feedback from the package maintainers is probably what's needed now. |
|
@jchristman : As for creating an actual, temporary/experimental package with the code from this PR so that people can actually start using it : why not I guess - at the moment I have no plans of doing that myself, but I'm perfectly fine with someone else taking the code and publishing a package though ;-) It should IMO just be very clear that it is a temporary package until react-meteor-data provides somithing similar, and that the code hasn't been vetted yet by the authors of the current React integration ? |
|
This looks really awesome! my only worry is createContainer/withTracker backwards compatibility. Are there any strong reasons why we can't leave them as is? |
|
@dburles: see above : the big gain is instantly making all existing apps compatible with React Suspense, by moving away from componentWillMount / componentWillUpdate (fixing #256, #252, #242, #261) FWIW, our app is pretty withTracker-intensive, and seems to work just fine with the drop-in hook-based reimplementation in this PR. |
|
@dburles, there’s also no reason that we couldn’t make a documentation note in README that says React <16 use [email protected]? |
|
Okay I agree, I think it's worth it. A note in the readme is also a good idea. @hwillson any thoughts here? |
|
I like it! We should do some additional testing though, so when this PR is ready to be merged, we'll cut a beta for people to try out. @yched Let us know when you think this PR is ready for review. Thanks! |
|
I think the PR is ready for review at the moment. You can read the comments above for details about another implementation I tried and abandoned (in short : React Suspense forbids Tracker.autorun / Meteor.subscribe at mount time) Still TBD : how exactly should the package deal with "stick with current implementation for React < 16.[the 1st version that officially ships hooks], use the hook-based implementation otherwise". Maybe just a new major release that requires a minimal React version ? |
|
Merge! :) |
|
Imho the benefits of this PR in conjunction with Also once generally approved https://github.com/meteor/react-packages/pull/262/files#diff-f54656ab7cd59d21708df27a3c521da5R21 should be solved ;) |
| }); | ||
| } | ||
|
|
||
| return <Component {...{ ...props, ...data }} />; |
There was a problem hiding this comment.
Just out of curiosity, is there a special reason against {...props} {...data}?
There was a problem hiding this comment.
changed to {...props} {...data}
|
I think these changes actually improved my app performance (the updated one, which is probably not visible in email for those following along there). I guess this makes sense, since it's now only setting up a computation once, on render, instead of on render, and then again after render. It is also not rendering twice anymore, which it would have been doing before since a state is set in the So we got a more responsive implementation (re-calculates immediately when deps are changed, instead of waiting), and also improved performance! I think this is still compatible with upcoming Suspense API, but I'm not sure. I wrapped the app in |
|
I think we're heading into the right direction. Your code results in two render calls on mount (in my example) which I can't observe with the current Today I tried to replicate the current function useTracker(reactiveFn, deps) {
const previousDeps = useRef();
const computation = useRef();
const trackerData = useRef();
const [, forceUpdate] = useState();
const dispose = () => {
if (computation.current) {
computation.current.stop();
computation.current = null;
}
};
// this is called at componentWillMount and componentWillUpdate equally
// simulates a synchronous useEffect, as a replacement for calculateData()
// if prevDeps or deps are not set shallowEqualArray always returns false
if (!shallowEqualArray(previousDeps.current, deps)) {
dispose();
// Todo
// if (Meteor.isServer) {
// return component.getMeteorData();
// }
// 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.
computation.current = Tracker.nonreactive(() => (
Tracker.autorun((c) => {
if (c.firstRun) {
// Todo do we need a try finally block?
const data = reactiveFn();
Meteor.isDevelopment && checkCursor(data);
// don't recreate the computation if no deps have changed
previousDeps.current = deps;
trackerData.current = data;
} else {
// make sure that shallowEqualArray returns false
previousDeps.current = Math.random();
// Stop this computation instead of using the re-run.
// We use a brand-new autorun for each call to getMeteorData
// to capture dependencies on any reactive data sources that
// are accessed. The reason we can't use a single autorun
// for the lifetime of the component is that Tracker only
// re-runs autoruns at flush time, while we need to be able to
// re-call getMeteorData synchronously whenever we want, e.g.
// from componentWillUpdate.
c.stop();
// trigger a re-render
// Calling forceUpdate() triggers componentWillUpdate which
// recalculates getMeteorData() and re-renders the component.
forceUpdate(Math.random());
}
})
));
}
// replaces this._meteorDataManager.dispose(); on componentWillUnmount
useEffect(() => dispose, []);
return trackerData.current;
}I only added In withTracker I added props as UpdateMaybe it's even better to pass no deps at all in conjunction with From my observations (this stuff is hard to debug) this code works 100% the same as After all I think that the current code from this PR introduces some performance issues as you've mentioned earlier (it's like managing state only with What do you think? |
|
Note: One thing I still need to determine is if Update I found ithttps://github.com/facebook/react/blob/master/packages/shared/objectIs.js I don't understand all reasons behind reacts implementation (for example, why continue in the for loop and no early exit!?) I'd propose: function is(x, y) {
return (
(x === y && (x !== 0 || 1 / x === 1 / y)) || (x !== x && y !== y) // eslint-disable-line no-self-compare
);
}
function shallowEqualArray(nextDeps, prevDeps) {
if (!prevDeps || !nextDeps) {
return false;
}
const len = nextDeps.length;
if (prevDeps.length !== len) {
return false;
}
for (let i = 0; i < len; i++) {
if (!is(nextDeps[i], prevDeps[i])) {
return false;
}
}
return true;
} |
|
Oh right! I added a I'd like to re-invent the wheel a little bit. The original Here's what I'm thinking (very close to your last implementation): function is(x, y) {
return (
(x === y && (x !== 0 || 1 / x === 1 / y)) || (x !== x && y !== y) // eslint-disable-line no-self-compare
);
}
function shallowEqualArray(nextDeps, prevDeps) {
if (!prevDeps || !nextDeps) {
return false;
}
const len = nextDeps.length;
if (prevDeps.length !== len) {
return false;
}
for (let i = 0; i < len; i++) {
if (!is(nextDeps[i], prevDeps[i])) {
return false;
}
}
return true;
}
function useTracker(reactiveFn, deps) {
const computation = useRef();
const previousDeps = useRef();
const trackerData = useRef();
// if prevDeps or deps are not set or falsy shallowEqualArray always returns false
if (!shallowEqualArray(previousDeps.current, deps)) {
previousDeps.current = deps;
// Stop the previous computation
if (computation.current) {
computation.current.stop();
}
// Note : we always run the reactiveFn in Tracker.nonreactive in case
// we are already 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 too.
computation.current = Tracker.nonreactive(() => (
Tracker.autorun((c) => {
trackerData.current = reactiveFn();
if (Meteor.isDevelopment) checkCursor(trackerData.current);
})
));
}
// Stop the last computation on unmount
useEffect(() => () => computation.current.stop(), []);
return trackerData.current;
}And then export const withTracker = options => Component => {
const expandedOptions = typeof options === 'function' ? { getMeteorData: options } : options
const { getMeteorData, pure = true } = expandedOptions
function WithTracker(props) {
const data = useTracker(() => getMeteorData(props) || {})
return data ? <Component {...props} {...data} /> : null
}
return pure ? memo(WithTracker) : WithTracker
}This should get us the benefits of the hooks lifecycle (or whatever it's called) while retaining the old behavior for |
|
I agree that this small piece of code should be as performant as it can get, it will be used several times in projects and might make the difference between a fast and slow meteor/react app. I must miss something but I don't see any crucial changes to your example code, but I agree that In short:
So the only place where a recomputation is wasted is when a reactive value changes over time and after the first run of the computation (!computation.firstRun). All my attempts to get rid of this failed, and I am not really sure if there is any performance gain to expect (after all, if deps change we MUST recreate the computation anyway?). |
|
Actually, I'm questioning why this is here: } else {
// make sure that shallowEqualArray returns false
previousDeps.current = Math.random();
// Stop this computation instead of using the re-run.
// We use a brand-new autorun for each call to getMeteorData
// to capture dependencies on any reactive data sources that
// are accessed. The reason we can't use a single autorun
// for the lifetime of the component is that Tracker only
// re-runs autoruns at flush time, while we need to be able to
// re-call getMeteorData synchronously whenever we want, e.g.
// from componentWillUpdate.
c.stop();
// trigger a re-render
// Calling forceUpdate() triggers componentWillUpdate which
// recalculates getMeteorData() and re-renders the component.
forceUpdate(Math.random());
}It says something about "re-runs" but what does that actually mean? Is it not enough to use the memoized value from the last reactive run? Why does it spend an entire additional render cycle each time the computation is re-built (which in the case of @menelike Do you know why it does this? It seems like we can get rid of that entire extra cycle, no? |
|
@CaptainN Yes I agree, this is exactly the part I wanted to get rid of in the past but failed. I faced async related issues. My idea was to call the reactive function, save it to ref.current and force an update. On the other hand I also think that re-creating a computation is not a heavy task, the most impact is on The comment and logic you mentioned is actually taken from https://github.com/meteor/react-packages/blob/devel/packages/react-meteor-data/ReactMeteorData.jsx#L68-L79 |
|
Oh! I got it. That's actually the part that causes a re-render when the tracker data changes from outside the render method (duh). My solution would not work, given that. Using the old code is perfect (now that I understand it.) Then here is some feedback for your PR - there is a duplicated effort - you don't need this, because it's already been handled at the bottom of the js file in the export statement. // When rendering on the server, we don't want to use the Tracker.
// We only do the first rendering on the server so we can get the data right away
if (Meteor.isServer) {
return reactiveFn();
}I'd probably use an incrementing value instead of a random value for force update - it's entirely possible, if improbable, that you get the same value from two separate Other than that, it looks golden! Nice job. |
fixed by yched@365584f
fixed by yched@c3803b9
Thank you very much and thanks a lot for this very productive collaboration. Actually this also helped me understand react hooks much better. |
|
Yeah, this was fun - I learned a ton about Hooks, and even some of the upcoming stuff like Suspense, and StrictMode, ConcurrentMode (which makes my app SO SMOOTH). Super great learning time, and I think it's a real improvement to the product. |
|
@menelike After sitting with this for a while, and doing some performance testing in my app, there is still one thing I don't quite understand. Why do we need to run the computation synchronously for every single change - what does this comment mean? // We use a brand-new autorun for each call
// to capture dependencies on any reactive data sources that
// are accessed. The reason we can't use a single autorun
// for the lifetime of the component is that Tracker only
// re-runs autoruns at flush time, while we need to be able to
// re-call the reactive function synchronously whenever we want, e.g.
// from next render.It would seem we should only have to re-build the autorun when the deps change, not every time, but I don't really understand what this comment means when it talks about re-runs and "flush time". I get why it should run synchronously for the first run, but why do we need it to run synchronously for every additional render? In my app, my computation is slow compared with my react render - almost 4 times slower (this probably indicates a problem with my queries, but still). It would be great to allow that to run asynchronously most of the time. |
|
Actually, I think I was reading that wrong. When I tried to verify that, being more careful to start from empty offline storage, etc., the graph looked a bit different. Reading it correctly showed me an error outside this component, which is pretty useful. I don't think my queries are that slow, but I did run a bunch more tests, and I can at least confirm that the synchronous version of useTracker is faster than the asynchronous (useEffect based) version - in my specific case, it's an average of around 30-50% faster. Pretty sweet! I would still like to know why its important to run the reactive function synchronously, as described in that comment. |
|
I have a couple of other thoughts, we might as well figure out before any of this ships (I guess it will at some point).
const myQueryFromProps = { _id: props.myDocumentId, public: true }
const myData = useTracker(() => {
const handle = Meteor.subscribe('myPublication', myQueryFromProps)
return myCollection.find(myQueryFromProps).fetch()
}, [myQueryFromProps])
const myQueryFromProps = useRef({ _id: null, public: true })
myQueryFromProps.current._id = props.myDocumentId
const myData = useTracker(() => {
const handle = Meteor.subscribe('myPublication', myQueryFromProps.current)
return myCollection.find(myQueryFromProps.current).fetch()
}, Object.values(myQueryFromProps.current))I guess this is more of a React thing than a Meteor thing, but is it reasonable to ask a Meteor user to do all this? In the React space, there is a react eslint "exhaustive-deps" package which would explicitly prohibit this kind of optimization, because they say it would lead to bugs. But I can't think of how you'd achieve the same level of of efficiency without it. There is a lengthy issue on this. (This example is contrived - if a user creates that myQueryFromProps object inside the I think the good news on this is, I don't think this is any less efficient than the old |
|
@boomfly Would you mind testing out the PR here? It's a fork of this PR with some additional work to make the initial render synchronous. The result is more immediate availability from the reactive function and 1 fewer re-renders (the current version here always renders at least twice). The PR @menelike spearheaded actually does a few nice things. If makes It's the best of both worlds! |
|
How do we get this (and this) merged and deployed? |
|
I just produced this neat package for maintaining state which survives HCP on top of I also figured out how to do unit tests for react hooks in there. Maybe we should add some of those for |
|
There has been a lot of conversation around this PR, so just to confirm - is this fully ready for review? |
|
@hwillson The PR we have over on @yched's fork (spearheaded by @menelike) is ready for review. Would it be simpler to create a new PR directly from that fork? That fork solves two important issues with the current PR:
|
|
@CaptainN Creating a new PR from that fork would be great - thanks! |
|
This is now superceded by #273, I should have closed this a while ago. |
As initially posted on forums.meteor, this is a first attempt at providing a useTracker hook to get reactive data in functionnal React components.
Example usage :
The PR also re-implements the existing withTracker HOC on top of it, which:
A couple questions remain open (more details in comments below, to follow soon), but this already seems to work fine in our real-world app heavily based on withTracker.