-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove potentially unnecessary use of useEffect in usePrevious #2605
Comments
I saw an interesting video about this today: https://www.youtube.com/watch?v=B-Xb_8n5wRg And I'm just curious why we would need JSON.stringify() and not just do it like https://github.com/uidotdev/usehooks/blob/90fbbb4cc085e74e50c36a62a5759a40c62bb98e/index.js#L1017-L1027 without it? |
btw. alibaba has the same discussion 😂: alibaba/hooks#2162 |
Cool to see that other libraries are also picking this up. Also, very good video @FleetAdmiralJakob, thanks for sharing! It’s interesting to see other libraries already using this implementation (without the About That said, I sometimes get a bit confused when it comes to object equality haha, so feel free to correct me if I’m wrong! all in all, this would lead to (basically the same implementation as used by usehooks from uidotdev): export const usePrevious = <T>(state: T): T | undefined => {
const [current, setCurrent] = useState<T>(state);
const [previous, setPrevious] = useState<T>();
if (current !== state) {
setCurrent(state);
setPrevious(current);
}
return previous;
}; |
Yes, totally agree with you @RickHuizing01. For https://github.com/childrentime/reactuse we went with the version without deep equality too: https://github.com/childrentime/reactuse/blob/main/packages/core/src/usePrevious/index.ts |
Is your feature request related to a problem? Please describe.
The current implementation of the
usePrevious
hook relies onuseEffect
to store the previous state. While this approach works, I believe that the use of useEffect might not be necessary. The usage of useEffect could be considered an anti-pattern according to the official React documentation (as described in You Might Not Need an Effect) if I interpret it correctly.Describe the solution you'd like
Initially, I considered removing the
useEffect
altogether, resulting in the following implementation:While this solution works, my colleague @dasblitz pointed out that reading or writing to a
ref
during rendering is also considered an anti-pattern, as highlighted in the official React documentation here: https://react.dev/reference/react/useRef (see the pitfall section). Consequently, I created an alternative version that avoids bothuseRef
anduseEffect
:However, there is a significant pitfall with this implementation: the
useState
hook maintaining the current value essentially duplicates the state of the component using this hook. Based of that, the second implementation would make more sense if it was theuseStateHistory
hook as it naturally handles the "main" state.I am still exploring alternative approaches but wanted to share my progress so far.
The text was updated successfully, but these errors were encountered: