react-redux-firebase icon indicating copy to clipboard operation
react-redux-firebase copied to clipboard

bug(hooks): populate within useSelector hook causes unnecessary re-renders

Open rbutov opened this issue 5 years ago • 7 comments

How to implement populate without HOC? I'm trying to do something like that:

const [profile, setProfile] = useState(null); useFirestoreConnect(() => [ { collection, storeAs: profileStorageName, populates, where: ['name', '==', params.id], } ]);

const profiles = useSelector((state: RootState) => populate(state.firestore, profileStorageName, populates, shallowEqual));

useEffect(() => { setProfile(profiles); // infinite loop... }, [profiles]);

but useEffect calling in infinite loop.... So how to implement populate data?

Thanks

rbutov avatar Jun 02 '20 14:06 rbutov

You shouldn't be setting to component state or calling useEffect - the state is already managed in redux. You can just render the profiles data.

Please reach out if you are still having an issue after removing and we can reopen

prescottprue avatar Jun 03 '20 00:06 prescottprue

You shouldn't be setting to component state or calling useEffect - the state is already managed in redux. You can just render the profiles data.

Please reach out if you are still having an issue after removing and we can reopen

Ok, that makes sense. But why this: const profiles = useSelector((state: RootState) => populate(state.firestore, profileStorageName, populates, shallowEqual)); trigger changes every time when I call setState from useState? Cuz useEffect just monitors state changing.

It works properly on from connect and HOC:

connect(({firestore}: {firestore: any}) => ({
    // populate original from data within separate paths redux
    [profileStorageName]: populate(firestore, profileStorageName, populates),
    // firebase.ordered.todos or firebase.data.todos for unpopulated todos
  })),

So I guess that can cause performance issue.

rbutov avatar Jun 03 '20 00:06 rbutov

I am not sure what you are using to set state - using an HOC will have the listeners reattached on mount/unmount of the component but only changes the listeners if settings change. If you are using the hook, then when your component re-renders you will be recalling the render function regardless of if listeners have changed.

It is not so much a performance issue as it is a different way to organize things - hopefully that is all making sense? Let me know if you think that we could add anything to the docs to make it more clear

prescottprue avatar Jun 03 '20 01:06 prescottprue

I see... But if I'm using: const profiles = useSelector(state => state.firestore.data.profiles); instead of: const profiles = useSelector(state => populate(state.firestore, profileStorageName, populates, shallowEqual)); It's working properly: not triggering profiles changes. So the problem is from populate function that updating every time when I call setState(re-rendering). But without populate working well.

May be this is not good idea to use populate inside the useSelector hook.. like that?: const profiles = useSelector(state => populate(state.firestore, profileStorageName, populates, shallowEqual));

Thank you!

rbutov avatar Jun 03 '20 02:06 rbutov

Going to reopen and rename this so we can look deeper, thanks for the detail

prescottprue avatar Jun 03 '20 18:06 prescottprue

So: const profiles = useSelector(state => populate(state.firestore, profileStorageName, populates, shallowEqual)); I found that every time when component re-rendering it get new profiles value from useSelector when populate is using and useEffect detecting that profiles value was changed. So this is wrong behavioral. Without populate useSelector is working correctly.

Thanks

rbutov avatar Jun 04 '20 01:06 rbutov

This also happens when we use react native navigation. For example, I load a profile from redux using useSelector and then when we do navigation (push). The previous profile is empty and then get a new profile again.

Everything works fine without populate, thank you

agungkes avatar Oct 13 '20 11:10 agungkes