DOCUMENT_ADDED being dispatched multiple times per single add operation

[gotdibbs]

Do you want to request a feature or report a bug?
I believe this to be a bug.

What is the current behavior? When you have multiple components listening to the same collection/key, it appears multiple listeners are added. This ends up producing the side effect of the corresponding ordered collection having new documents be inserted multiple times erroneously.

What is the expected behavior? When a document is added to a collection, DOCUMENT_ADDED should only fire once, and the ordered collection shouldn't have any duplicates.

Which versions of dependencies, and which browser are affected by this issue? Did this work in previous versions or setups? redux-firestore@0.6.2 react-redux-firebase@3.0.0-alpha.6

Steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

1. Add multiple components that connect to the same collection via firebaseConnect.
2. Ensure each component is mounted at least once.
3. Insert a new document into the collection from step 1.
4. Notice that DOCUMENT_ADDED fires multiple times and the corresponding ordered collection now contains duplicates.

Note: my scenario is that of a list view and several derived views (such as dashboards) on separate pages.

[prescottprue]

Are the queries the same settings? There is a feature called allowMultipleListeners which is set to false by default. That said, newer versions have oneListenerPerPath, and there is a PR currently in progress for making that take into account storeAs.

Could you possible provide your listener configs so I can attempt to replicate directly?

[gotdibbs]

@prescottprue Thanks for the quick response! I think I've uncovered the issue. I found ReactReduxFirebaseProvider calls createFirebaseInstance each time it is rendered. In createFirebaseInstance firebase._ is recreated anew each time. Sounds like this is maybe a bug with react-redux-firebase@next instead then?

Arguably, the issue was that I had ReactReduxFirebaseProvider nested under the BrowserRouter such that each time I switched pages, the instance was recreated. However, I would argue that createFirestoreInstance takes the smarter approach and uses merge to backfill defaults on firebase._. If you can provide decision, I'm happy to provide a PR to swap for merge in createFirebaseInstance.

Also, for what it's worth, here's my config for react-redux-firebase:

const rrfProps = {
    firebase,
    config: {
        enableLogging: process.env.NODE_ENV === 'development'
    },
    createFirestoreInstance,
    dispatch: store.dispatch
};

...and here is an example firebaseConnect instance:

firestoreConnect(({ uid, startDate, endDate }) => {
    return [
        {
            collection: 'transactions',
            where: [
                ['owner', '==', uid],
                ['date', '>=', startDate],
                ['date', '<=', endDate],
            ],
            orderBy: ['date'],
            storeAs: `transactions/${start.getTime()}`
        }
    ];
})

[prescottprue]

Great points, definitely thought about this before when starting to hear of unnecessary re-render issues. Yeah I like the approach of merging. If you want to go ahead and make a PR, I'll try to review it as soon as possible.

It seems like a better approach for now, and if that doesn't work out we can try some other things before releasing to latest.

[ljacho]

I can confirm this behaviour: http://bit.ly/DOCUMENT_ADDED @prescottprue

The same thing as in your case @gotdibbs, problem occurs if I switch page. I am using ConnectedRouter and "react-redux-firebase": "2.2.6"

Did you manage to figure it out? Can you give me an advice about how to solve that in my case - filling a table?

Thanks!

[prescottprue]

If it is due to multiple listeners being attached for the same path, folks could try using oneListenerPerPath which will only attach one listener, which seems like the intention (since all of those components are connected to the same piece of state, you don't need multiple listeners).

That said, in the future it may be smart to add warnings for this sort of thing since as mentioned, it is really the data that is wanted in all of the components - not multiple listeners attached at the same path. Right?

[ljacho]

I think it's not the thing. After the location is changed, the listener is set, got the response and then one DOCUMENT_ADDED action is fired for every single document in my collection - users. It's connected with firebase - auth.

@prescottprue , did you saw the video how it goes? http://bit.ly/DOCUMENT_ADDED

I tried to change rrfconfig:

const rrfConf = {
  attachAuthIsReady: true,
  userProfile: 'users',
  useFirestoreForProfile: true,
  oneListenerPerPath: true
}

and bottom of UserPage.js looks like this:

const mapStateToProps = (state) => {
  return {
    users: state.firestore.ordered.users,
  }
}

const mapDispatchToProps = (dispatch) => {
  return {
    register: (newUser) => dispatch(register(newUser)),
    remove: (user) => dispatch(remove(user))
  }
}

export default compose(
  firestoreConnect(['users']),
  connect(mapStateToProps, mapDispatchToProps),
)(UsersPage);

[gotdibbs]

@sbtgE I've corrected the underlying issue in my fork but I haven't had time to sort out where the unit test fits in yet.

The workaround is to move the react-redux-firebase provider HOC up in the tree. For example, I changed this:

<Provider store={store}>
    <BrowserRouter>
        <ReactReduxFirebaseProvider {...rrfProps}>
            // ...
        </ReactReduxFirebaseProvider>
   </BrowserRouter>
</Provider>

...to this:

<Provider store={store}>
    <ReactReduxFirebaseProvider {...rrfProps}>
        <BrowserRouter>
            // ...
        </BrowserRouter>
   </ReactReduxFirebaseProvider>
</Provider>

[prescottprue]

@gotdibbs nice! Thanks for looking into it. Feel free to make the PR anyway and then we can take a look at what is failing. There are some tests that have been around for a while and may need updating anyway.

[gotdibbs]

@prescottprue You bet, just couldn't figure out where the test fit in was all so would appreciate thoughts there.