meteorrn / meteor-react-native

Meteor client for React Native matching Meteor Spec
https://guide.meteor.com/react-native.html
Other
59 stars 31 forks source link

Make Collection updates more seperate and don't auto logout client with bad internet #93

Closed ToyboxZach closed 2 years ago

lgtm-com[bot] commented 2 years ago

This pull request introduces 3 alerts when merging 4a2f1318759f092e03c58a1e1f9222f4b94db53a into 0f005422d078a218305e46f9e2b9e1b7d1b209b0 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 3 alerts when merging d5dc78a68dd7975a9c862c872ce3e16bd65556da into 0f005422d078a218305e46f9e2b9e1b7d1b209b0 - view on LGTM.com

new alerts:

ToyboxZach commented 2 years ago

This PR looks bigger than it is because I used prettier, like I expected master to have been using.

ToyboxZach commented 2 years ago

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

lgtm-com[bot] commented 2 years ago

This pull request introduces 3 alerts when merging 7d1acdecac63fc7d93a2a054c44fa27983d9c530 into 0f005422d078a218305e46f9e2b9e1b7d1b209b0 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging 598c2cf2cd387c1c8d0ff0be5cccc75380ed938a into 0f005422d078a218305e46f9e2b9e1b7d1b209b0 - view on LGTM.com

new alerts:

TheRealNate commented 2 years ago

Hey @ToyboxZach, thinking due to the size of the change maybe we merge this onto dev branch? From there I can publish a beta version for people to test.

ToyboxZach commented 2 years ago

Ok moved to dev