Fix: memory leaks in application #242
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 4 commits
November 19, 2020 07:20
Previously, if you tried to navigate away from a layout / component while it was still performing an async operation, we would receive the following error in the console: ``` Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function ``` This is slightly differently-phrased if the component is a class component but the idea is the same. The solution to this is to specify a cleanup function. To execute this, we define an `_isMounted` variable, and only set state updates when this variable is set to true. In our cleanup function, we set this variable back to false so that state updates are not performed on unmounted components. Reference: https://stackoverflow.com/questions/53949393/cant-perform-a-react-state-update-on-an-unmounted-component
This commit refactors the LoadingButton so that it runs the callback in the proper order. Previously, we would attempt to set the button to a loading state, await on the callback function, and then set the button back to its non-loading state. The problem is that the setState functions are not asynchronous so they do not run on the same event loop as the callback. This commit refactors the code so that the code is run more reactively using a useEffect.
Previously, React throws an error complaining about a memory leak whenever we select an image in the EditPage layout. It turns out this is because we attempt to set the LoadingButton component's loading state to false even after the component has unmounted. This is fixed by applying the same fix from a couple commits ago to the LoadingButton component.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR fixes instances in the app where we attempt to perform state updates on unmounted components. It resolves issue #228.
Occasionally, when we navigate away from a layout on the CMS, or close a modal before it's done loading something, we receive the following error message in the console:
The problem arises when we are awaiting some async operation, and setting state after the async operation completes. For example, I might be in the
EditPagelayout, and after I retrieve the page's content, I set that content into the component state.The problem is that if I navigate away from the
EditPagelayout while it is retrieving the content (the async operation), theEditPagecomponent unmounts, but thecomponentWillMountfunction, or theuseEffectfunction will continue to run and attempt to set state after the async operation completes. This is why we get the error message about updating state on an unmounted component.Solution
The solution is to only set state when we can be sure that the component is mounted. We can do this by introducing an
_isMountedlocal variable, which is set totruewhen the async operation is run, but also define a cleanup function (either within theuseEffector ascomponentWillUnmount) which sets the_isMountedvariable tofalse. Since state updates are only performed when_isMountedistrue, we no longer attempt to update state after a component is unmounted. The caveat is that_isMountedneeds to be defined as a normal variable, and not as state, since it needs to be updated synchronously for this to work.