-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
react-meteor-data v2.2.1 - unnecessary renders #313
Comments
Subscriptions are a little bit of black magic in Meteor. One of the things that changed in 2.2.1 is that I removed a lot of code which was designed to keep the initial computation (a side effect), created during render alive after the component mounts. This was causing some rare edge case failures with Subscriptions, but it was also making the code base super complex, and difficult to maintain. All that was replaced with a system which runs and disposes of the computation on render (to avoid creating lingering side effects) and then reruns the computation on mount (in useEffect). That said, Meteor's subscription black magic should be retaining the data loaded in the initial subscribe call even though it subscribes again, without causing all that data to load a second time. It should be recycling the subscription, or at least the data in the subscription. If that's not happening, we may need to take another look at this. This does mean that it's rendering a second time, on every render. The React team has been stressing that extra renders aren't really that big of a deal. They should be quick and disposable, they say. The new hook is meant to honor this philosophy. It is annoying that it has to render a second time, but it's necessary to support things like concurrent mode properly. If you want something super extra optimized, a few of us have been working a pair of alternative hooks, which will provide a semi-immutable data source for for |
Thanks for the reply. I'll check out that PR. In the mean time, I have a little more info to add that may influence things a little.. Oh, and one thing I forgot to add, I'm also using React-Router (I seem to remember that gave you a little grief). In the code above, you can see that I'm using useTracker to get a value for projectsReady. Whilst that is false, I show a spinner. Once it becomes true, I load my app for real. ie, I don't want to load my app until the subscription is ready (actually a few subscriptions, but that doesn't matter for this example). A component lower down in the app is tracking the number of projects, like this:
In reality, the tracker is waaaay more complicated than this, but I tested this simple tracker and I got the same problem. I want this component to re-render if the count in Projects changes. Simple enough. However (and this was driving me insane) no matter how the projects collection changes, the count tracker wasn't triggering. I had a thought that this maybe due to the 'cancelled subscriptions' from the subscription useTracker (in my first post) firing too many times right at the very beginning. I got my first clue by removing the dependancies array in the projectCount tracker. I got my second clue by adding a dependancy that I changed elsewhere (which forced a recalculation of the tracker after the initial render). Both of these things worked (ie, I would be told of a count change). However, in my real app, I have a list of dependancies that don't change on initial page load, but are needed, so I couldn't use either of these 2 solutions. So, I did this: Specifically, I do this:
Given the state change and re-render, I could probably get away without a timer. I know that it still works at 20ms for instance. Tomorrow I might try removing the timer and see if the extra state change alone is enough. In my app, this 200ms delay is not a big deal. It will only happen once either on login or on a forced page re-load. Anyway, I thought I would mention it, as I'm likely not going to be alone with this problem. People may be scratching their heads wondering why their trackers are no longer tracking in certain circumstances... |
Thanks for the detailed response. I'm actually curious if any smaller value would work. 0ms, 1ms, or 2ms. This should create a reactive computation, regardless of what the subscription is doing, or whether it was mounted before or after the subscription:
That it's not, is concerning. |
If you could, please try this version of export const useTracker = (reactiveFn, deps) => {
let [data, setData] = useState();
useMemo(() => {
// To jive with the lifecycle interplay between Tracker/Subscribe, run the
// reactive function in a computation, then stop it, to force flush cycle.
const comp = Tracker.nonreactive(
() => Tracker.autorun((c) => {
if (c.firstRun) data = reactiveFn();
})
);
// To avoid creating side effects in render, stop the computation immediately
Meteor.defer(() => { comp.stop() });
if (Meteor.isDevelopment) {
checkCursor(data);
}
}, deps);
useEffect(() => {
// NOTE: Wrapping this in nonreactive to test an idea
const computation = Tracker.nonreactive(
Tracker.autorun((c) => {
setData(reactiveFn(c));
})
);
return () => {
computation.stop();
};
}, deps);
return data;
} I have one other idea if this doesn't solve it. |
Quick note, I deleted a comment I added earlier about things now working even without my hack. It's not as simple as that, so please ignore the mail you would have received. Here is the latest. I have tested different timers on my little 'delay' function. I can get the timer all the way down to 0ms, and things work. Meteor.setTimeout(()=>setOkToGo(true), 0) But if I remove the timer altogether (just set the state) then they don't work - my trackers tracking collections are not reactive. So, just the extra delay setting a 0ms timer is enough to 'solve' the problem, but not a state change on its own. As a note, this app is running on localhost. On the local useTracker code you provided. I gave it a go, but it complained about checkCursor not being defined - not knowing what I was doing, I copied that function from the original react-meteor-data file and put it in the local useTracker version, but now it's complaining about a type error (f is not a function, tracker line 603 being called from the useEffect in the code you provided). Clearly I need to do something different..... |
If you would, remove that check, and give it a try - it's only useful to provide development experience, but we don't need it for this test. export const useTracker = (reactiveFn, deps) => {
let [data, setData] = useState();
useMemo(() => {
// To jive with the lifecycle interplay between Tracker/Subscribe, run the
// reactive function in a computation, then stop it, to force flush cycle.
const comp = Tracker.nonreactive(
() => Tracker.autorun((c) => {
if (c.firstRun) data = reactiveFn();
})
);
// To avoid creating side effects in render, stop the computation immediately
Meteor.defer(() => { comp.stop() });
// if (Meteor.isDevelopment) {
// checkCursor(data);
// }
}, deps);
useEffect(() => {
// NOTE: Wrapping this in nonreactive to test an idea
const computation = Tracker.nonreactive(
Tracker.autorun((c) => {
setData(reactiveFn(c));
})
);
return () => {
computation.stop();
};
}, deps);
return data;
} |
With that check removed, I still get the same problem of:
local-useTracker is my file name for the file you provided, and my line 24 is this line from your code:
Whether this was the right thing to do or not, I changed your code to this:
And now things are working as expected, ie, I have reactivity back. No idea of the consequences of what I just did, and I've not got the time right now to look into it. I'll have more time tomorrow, but I guess that it's the same as running the original code with no dependancy array..? |
The consequences of that change would be that it'll never stop your computation on unmount (which is probably why the reactivity returned). I'm tracking a separate set of issues with the deps array not being applied correctly. I wonder if you are having a similar problem to that. Needless to say, I'm working on it. |
OK, thanks for the update - my day job has taken over at the moment, so I'm likely not going to get back to my app until the weekend. Perhaps I can add more then. |
Try this one when you get a chance const useTrackerWithDeps = (reactiveFn, deps) => {
let [data, setData] = useState();
const { current: refs } = useRef({ reactiveFn });
refs.reactiveFn = reactiveFn;
useMemo(() => {
// To jive with the lifecycle interplay between Tracker/Subscribe, run the
// reactive function in a computation, then stop it, to force flush cycle.
const comp = Tracker.nonreactive(
() => Tracker.autorun((c) => {
if (c.firstRun) data = refs.reactiveFn();
})
);
// To avoid creating side effects in render, stop the computation immediately
Meteor.defer(() => { comp.stop() });
}, deps);
useEffect(() => {
const computation = Tracker.nonreactive(
() => Tracker.autorun((c) => {
setData(refs.reactiveFn(c));
})
);
return () => {
computation.stop();
}
}, deps);
return data;
} |
@CaptainN Just did a quick test of the latest code you provided. That works. Reactivity restored. |
I've also discovered why the tracker was subscribing to the collections twice. It's nothing to do with useTracker, it's was me being an idiot! (I wrote a little function on the Route to check that the user was authenticated and that was forcing the creation of a new react element, hence 2 calls). So, the first part of my first post is no longer valid. Still hoping that some good will come of this thread though (in terms of helping get to the bottom of last reactivity) ;-) |
This issue should be fixed on 2.2.2 released just now. Thank you @CaptainN |
@CaptainN I'm running
With that component, when I click the button to increase the number, I can see the console output:
|
Perhaps I'm missing something here, but I'm having a spot of bother with v2.2.1. I'm new to react, Javascript and Meteor, so please don't bite!
I recently updated my project to Meteor 2.0. After seeing all the warnings in the logs from withTracker RE undefined deps array, I separately updated react-meteor-data to 2.2.1 as the warnings were due to a bug that was fixed in 2.2.1
I'm using useTracker to tell me when a Meteor subscription is ready, like this:
But when using Meteor Dev tools to watch DDP messages, I see that there are 2 'Subscribing to projects' going off to the server. I don't really see why as I have an empty dependancy array rather than no dependancies. Sure enough, logging on the server shows me that 2 subscription requests arrived there. As a note, the subscription handles I get back had different IDs, but I thought that Tracker.autorun took care of that behind the scenes (looking at Meteor Docs)?
Just to be silly, I did this to see what would happen:
And, I get 'Hello world' in my log twice (the function is not rendering due to a props change, BTW).
Final note: this didn't used to happen in version 2.1.1 (98% sure of that).
My concern is is two fold - First the extra Subscribes (and corresponding unsubscribes that happen right after) and also the unnecessary renders this causes.
Am I missing something? Is it a bug? Something else?
Looking forward to an expert opinion.
The text was updated successfully, but these errors were encountered: