fix(react): inline overlays dismiss when parent component unmounts

[sean-perkins]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
• Build (npm run build) was run locally and any changes were pushed
• Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

When navigation events occur (push, pop, replace), presented inline overlays are not unmounted from the DOM. Additionally, when a parent component is unmounted, presented overlays are not unmounted.

Issue URL: #25775, #26185

What is the new behavior?

• When a parent component is unmounted, any presented overlay within the context of that component will additionally be unmounted.
  • This operation skips the dismiss life cycle and does not perform the dismiss transition, emit the willDismiss or didDismiss events.
• Navigation actions, such as push, pop or replace will unmounted presented overlays.
  • This operation skips the dismiss life cycle and does not perform the dismiss transition, emit the willDismiss or didDismiss events.

Does this introduce a breaking change?

• Yes
• No

Other information

Dev-build: 6.5.4-dev.11676062329.117ad0d6 (updated 02/10/2023)

[sean-perkins]

Updated dev-build: 6.3.6-dev.11668106431.125dbfa9

[sean-perkins]

Thinking about this approach a little more, if we do not like the filtering out of events to pass to attachProps. We could probably come up with a detachProps utility that pulls the cached events from the node.__events event store and remove the event listener when the component is unmounted.

[puopg]

Just checking in on this fix, could the team provide an update what is its status?

[patdx]

I just tried out the dev build 6.3.6-dev.11668106431.125dbfa9 above, it also gives better behavior for React Error Boundary:

const BuggyComponent = () => {
  throw new Error("Failed to render!");
};

const App = () => {
  return (
    <ErrorBoundary>
      <IonModal>
        <BuggyComponent />
      </IonModal>
    </ErrorBoundary>
  );
};

In the current prod build I just discovered that code like above freezes the (inactive) modal over the whole screen.

But with this dev build it automatically closes the modal as I would have hoped.

[puopg]

@sean-perkins Bump?

[sean-perkins]

Open PRs are considered active work. There are a number of factors that effects our prioritization of different work items.

This specific PR addresses behavior that has not been designed or documented by Ionic and is important that we spent time to thoroughly designing the expected behaviors internally, so we do not need to perform a breaking change to correct that behavior in the future.

This PR has been updated to remove incorrect assumptions I had made in the first implementation pass, such as trying to perform the dismiss transition for the overlay when the parent component unmounts or a router action is performed. In these scenarios, we should be treating the overlay lifecycle as:

graph TD
A[Overlay]--> Mounted
Mounted --> Presented
Presented --> Unmounted

Loading

In this diagram we show that the "Dismiss" lifecycle does not occur. This means that a dismiss transition is not performed and the respective events willDismiss and didDismiss are not emitted. This is intentional to avoid memory leaks and unexpected scenarios where callback events are firing as a result of side-effects.

Here is an updated dev-build to test with: 6.5.4-dev.11676062329.117ad0d6.

[liamdebeasi]

Why do we only remove the didDismiss event?

[sean-perkins]

The createInlineOverlayComponent binds two internal event listeners for didDismiss and willPresent. Both implementations have an internal state update for:

this.setState({ isOpen: false }); // `true` for willPresent

At this point in the lifecycle of the component the component is unmounting. willPresent cannot fire. However, didDismiss can fire and cause any user-implemented callback handlers for onDidDismiss to run after the React component has unmounted. If this occurs, React will throw an exception in the console about a memory leak.

This code disconnects the internal event listener manually, before the element can dispatch didDismiss.

[averyjohnston]

Would detachProps handle removing that listener? If so, would it make sense to instead call detachProps before node.remove and then remove this line?

[sean-perkins]

detachProps only removes event listeners added by attachProps (because we track the events on the element node on a key called __events). This specific event listener is manually added within the React class and isn't removed as a result of detachProps.

[averyjohnston]

Would detachProps handle removing that listener? If so, would it make sense to instead call detachProps before node.remove and then remove this line?