useDeepCompareEffect should warn when dependencies includes function
jasperck opened this issue · 5 comments
use-deep-compare-effectversion: 1.6.1
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?
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.
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?
An update to the README would be welcome 👍 good idea.
Hey @kentcdodds, sorry I was AWK last week. Thanks for the feedback and I just opened a PR 👆🙏
🎉 This issue has been resolved in version 1.7.0 🎉
The release is available on:
Your semantic-release bot 📦🚀