Use effect warnings
Requirements for a pull request
Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
- [ ] Unit tests related to the change have been updated
- [ ] Documentation related to the change has been updated
Description of the Change
This refactor's intent is to reduce the inconsistent behaviors that have been seen in MFGUI.
The original code had a number of instances of the // eslint-disable-line directive that were masking missing dependencies in the dependency array of useEffect() functions. This pattern can be problematic because the code within the useEffect() can be using stale versions of the variables that should be included in the dependency array.
e.g.
useEffect(()=> {
if (isAGiraffe) {
eatAcacia()
}
} , []) // eslint-disable-line
In this case isAGiraffe is not defined in the dependency array and the value of isAGiraffe will be the one that existed when the useEffect() function was initialized. Subsequent calls of this function would still have the same value, even if isAGiraffe changed.
A better pattern is
useEffect(()=> {
if (isAGiraffe) {
eatAcacia()
}
} , [isAGiraffe])
However this pattern can lead to multiple (even infinite) re-renders of components. This is usually the reason why developers use the first pattern. To prevent those re-renders, I have used useMemo(), useCallback(), some object comparisons, and have moved some functions out of prop definitions.
Alternate Designs
- Remove almost all
useEffect()functions
Possible Drawbacks
- There may be some infinite re-renders caused by this refactor in areas of the app that have not been tested. Here is a good article explaining these issues - https://beta-reactjs-org-git-fx-deps-fbopensource.vercel.app/learn/removing-effect-dependencies
Verification Process
MFUI should work as before, probably with less inconsistencies.