prescottprue / react-redux-firebase

Redux bindings for Firebase. Includes React Hooks and Higher Order Components.
https://react-redux-firebase.com
MIT License
2.55k stars 559 forks source link

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

Open rbutov opened 4 years ago

rbutov commented 4 years ago

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

prescottprue commented 4 years ago

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

rbutov commented 4 years ago

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.

prescottprue commented 4 years ago

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

rbutov commented 4 years ago

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!

prescottprue commented 4 years ago

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

rbutov commented 4 years ago

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

agungkes commented 3 years ago

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