fix(actions): do not dispatch duplicate events from already hooked listeners

[tjokimie]

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 equation 2*n^2 where n 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.

import React from 'react'
import { isLoaded, isEmpty, useFirebaseConnect } from 'react-redux-firebase'
import { useSelector } from 'react-redux'

const Avatar = ({ uid }) => {
  useFirebaseConnect(`profiles/${uid}`)
  const avatar = useSelector(state => state.firebase.ordered.profiles ? state.firebase.ordered.profiles[uid] : undefined)

  if (!isLoaded(avatar)) {
    return <div>Loading...</div>
  }

  if (isEmpty(avatar)) {
    return <div>Avatar Is Empty</div>
  }

  return (
    <div>
      <h1>Avatar</h1>
      <div>
        {JSON.stringify(avatar, null, 2)}
      </div>
    </div>
  )
}

const Feed = () => (
  <div>
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
    <Avatar uid="42ab723f-539c-4c50-82ef-94e79ce19d69" />
  </div>
)

export default Feed

Check List

If not relevant to pull request, check off as complete

• All tests passing
• Docs updated with any changes or examples if applicable
• Added tests to ensure new feature(s) work properly

[codecov]

Codecov Report

Merging #995 into master will decrease coverage by 0.11%.
The diff coverage is 100.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     

[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)

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

[tjokimie]

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! 😎

[tjokimie]

Hi @prescottprue! Any progress with this one? 👀 Would love to get this fixed 😄

[prescottprue]

Thanks for the reminder

[tjokimie]

Thanks for the reminder

Awesome! 🤩 Thank you so much!