Skip to content
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

Open
RickHuizing01 opened this issue Nov 15, 2024 · 5 comments · May be fixed by #2617
Open

Remove potentially unnecessary use of useEffect in usePrevious #2605

RickHuizing01 opened this issue Nov 15, 2024 · 5 comments · May be fixed by #2617

Comments

@RickHuizing01
Copy link

RickHuizing01 commented Nov 15, 2024

Is your feature request related to a problem? Please describe.

The current implementation of the usePrevious hook relies on useEffect 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:

import { useRef } from "react";

export const usePrevious = <T>(state: T): T | undefined => {
    const ref = useRef<T>();

    const previousValue = ref.current;
    ref.current = state;

    return previousValue;
};

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 both useRef and useEffect:

import { useState } from "react";

export const usePrevious = <T>(state: T): T | undefined => {
    const [current, setCurrent] = useState<T>(state);
    const [previous, setPrevious] = useState<T>();

    if (JSON.stringify(current) !== JSON.stringify(state)) {
        setCurrent(state);
        setPrevious(current);
    }

    return previous;
};

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 the useStateHistory hook as it naturally handles the "main" state.

I am still exploring alternative approaches but wanted to share my progress so far.

@RickHuizing01 RickHuizing01 changed the title Use potentially unnecessary use of useEffect in usePrevious Remove potentially unnecessary use of useEffect in usePrevious Nov 15, 2024
@FleetAdmiralJakob
Copy link

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?

@childrentime
Copy link

@FleetAdmiralJakob
Copy link

btw. alibaba has the same discussion 😂: alibaba/hooks#2162

@RickHuizing01
Copy link
Author

RickHuizing01 commented Dec 9, 2024

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 JSON.stringify() as you mentioned).

About JSON.stringify vs ===:
I think we can go with just ===. React ensures that state updates always create a new reference, even when the content is the same, due to its immutability rules. This means === should be sufficient to detect changes, as references will differ for updated states. Using JSON.stringify or deep equality seems unnecessary in this context.

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;
};

@FleetAdmiralJakob
Copy link

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

@RickHuizing01 RickHuizing01 linked a pull request Jan 6, 2025 that will close this issue
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants