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

[Feature Request] Allow custom compare function on hooks. #16221

Closed
niwinz opened this issue Jul 26, 2019 · 14 comments
Closed

[Feature Request] Allow custom compare function on hooks. #16221

niwinz opened this issue Jul 26, 2019 · 14 comments
Labels
Resolution: Stale Automatically closed due to inactivity

Comments

@niwinz
Copy link

niwinz commented Jul 26, 2019

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
React performs a Shallow Compare on depenedencies for avoid unnecesary calls.

What is the expected behavior?

Shallow Compare is ok for plain values. For object it uses reference values and this may cause hook execution when maybe you don't want to.

My concrete use case is: im using ClojureScript immutable data structures that I want to be able to pass as dependency to a Hook and prevent execute the useEffect (as example) if the passed dependency is the same as previous one. With shallow compare two identical objects will have different reference values that will make execute again the useEffect (with the corresponding cleanup) when I really don't want.

I think that a good solution for this is provide the ability to pass a custom compare function to hooks (in the same way as React.memo).

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.8.6

@otakustay
Copy link
Contributor

I have a very interesting custom hook used for months to solve this issue:

const useOriginalCopy = <T>(value: T, customEquals: (x:T, y: T) => boolean): T => {
    const copy = useRef(value);

    if (!customEquals(copy.current, value)) {
        copy.current = value;
    }

    return copy.current;
};

This simple function can force a "custom equal" to become a "reference equal" and is IMO the greatest hook we ever made.

@lilactown
Copy link

lilactown commented Jul 26, 2019

@niwinz I've been using Hooks for awhile with ClojureScript and I believe that needing a custom comparator is a code smell.

I wrote a post that tries to explain the reasons why: https://lilac.town/writing/when-is-immutability-fast/

The TL;DR is that even when using immutable data, you should architect your app in a way that if the reference is different, then that means the data has changed. If not, you're not using immutable data well.

This means that you should move static data creation into a top-level def or inside of a useMemo. State updates should prefer using immutable updates like assoc / update / etc. instead of returning a map literal.

Without seeing your code, it's hard to give specific advice but the above should cover 80% of cases that you feel like you need a custom comparator. It's much better to not re-compute the data if it hasn't changed then to do a deep comparison every render.

@otakustay
Copy link
Contributor

Even with best efforts there are situations that an object may change its reference without content modification, for example, we fetch a data periodically from remote, even within its Expire header time, the response is a new object with exactly same content.

Also, useMemo doesn't guarantee its cache to be permanent, we can leave these reference change to a re-render in performance realm, but things get different considering side effects, we need a custom equality to eliminate unpredictable side effects.

@lilactown
Copy link

lilactown commented Jul 27, 2019

Yes, there is a corner case where fetching data that returns the exact same data would be a different object. You can use something like your hook above (here’s the ClojureScript version), but you’re still relying on useMemo.

I believe this will become much easier to handle once Suspense is delivered. It seems like we should be able to memoize within the cache layer to return the same reference if the data is the same.

@niwinz
Copy link
Author

niwinz commented Jul 29, 2019

Thank you all @Lokeh @otakustay

The TL;DR is that even when using immutable data, you should architect your app in a way that if
the reference is different, then that means the data has changed. If not, you're not using immutable data well.

Yeah, I have readed your post @Lokeh, I know that, but i need that for the corner cases. Seems like there are a good solution using custom hooks, so I probably use it.

I leave this issue open, because I think that if React.memo supports it, the hooks also can have the same. So I leave the decision to close this to the react team.

@alirezamirian
Copy link

This is so useful.
People are creating things like useDeepMemo and useDeepCompareEffect which will be covered by a simple compare function.

@justingrant
Copy link
Contributor

@Lokeh - another case where this custom-compare-function approach is helpful is with objects like Date, RegExp, Array, and other types where the data is immutable but the Javascript implementation of those types requires new object creation. As a result I wrote a generic useCustomState implementation that's almost identical to @otakustay's above, and then I wrote a useDateState, useArrayState, etc. which are all implemented on top of it.

An alternative to this approach is to serialize objects into a string or number and use that non-object value for comparison. But to avoid errors and bad DX I'd still end up wrapping these in custom hooks that do the serializing, so IMHO there's no substantive difference--just implementation details-- between implementing useDateState using a custom comparer function or using serialization.

I do agree with you, however, that if the data is semantically different (not just a different object built around semantically identical data) then the custom-compare-function solution can be a risky slippery slope to bugs.

@stale
Copy link

stale bot commented Mar 14, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Mar 14, 2020
@justingrant
Copy link
Contributor

Not stale.

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Mar 14, 2020
@stale
Copy link

stale bot commented Jun 12, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jun 12, 2020
@justingrant
Copy link
Contributor

Not stale

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jun 12, 2020
@giautm
Copy link

giautm commented Jun 25, 2020

I'm request same functional, too.

@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Oct 4, 2020
@stale
Copy link

stale bot commented Oct 12, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

No branches or pull requests

6 participants