Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix useForceUpdate Memory Leak: Only setState if the component is still mounted #30667

Merged

Conversation

mikejolley
Copy link
Contributor

Description

Whilst working with Slot Fills, I found that when removing a component that renders a Slot Fill, a memory leak error is logged to the console:

Screenshot 2021-04-09 at 14 51 41

Tracing it back, it seems to be occurring because useForceUpdate returns a setState method which can still be called after the component consuming it is unmounted. This can be fixed by checking if useForceUpdate is unmounted before calling setState.

function useForceUpdate() {
const [ , setState ] = useState( {} );
return () => setState( {} );
}

This only occurs if using Slot Fills with bubblesVirtually enabled.

This issue might be related to this note from @diegohaz #17355 (comment), however, in my testing, it only occurred with bubblesVirtually on, not off.

How has this been tested?

I tested this by applying the fix directly to the node_modules in WooCommerce Gutenberg Products Block plugin, where Slot Fills are imported. After applying the patch I could no longer reproduce the memory leak issue.

Note it can only be tested if:

  • bubblesVirtually is on
  • The Slot has fills (empty slots do not break)

It was logged here: woocommerce/woocommerce-blocks#4047

Types of changes

This is a bug fix within the components package, for Slot Fills.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.

@nerrad nerrad added [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Apr 9, 2021
@nerrad nerrad requested a review from diegohaz April 9, 2021 15:13
@nerrad
Copy link
Contributor

nerrad commented Apr 9, 2021

@diegohaz would you mind reviewing this given you've done some work in this area?

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! There are some failing e2e tests, but I'm not sure they're related to this change. I did a rerun on them just in case.

I also believe this PR fixes #28478

@nerrad nerrad merged commit 2e6676a into WordPress:trunk Apr 9, 2021
@nerrad
Copy link
Contributor

nerrad commented Apr 9, 2021

Thanks Mike 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants