Closed tjokimie closed 3 years ago
Merging #995 into master will decrease coverage by
0.11%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #995 +/- ##
==========================================
- Coverage 88.33% 88.22% -0.12%
==========================================
Files 29 29
Lines 797 798 +1
==========================================
Hits 704 704
- Misses 93 94 +1
So something of note: Firebase's SDK should actually internally de-dupe these listeners based on the query - even if you have listeners in multiple places only one actual query is made against the database (this can be proved with the database profiler). This is not clear based on the actions that are being dispatched though (which are not de-deuped)
Your PR makes the redux action dispatching more closely match that behavior from the Firebase SDK 👏
At first I was thinking that this should be in a new major version since some apps could function differently with non duplicated action dispatches but then I thought about how dispatching a ton of extra actions isn't really intended functionality 🤔 What are your thoughts?
Apologies that this has taken so long to get to
Hi @prescottprue!
So something of note: Firebase's SDK should actually internally de-dupe these listeners based on the query - even if you have listeners in multiple places only one actual query is made against the database (this can be proved with the database profiler). This is not clear based on the actions that are being dispatched though (which are not de-deuped)
I'm aware of that 🙂 However in this case the performance problem is caused by all the React re-renders caused by react-redux-firebase
actions. The example above demonstrates how the re-render count goes up when we have multiple instances of a component using the same data. The amount of re-renders will quite quickly crash the app.
Your PR makes the redux action dispatching more closely match that behavior from the Firebase SDK 👏
At first I was thinking that this should be in a new major version since some apps could function differently with non duplicated action dispatches but then I thought about how dispatching a ton of extra actions isn't really intended functionality 🤔 What are your thoughts?
I guess it depends how would you treat the actions dispatched by react-redux-firebase
. If those are considered to be internal implementation details this would not require major version bump. If those considered to be an API exposed by react-redux-firebase
major version upgrade would be more safe 🤔
Apologies that this has taken so long to get to
Thanks for getting back on this and thanks for all the good work on this library! 😎
Hi @prescottprue! Any progress with this one? 👀 Would love to get this fixed 😄
Thanks for the reminder
Thanks for the reminder
Awesome! 🤩 Thank you so much!
Description
src/actions/query.js
hooks the listener for each component. If the same data is used in multiple components on the same page each component will trigger a rerender for the other components as well. The amount of renders due to same data updates seem to follow the equation2*n^2
wheren
is the number of components using the same data. This increases quite quickly and freezes the app.Following example demonstrates the issue. The render method for
Avatar
is called 840 times. After the patch the render calls go down to 60.Check List
If not relevant to pull request, check off as complete