Fix not cancelling delayed portal removal when component is remounted

[tstehr]

Fixes the bug described in #1063, where a modal unexpectedly closes after being suspended when using React 18.

Issue details

The issue happens when using React 18 due to a change in the semantics of Suspense. Since React 18, a component's componentWillUnmount is called when it is suspended and componentDidMount is called when it is unsuspended.

In react-modal a call to componentWillUnmount triggers removePortal with a timeout when closeTimeoutMS is set (see #17 and #253). This timeout is never cancelled, even if the component is mounted again afterwards, as happens when it is unsuspended. Thus, the following sequence of events can happen, leading to the modal staying closed:

1. The component is suspended, componentWillUnmount is called and a timeout for removePortal is started.
2. The component is unsuspended, componentDidMount is called. It adds the portal node as a child of the parent element, which is a noop at this point because the node has not yet been removed.
3. The removePortal timeout happens so the portal node is removed.

Proposed fix

Clear the removePortal timeout in componentDidMount. In my testing, this leads to the portal node no longer being removed, fixing the issue.

Acceptance Checklist:

• Tests
  • This can't be easily tested, as the issue happens from React 18 onwards only and the tests use React 17.
• Documentation and examples (if needed)
  • Not needed, this is only a bug fix

[diasbruno]

Thanks, @tstehr. I'll need to update the CI. It still runs on node 16. I'll get back to you when it's updated.

[diasbruno]

@tstehr If you rebase with master branch now, the tests should run.

[tstehr]

@diasbruno Thank you for the quick reply and for updating the CI! I've rebased the change 👍

[diasbruno]

I’ll accept this patch since it seems to work on the current version.

I’ll try to find a way to test on other react versions.

Thanks, @tstehr

[diasbruno]

One more thing…if you can check the constructor to include this new property as null.

[tstehr]

One more thing…if you can check the constructor to include this new property as null.

Sure, I've opened #1066 for that