search

kentcdodds / use-deep-compare-effect

🐋 It's react's useEffect hook, except using deep comparison on the inputs, not reference equality
https://npm.im/use-deep-compare-effect
MIT License
1.88k stars 84 forks source link

useDeepCompareEffect should warn when dependencies includes function #47

Closed jasperck closed 3 years ago

jasperck commented 3 years ago

What you did:

Pass a nested object with/without a function as deps

What happened:

A nested object with property has function as value will break deep compare and trigger the hooks

Reproduction repository:

https://codesandbox.io/s/use-deep-compare-effect-repro-fn-break-compare-mt5tk

Problem description:

Given how dequal do the comparison plus how people pass function as props in React, effect callback might has chance to be called while we thought no properties were changed. Ideally, we will destruct props and pass as deps, or wrap function with useCallback when passing it for deps comparison, but with use-deep-compare-effect handling nested object, we might not realize the risk and just pass the whole object includes function to the deps array, in this case, the comparison will failed which calling the effect callback every time we click the button.

Suggested solution:

Not sure if we can handle this more properly here, but I'm thinking at least a warning would be very helpful for people who try to debug the source of triggering the hooks, e.g. when identify function during checkDeps raise a warning like useDeepCompareEffect would be triggered when dependencies has function, make sure this is expected behavior, otherwise, handle it properly. What do you think?

kentcdodds commented 3 years ago

Hi @jasperck,

Unfortunately providing a function isn't necessarily wrong and there's no way for us to know if you're doing it wrong or right. For example: https://codesandbox.io/s/use-deep-compare-effect-repro-fn-break-compare-forked-sqidp?file=/src/App.js

So I don't think there's anything we can do about this.

jasperck commented 3 years ago

Hey @kentcdodds,

providing a function isn't necessarily wrong and there's no way for us to know if you're doing it wrong or right

I totally agree with this, that's why I am purposing to add a warning but not error, because I think it's annoying that the hooks will be triggered out of the expectation while we thought deep compare handle things under the hood.

But thinking of when we have several unstable function in such case, logging warning probably is not an ideal developer experience, another thought is update README to make sure people know this scenario, what do you think?

kentcdodds commented 3 years ago

An update to the README would be welcome 👍 good idea.

jasperck commented 3 years ago

Hey @kentcdodds, sorry I was AWK last week. Thanks for the feedback and I just opened a PR 👆🙏

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 1.7.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: