facebookarchive / redux-react-hook

React Hook for accessing state and dispatch from a Redux store
MIT License
2.16k stars 103 forks source link

Reducing components rerenders #53

Closed goloveychuk closed 5 years ago

goloveychuk commented 5 years ago

Looks like we can reduce component rerenders count by removing useEffect from here https://github.com/facebookincubator/redux-react-hook/blob/676a270be12f0bcbffe9d650ef769d6dd486ef70/src/create.ts#L91-L94 Since useEffect will execute somewhen in the future after component renders, even if our hook already returned new state, https://github.com/facebookincubator/redux-react-hook/blob/676a270be12f0bcbffe9d650ef769d6dd486ef70/src/create.ts#L110 still compares to old.

In practice, for my case, with using 6 hooks and 4 dispatches, it's reducing rerenders counts from 6 to 4.

here is used useLayoutEffect https://github.com/reduxjs/react-redux/blob/06a674e733454b08bb4090f8bcb038a3a003e6aa/src/hooks/useSelector.js#L79-L85 Which seems to work correctly too.

Turanchoks commented 5 years ago

It will be unsafe to mutate refs in the render phase when concurrent mode arrives . You can read more on why here https://github.com/facebookincubator/redux-react-hook/pull/9 for example. In short a render might be aborted and the commit won't happen so you might end up in a situation where you have a ref pointing to a future state that hasn't been committed yet.

Do you have 6 rerenders because you dispatch 4 times in the body of the component? Have you considered using 1 action that does what all 4 do?

goloveychuk commented 5 years ago

@Turanchoks shouldn't useLayoutEffect work correctly? It should execute synchronously after mutating dom.

Do you have 6 rerenders because you dispatch 4 times in the body of the component? Have you considered using 1 action that does what all 4 do?

Yea, I can fix this many ways, I'm just saying that for 4 state mutations, having multiple store subscriptions (useMappedState hooks), I have 6 component rerenders. With both removing useEffect (which is bad idea, as you said) and using useLayoutEffect I have 4.

Turanchoks commented 5 years ago

So there are actual commits between the dispatches in your case, right? I originally used useLayoutEffect too as it solved a different issue which is no longer a problem now. There is a whole paragraph https://github.com/reduxjs/react-redux/blob/06a674e733454b08bb4090f8bcb038a3a003e6aa/src/hooks/useSelector.js#L7 that describes why they use useLayoutEffect but I'm not sure I fully understand the issue.

There is no overhead in using useLayoutEffect instead of useEffect so I think we can change it. A test case would be great.

Maybe @markerikson could help. Is there a test case in the react-redux repo or anywhere that covers a problem of using useEffect in https://github.com/reduxjs/react-redux/blob/06a674e733454b08bb4090f8bcb038a3a003e6aa/src/hooks/useSelector.js#L85? Thanks

goloveychuk commented 5 years ago

@Turanchoks https://github.com/facebookincubator/redux-react-hook/pull/54 this test should cover this case. The reason I removed act is because, it seems to commit useEffect hook synchronously, which is not how it works in real apps. If you remove useLayoutEffect - test will fail with 5 rerenders.

markerikson commented 5 years ago

The original implementation connect in v7.0 used useEffect(), but we ran into issues with the timing of subscriptions, so we had to change that touseLayoutEffect()` as well:

https://github.com/reduxjs/react-redux/issues/1226 https://github.com/reduxjs/react-redux/pull/1235

arwysyah commented 3 years ago

const Home = ({navigation}) => { const globalState = useSelector(state=>state)

const [loading, setLoading] = useState(true); const [data, setData] = useState([]); const dispatch = useDispatch() useLayoutEffect(()=>{ dispatch(watchData()) },[dispatch])

useLayoutEffect(() => { setTimeout(() => { setData(data);

  setLoading(false);
}, 1000);

}, [data]); console.log(globalState.request) return()}

this functional component rander three times when i dispatch a action with redux, can anyone tell me whats wrong with this one ?

ianobermiller commented 3 years ago

@arwysyah A codesandbox link would make your problem easier to diagnose, as the code doesn't seem to make sense on its own.