meteor-react-native
meteor-react-native copied to clipboard
meteorrn
Better managing of Mongo Updates/Tracker
Restructure how we handle the Tracker, instead of a global listen to any change make the listens happen on specific queries and collections. Also fixes some bugs where we would generate way too many instances of subscriptions.
This also fixes the log in flow so the series of loggingIn -> LoggedIN is consistent and doesn't allow the package to ever auto log you out as that should be handled by the owning app just like in base meteor.
Every time you register a callback it gets added to a generic "changed" callback, that means that every single callback needs to be called on every single change to a collection which causes a huge hold up on the UI thread.
To make it even worst the subscriptions don't get cleaned up when you resubscribe so every time a "withTracker" or "useTracker" was called you would get stacked subscriptions. This then would lead to 100 plus subscriptions for a single component after some time which leads to bigger and bigger slow downs.
My PR separates that out as much as possible and makes sure that you only have as many subscriptions as you actually need. It is a pretty hefty change and I don't personally use and local collections in my app so those are going to be pretty poorly tested, but if other people want to take my change as a jumping off point I think its a pretty good point for server only subscriptions.
This also fixes up weirdness with the logging in and order of events where you would have an invalid user state or it would force you to logout because of bad internet
Likely fixes these erorrs: https://github.com/TheRealNate/meteor-react-native/issues/75 https://github.com/TheRealNate/meteor-react-native/issues/58
and maybe https://github.com/TheRealNate/meteor-react-native/issues/79
Hi @ToyboxZach,
I want to thank you for your extreme amount of work on this, however I don't think we can safely merge in a PR of this size.
We're going to adjust our contribution guidelines and formatting spec shortly, then we can circle back to these changes.
Closing for now.
@TheRealNate any more thoughts on an actual solution for the issue? This is a major issue for us too.
@TheRealNate Hi, commenting on behalf of @adamgins. Essentially we were seeing the issues that were described by @ToyboxZach above and in the issues https://github.com/meteorrn/meteor-react-native/issues/75 https://github.com/meteorrn/meteor-react-native/issues/96, excessive callbacks. We were able to find this from profiling our application using the hermes profiler. We previously used our own fork of inProgress-team/react-native-meteor but now migrated to a fork of ToyboxZach/meteor-react-native as we have some custom code of our own that runs in the meteor-react-native library. Anyway, switching the bases of our meteor-react-native fork to utilise the new meteorrn/meteor-react-native changes as well as @ToyboxZach's changes, we found that it solved our excessive callback issue.
It is hard to reproduce this issue as we did not notice it before but have run into it now as our application has become more complex. Not sure if it will be helpful but below is the before and after of doing the same action in our application (editing a document and observing the live changes):
Before: Excessive callbacks occurring due to Data.js highlighted in the red boxes.
After: No sign of callbacks.
Thanks to @ToyboxZach for the changes as it solved our issue and helped us avoid writing a solution ourself.
I have no time to push this any past what I need but fully support if someone wants to use this as a jumping off point for a new npm package. I do understand I didn’t make this change planning to merge it into this existing package and it is extremely risky for a lot of existing apps that use features that I do not.
Just wanted to quickly report that @ToyboxZach s fork solves all my performance issues with useTracker from the core package immediately. Had two simple trackers each of them subscribing to a meteor publication. The subscriptions got called up to 15 times, resulting in massive lags of the ui thread and bursting the server, although dependency list was empty.
I'd really love to get this merged.
@TheRealNate is it possible to please re-assess the position there's too much code to merge, given the impact here?
I believe if someone has time to push it through you could split my change into a few things:
src/components/useTracker.js This should solve most of the problem of too many trackers being added and not being cleaned up, I believe you can take that file as a single change and it will improve a chunk of these bugs
src/user/User.js I was having trouble with it autologging out or spamming my server too much, I did my best to fix the reactivity ( I know this isn't perfect as is, but it was a huge improvement experience-wise for me)
src/Collection.js src/Data.js src/Meteor.js src/ReactiveDict.js These changes are to fix up the issue of too many things being updated at once, so it splits the the updates out to be much more specific, similar to how web handles it
Everything else appears to be prettifying or some change I must have pulled in elsewhere.
Again sorry I don't have the time to break these into smaller testable PRs myself
@ToyboxZach yeah agreed the majority of the changes are prettier changes that make it harder to review.
@jankapunkt, what are your thoughts on merging this?
Reopening
Tried to get a minimal diff using @ToyboxZach s PR to resolve the useTracker issues. Basically step by step merged changes that seemed significant to the solution, did not yet review the code changes in detail, but the seem to be solved at the first glance. Will do some further testing. Please cf my fork.
hey @TheRealNate @ToyboxZach I can take a look at it. @ToyboxZach can you please comment on the files / lines that are most related to the fixes? I would go for them first and go through the code review etc. afterwards.
Edit: is this still the core fixes here:
src/components/useTracker.js This should solve most of the problem of too many trackers being added and not being cleaned up, I believe you can take that file as a single change and it will improve a chunk of these bugs
src/user/User.js I was having trouble with it autologging out or spamming my server too much, I did my best to fix the reactivity ( I know this isn't perfect as is, but it was a huge improvement experience-wise for me)
src/Collection.js src/Data.js src/Meteor.js src/ReactiveDict.js These changes are to fix up the issue of too many things being updated at once, so it splits the the updates out to be much more specific, similar to how web handles it
Please also note, that I have updated minimongo and removed nearly all deprecated dependencies that were replaceable by native es6+ code. So we somehow need to merge both of them into master.
A lot of these changes are only showing up here because its pulling into dev, which is behind master by a chunk of commits. A PR directly to master would be much smaller or if dev is updated to master
This one is much more sane, still has a bunch of prettify-ing which again sorry, but its much fewer changes https://github.com/meteorrn/meteor-react-native/pull/93/files?diff=split&w=1
A lot of these changes are only showing up here because its pulling into dev, which is behind master by a chunk of commits. A PR directly to master would be much smaller or if dev is updated to master
@ToyboxZach Ah gotcha. I've brought dev up to date with master. Could you try rebasing and lets see how big the PR ends up being?
I messed up a little bit while rebasing, and misunderstood a prompt, I think I have everything back to what I had before
Rebase can be such a pain... Let me know when you won the battle. Alternatively, there will be no blame if you close and open a new PR in case this gets out of hand.
@ToyboxZach could you please take a look at #99 and see if there may be potential clashes? I am still trying to figure out how we get both PRs work out in a smooth fashion.
My suggestion would be to make a change that only updates prettier and runs it on all the files, then merge that in, then I can load in that change, pretty much ignoring all of it, and then run prettier again in the same fashion so that most things don't change
@ToyboxZach sorry for late response :see_no_evil: I agree on that as it makes much sense. I would therefore go with #99 (it also includes all the prettier updates etc.) and then we tackle this one, right?
@ToyboxZach I merged #99 it's now on dev and contains all prettier fixes. It also contains CI for prettier lint and unit tests. You can merge dev and then continue providing the changes. A review from my end should be much easier without the prettier changes.
I opened a new PR from a different branch so that I don't mess up my main up. Its still probably two PRs within one, and I don't have the time to break it apart at all, but should be easier to see what I'm doing now