metaflow-ui icon indicating copy to clipboard operation
metaflow-ui copied to clipboard

Use effect warnings

Open obgibson opened this issue 3 years ago • 0 comments

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.

Release Notes

obgibson avatar Jun 13 '22 16:06 obgibson