From f14a59c5e0670ed7cc9ce1a73a087a5af13266e2 Mon Sep 17 00:00:00 2001 From: Sean Perkins <13732623+sean-perkins@users.noreply.github.com> Date: Thu, 12 Oct 2023 11:02:21 -0400 Subject: [PATCH] fix(react): lifecycle events are removed on page unmount (#28316) Issue number: N/A --------- ## What is the current behavior? While debugging #28186, Maria and I identified that Ionic's lifecycle event listeners (`ionViewWillEnter`, etc.) were being registered multiple times on the same `.ion-page` element. This resulted in problematic behavior, where a user's implementation of our lifecycle hooks, would execute their callback multiple times. ```ts useIonViewWillEnter(() => { // This is called 2x for every time the `ionViewWillEnter` event is emitted (in React 18, dev mode) console.log('hello world'); }); ``` When the Ionic lifecycle event listeners are registered in React, we bind the scope of the class to the callback function. When removing the event listener we additional use the `.bind` syntax. ```tsx componentDidMount() { element.addEventListener('ionViewWillEnter', this.ionViewWillEnter.bind(this)); } componentWillUnmount() { // This creates a new instance of the function to remove! It doesn't remove the original event listener. element.removeEventListener('ionViewWillEnter', this.ionViewWillEnter.bind(this)); } ``` The `.bind` method returns a new instance of the function. This means in the implementation we are creating a new instance of the function when both adding and removing the event listener - resulting in the `removeEventListener` to never remove the original event listener. This behavior only occurred in React 18 in dev mode, as a result of the mount/unmount behavior running 2x for `useEffect` hooks. ## What is the new behavior? - Ionic lifecycle event listeners are removed from element references when they are unmounted. - User's lifecycle callback methods are only invoked once per event emission. |Before|After| |----|----| |CleanShot 2023-10-09 at 18 32 08@2x|CleanShot 2023-10-09 at 18 29 37@2x| ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information --------- Co-authored-by: Maria Hutt Co-authored-by: Amanda Johnston <90629384+amandaejohnston@users.noreply.github.com> --- .../react/src/routing/OutletPageManager.tsx | 27 +++++++++++++------ packages/react/src/routing/PageManager.tsx | 27 +++++++++++++------ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/packages/react/src/routing/OutletPageManager.tsx b/packages/react/src/routing/OutletPageManager.tsx index ea77c0c625..0e0d1f9ea0 100644 --- a/packages/react/src/routing/OutletPageManager.tsx +++ b/packages/react/src/routing/OutletPageManager.tsx @@ -24,6 +24,17 @@ export class OutletPageManager extends React.Component { super(props); this.outletIsReady = false; + + /** + * This binds the scope of the following methods to the class scope. + * The `.bind` method returns a new function, so we need to assign it + * in the constructor rather than when adding or removing the listeners + * to avoid creating a new function. + */ + this.ionViewWillEnterHandler = this.ionViewWillEnterHandler.bind(this); + this.ionViewDidEnterHandler = this.ionViewDidEnterHandler.bind(this); + this.ionViewWillLeaveHandler = this.ionViewWillLeaveHandler.bind(this); + this.ionViewDidLeaveHandler = this.ionViewDidLeaveHandler.bind(this); } componentDidMount() { @@ -39,19 +50,19 @@ export class OutletPageManager extends React.Component { }); } - this.ionRouterOutlet.addEventListener('ionViewWillEnter', this.ionViewWillEnterHandler.bind(this)); - this.ionRouterOutlet.addEventListener('ionViewDidEnter', this.ionViewDidEnterHandler.bind(this)); - this.ionRouterOutlet.addEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler.bind(this)); - this.ionRouterOutlet.addEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler.bind(this)); + this.ionRouterOutlet.addEventListener('ionViewWillEnter', this.ionViewWillEnterHandler); + this.ionRouterOutlet.addEventListener('ionViewDidEnter', this.ionViewDidEnterHandler); + this.ionRouterOutlet.addEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler); + this.ionRouterOutlet.addEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler); } } componentWillUnmount() { if (this.ionRouterOutlet) { - this.ionRouterOutlet.removeEventListener('ionViewWillEnter', this.ionViewWillEnterHandler.bind(this)); - this.ionRouterOutlet.removeEventListener('ionViewDidEnter', this.ionViewDidEnterHandler.bind(this)); - this.ionRouterOutlet.removeEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler.bind(this)); - this.ionRouterOutlet.removeEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler.bind(this)); + this.ionRouterOutlet.removeEventListener('ionViewWillEnter', this.ionViewWillEnterHandler); + this.ionRouterOutlet.removeEventListener('ionViewDidEnter', this.ionViewDidEnterHandler); + this.ionRouterOutlet.removeEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler); + this.ionRouterOutlet.removeEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler); } } diff --git a/packages/react/src/routing/PageManager.tsx b/packages/react/src/routing/PageManager.tsx index 8c12b460ac..1ba3f94b98 100644 --- a/packages/react/src/routing/PageManager.tsx +++ b/packages/react/src/routing/PageManager.tsx @@ -23,6 +23,17 @@ export class PageManager extends React.PureComponent { this.ionPageElementRef = React.createRef(); // React refs must be stable (not created inline). this.stableMergedRefs = mergeRefs(this.ionPageElementRef, this.props.forwardedRef); + + /** + * This binds the scope of the following methods to the class scope. + * The `.bind` method returns a new function, so we need to assign it + * in the constructor rather than when adding or removing the listeners + * to avoid creating a new function. + */ + this.ionViewWillEnterHandler = this.ionViewWillEnterHandler.bind(this); + this.ionViewDidEnterHandler = this.ionViewDidEnterHandler.bind(this); + this.ionViewWillLeaveHandler = this.ionViewWillLeaveHandler.bind(this); + this.ionViewDidLeaveHandler = this.ionViewDidLeaveHandler.bind(this); } componentDidMount() { @@ -31,19 +42,19 @@ export class PageManager extends React.PureComponent { this.ionPageElementRef.current.classList.add('ion-page-invisible'); } this.context.registerIonPage(this.ionPageElementRef.current, this.props.routeInfo!); - this.ionPageElementRef.current.addEventListener('ionViewWillEnter', this.ionViewWillEnterHandler.bind(this)); - this.ionPageElementRef.current.addEventListener('ionViewDidEnter', this.ionViewDidEnterHandler.bind(this)); - this.ionPageElementRef.current.addEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler.bind(this)); - this.ionPageElementRef.current.addEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler.bind(this)); + this.ionPageElementRef.current.addEventListener('ionViewWillEnter', this.ionViewWillEnterHandler); + this.ionPageElementRef.current.addEventListener('ionViewDidEnter', this.ionViewDidEnterHandler); + this.ionPageElementRef.current.addEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler); + this.ionPageElementRef.current.addEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler); } } componentWillUnmount() { if (this.ionPageElementRef.current) { - this.ionPageElementRef.current.removeEventListener('ionViewWillEnter', this.ionViewWillEnterHandler.bind(this)); - this.ionPageElementRef.current.removeEventListener('ionViewDidEnter', this.ionViewDidEnterHandler.bind(this)); - this.ionPageElementRef.current.removeEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler.bind(this)); - this.ionPageElementRef.current.removeEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler.bind(this)); + this.ionPageElementRef.current.removeEventListener('ionViewWillEnter', this.ionViewWillEnterHandler); + this.ionPageElementRef.current.removeEventListener('ionViewDidEnter', this.ionViewDidEnterHandler); + this.ionPageElementRef.current.removeEventListener('ionViewWillLeave', this.ionViewWillLeaveHandler); + this.ionPageElementRef.current.removeEventListener('ionViewDidLeave', this.ionViewDidLeaveHandler); } }