redux-firestore icon indicating copy to clipboard operation
redux-firestore copied to clipboard

bug(query): next.js causes firing order of SET_LISTENER and UNSET_LISTENER action types to be incorrect

Open tky5622 opened this issue 6 years ago • 10 comments

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

What is the current behavior? when I use firestoreConnect in pages file and move another page, the order of setting SET_LISTENER and UNSET_LISTENER become reverse. So, if pageA and pageB connect same path, it occurs that firstly pageB fires SET_LISTENER , then pageA unset pageB's listener.

What is the expected behavior? I want some method to wait for finishing UNSET_LISTENER or delay SET_LISTENER's timing.

Which versions of dependencies, and which browser are affected by this issue? Did this work in previous versions or setups? "next": "^6.1.1", "redux-firestore": "^0.5.7", "react-redux-firebase": "^2.1.6",

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

tky5622 avatar Jul 18 '18 09:07 tky5622

@tky5622 That is most likely due to the fact that multiple listeners are being attached for the same path. Though this is sometimes intended, it seems like you want at maximum one listener per path. Can you give the oneListenerPerPath: true option a try?

Could be remembering wrong, but I believe that option was added by @compojoom. He may be able to put it into words better, but I believe this is a similar issue he was experiencing before making the oneListenerPerPath option.

Going to leave open until we confirm.

prescottprue avatar Jul 18 '18 17:07 prescottprue

Yep, sounds like what I had. My listeners were being unmounted.

If you set oneListenerPerPath we'll set the listener the first time you need it and afterwards if you try to set the same listener again we'll just increment a counter to know how many components are listening on this path. We won't unset the listener until the counter drops to 0.

compojoom avatar Jul 18 '18 18:07 compojoom

I tried oneListenerPerPath: true , but this doesn't works for me. I use this HOC introduced in this Issue.

https://gist.github.com/artisin/d56750d955ff4a60ed11e6a714cbd1bd https://github.com/prescottprue/react-redux-firebase/issues/230

I don't know the exact cause, but I feel that this library is not compatible with next.js now.

tky5622 avatar Jul 22 '18 22:07 tky5622

I reproduced this issue. If I connect same path between two pages, this connection will fail. https://github.com/tky5622/next_with_redux-firestore

tky5622 avatar Jul 23 '18 18:07 tky5622

@tky5622 Thanks for the repro, I'll try to look into this sometime this week.

prescottprue avatar Jul 23 '18 18:07 prescottprue

I am having the same issue, but without next. My problem is that the component which is hooked to firestoreConnect is mounted and umounted always that my route change. This behaviour triggers the componentWillMount (for the 2nd route) before componentWillUmount (of the 1st route) making a call to unsetListener right after setListener.

I think that if we change the call to firestore.setListener to be inside componentDidMount instead of componentWillMount can solve the issue.

hudovisk avatar Jul 23 '18 20:07 hudovisk

@hudovisk Great to hear there is a theory of how to fix it. Wondering if this would be breaking for anyone. Either way, we can may try an alpha version and release it to the next tag with that change and see if it helps. Feel free to open a PR, and I can handle releasing it (otherwise I'll get to it when).

Out of curiosity, did you try the oneListenerPerPath: true config option as well?

prescottprue avatar Jul 23 '18 21:07 prescottprue

I tried it but the wrong way, I was passing it to the reduxFirestore from react-redux instead of reactReduxFirebase from react-redux-firebase. After I got it right the problem was solved :) thanks!

But the order of the lifecycle hooks might be interesting to take a look.

hudovisk avatar Jul 24 '18 13:07 hudovisk

@compojoom oneListenerPerPath fixed my problem! Thanks so much for implementing this!!

devinmorgan avatar Feb 03 '19 23:02 devinmorgan

@devinmorgan Super great to know, thanks for updating! I am going to look into adding that to the SSR/next section of the docs. Also, in newer versions the plan is to have this be the default and warn in the case of multiple listeners on the same path (obviously with a way to silence the warning if that is the intention).

prescottprue avatar Feb 05 '19 00:02 prescottprue