bug(firebaseConnect): react-router race condition

[garyforsterio]

Do you want to request a feature or report a bug?
bug

(If this is a usage question, please do not post it here—post it on gitter. If this is not a “feature” or a “bug”, or the phrase “How do I...?” applies, then it's probably a usage question.)

What is the current behavior? When transitioning between two components using react-router, the new component is mounted before the existing component is unmounted. Consequently, firebaseConnect() creates a listener before the old one has been detached. This in most cases is fine, however in my specific case, both components are subscribing to the same firebase path (albeit with different query parameters) which doesn't work. The way that listeners are currently detached (see here), means that both the new and the old listeners are detached.

This if statement does not catch this edge case as my query parameters are different.

I think this could be overcome by passing the listener to off(listener) as documented here

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

1. Mount a component using firebaseConnect.
2. Mount a second component using firebaseConnect and referencing the same firebase path with different queries.
3. Unmount the first component.

What is the expected behavior? The second component continues to receive data from firebase after the first component has unmounted.

Which versions of dependencies, and which browser and OS are affected by this issue? Did this work in previous versions or setups?

    "react": "^16.3.2",
    "react-redux-firebase": "^2.1.1",
    "react-router-dom": "^4.2.2",

[prescottprue]

@garyforsterio Thanks for reporting, this is an interesting case! I agree with the off(listener) pattern, and have been looking to move towards it inside of react-redux-firebase for a while -- now there is a solid reason.

Interested if storeAs would be helpful as part workaround in the meantime?

Totally open to a pull request if you have the time. If not I'll try to get to it as soon as I can.

[cryser29]

I faced the same issue. It seems that we should replace componentWillMount() with componentDidMount() in both firebaseConnect and firestoreConnect methods. In that case unsetListeners will be called before setListeners as it should be.

Do we have some another reason why componentWillMount() can be really required here?

Meanwhile, as a temporary workaround I've used allowMultipleListeners: true but oneListenerPerPath: true helps to resolve the issue too.

[prescottprue]

@cryser29 Thanks for the input and ideas for work arounds!

I would be interested to see what is impacted by this change. Some folks may be depending on this order, so it may warrant v3.

[garyforsterio]

I was going to have a look at implementing the off(listener) pattern this week and try and put together a pull request but to be honest I think @cryser29 has a good point, especially so considering componentWillMount is being renamed/depreciated. Not really sure about any knock-on effects of this would be though...

[StanislavMayorov]

@cryser29 thanks.

reactReduxFirebase reducer is used to set this setting.

  reactReduxFirebase(firebase, {
    allowMultipleListeners: true,
    ...
  }),

[yevgenypats]

Hey Guys, Any news with that? got the same issue + the workaround didn't help so I have to use the following workaround: storeAs: 'incident_' + props.match.params.id instead of using just storeAs: 'incident'

[yevgenypats]

Any news with that one?

[prescottprue]

@yevgenypats Due to the potential impact, work on this will go into the next major version. I started working on it on the v3.0.0-alpha branch. There may be a bunch of other big changes, so more updates to come about timelines.

As mentioned above, anyone experiencing this should use a unique storeAs to get around it. For instance, if you are seeing this on a project page, using:

storeAs: `project_${props.params.projectId}`