From 5a3b63bf7a040dd3fdae71628f162fffe59e5a20 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Mon, 9 Oct 2023 17:37:33 -0400 Subject: [PATCH 1/6] fix(react): lifecycle events are removed on page unmount Co-authored-by: Maria Hutt --- .../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 ea77c0c625c..da35a220afb 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 to avoid creating a new function when removing the + * event listeners. + */ + 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 8c12b460ac4..090c717ae13 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 to avoid creating a new function when removing the + * event listeners. + */ + 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); } } From 459b38cdb7df213545082381edd49e3f803a2f25 Mon Sep 17 00:00:00 2001 From: Sean Perkins <13732623+sean-perkins@users.noreply.github.com> Date: Wed, 11 Oct 2023 16:09:21 -0400 Subject: [PATCH 2/6] Update packages/react/src/routing/OutletPageManager.tsx Co-authored-by: Amanda Johnston <90629384+amandaejohnston@users.noreply.github.com> --- packages/react/src/routing/OutletPageManager.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/routing/OutletPageManager.tsx b/packages/react/src/routing/OutletPageManager.tsx index da35a220afb..0e0d1f9ea08 100644 --- a/packages/react/src/routing/OutletPageManager.tsx +++ b/packages/react/src/routing/OutletPageManager.tsx @@ -28,8 +28,8 @@ export class OutletPageManager extends React.Component { /** * 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 to avoid creating a new function when removing the - * event listeners. + * 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); From d5782c828ef24db1401687199871e7af42a19f8e Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 11 Oct 2023 16:10:13 -0400 Subject: [PATCH 3/6] chore: apply same comment to pagemanager --- packages/react/src/routing/PageManager.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/routing/PageManager.tsx b/packages/react/src/routing/PageManager.tsx index 090c717ae13..1ba3f94b98f 100644 --- a/packages/react/src/routing/PageManager.tsx +++ b/packages/react/src/routing/PageManager.tsx @@ -27,8 +27,8 @@ export class PageManager extends React.PureComponent { /** * 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 to avoid creating a new function when removing the - * event listeners. + * 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); From 251b4d30c7a67f19c0e1b934aea6d3244ef613b3 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Mon, 9 Oct 2023 19:23:31 -0400 Subject: [PATCH 4/6] fix(react): cleanup functions are execute for lifecycle hooks --- .../src/contexts/IonLifeCycleContext.tsx | 117 +++++++++++++++++- packages/react/src/lifecycle/hooks.ts | 12 ++ 2 files changed, 124 insertions(+), 5 deletions(-) diff --git a/packages/react/src/contexts/IonLifeCycleContext.tsx b/packages/react/src/contexts/IonLifeCycleContext.tsx index 6dc9d825509..f915884ceca 100644 --- a/packages/react/src/contexts/IonLifeCycleContext.tsx +++ b/packages/react/src/contexts/IonLifeCycleContext.tsx @@ -9,6 +9,10 @@ export interface IonLifeCycleContextInterface { ionViewWillLeave: () => void; onIonViewDidLeave: (callback: () => void) => void; ionViewDidLeave: () => void; + cleanupIonViewWillEnter: (callback: () => void) => void; + cleanupIonViewDidEnter: (callback: () => void) => void; + cleanupIonViewWillLeave: (callback: () => void) => void; + cleanupIonViewDidLeave: (callback: () => void) => void; } export const IonLifeCycleContext = /*@__PURE__*/ React.createContext({ @@ -36,19 +40,40 @@ export const IonLifeCycleContext = /*@__PURE__*/ React.createContext { return; }, + cleanupIonViewWillEnter: () => { + return; + }, + cleanupIonViewDidEnter: () => { + return; + }, + cleanupIonViewWillLeave: () => { + return; + }, + cleanupIonViewDidLeave: () => { + return; + }, }); export interface LifeCycleCallback { - (): void; + (): void | (() => void | undefined); id?: number; } +interface LifeCycleDestructor { + id: number; + destructor: ReturnType; +} + export const DefaultIonLifeCycleContext = class implements IonLifeCycleContextInterface { ionViewWillEnterCallbacks: LifeCycleCallback[] = []; ionViewDidEnterCallbacks: LifeCycleCallback[] = []; ionViewWillLeaveCallbacks: LifeCycleCallback[] = []; ionViewDidLeaveCallbacks: LifeCycleCallback[] = []; componentCanBeDestroyedCallback?: () => void; + ionViewWillEnterDestructorCallbacks: LifeCycleDestructor[] = []; + ionViewDidEnterDestructorCallbacks: LifeCycleDestructor[] = []; + ionViewWillLeaveDestructorCallbacks: LifeCycleDestructor[] = []; + ionViewDidLeaveDestructorCallbacks: LifeCycleDestructor[] = []; onIonViewWillEnter(callback: LifeCycleCallback) { if (callback.id) { @@ -63,8 +88,75 @@ export const DefaultIonLifeCycleContext = class implements IonLifeCycleContextIn } } + teardownCallback(callback: LifeCycleCallback, callbacks: any[]) { + // Find any destructors that have been registered for the callback + const matches = callbacks.filter((x) => x.id === callback.id); + if (matches.length !== 0) { + // Execute the destructor for each matching item + matches.forEach((match) => { + if (match && typeof match.destructor === 'function') { + match.destructor(); + } + }); + // Remove all matching items from the array + callbacks = callbacks.filter((x) => x.id !== callback.id); + } else { + /** + * If the destructor isn't registered in the array, then the + * user-provided callback was never invoked. This is usually + * the result of the useEffect hook in dev mode. So we manually + * invoke the callback to get the destructor to tear it down. + */ + const destructor = callback(); + if (typeof destructor === 'function') { + destructor(); + } + } + } + + /** + * Tears down the user-provided ionViewWillEnter lifecycle callback. + * This is the same behavior as React's useEffect hook. The callback + * is invoked when the component is unmounted. + */ + cleanupIonViewWillEnter(callback: LifeCycleCallback) { + this.teardownCallback(callback, this.ionViewWillEnterDestructorCallbacks); + } + + /** + * Tears down the user-provided ionViewDidEnter lifecycle callback. + * This is the same behavior as React's useEffect hook. The callback + * is invoked when the component is unmounted. + */ + cleanupIonViewDidEnter(callback: LifeCycleCallback) { + this.teardownCallback(callback, this.ionViewDidEnterDestructorCallbacks); + } + + /** + * Tears down the user-provided ionViewWillLeave lifecycle callback. + * This is the same behavior as React's useEffect hook. The callback + * is invoked when the component is unmounted. + */ + cleanupIonViewWillLeave(callback: LifeCycleCallback) { + this.teardownCallback(callback, this.ionViewWillLeaveDestructorCallbacks); + } + + /** + * Tears down the user-provided ionViewDidLeave lifecycle callback. + * This is the same behavior as React's useEffect hook. The callback + * is invoked when the component is unmounted. + */ + cleanupIonViewDidLeave(callback: LifeCycleCallback) { + this.teardownCallback(callback, this.ionViewDidLeaveDestructorCallbacks); + } + ionViewWillEnter() { - this.ionViewWillEnterCallbacks.forEach((cb) => cb()); + this.ionViewWillEnterCallbacks.forEach((cb) => { + const destructor = cb(); + if (cb.id) { + this.ionViewWillEnterDestructorCallbacks.push({ id: cb.id, destructor }); + } + }); } onIonViewDidEnter(callback: LifeCycleCallback) { @@ -81,7 +173,12 @@ export const DefaultIonLifeCycleContext = class implements IonLifeCycleContextIn } ionViewDidEnter() { - this.ionViewDidEnterCallbacks.forEach((cb) => cb()); + this.ionViewDidEnterCallbacks.forEach((cb) => { + const destructor = cb(); + if (cb.id) { + this.ionViewDidEnterDestructorCallbacks.push({ id: cb.id, destructor }); + } + }); } onIonViewWillLeave(callback: LifeCycleCallback) { @@ -98,7 +195,12 @@ export const DefaultIonLifeCycleContext = class implements IonLifeCycleContextIn } ionViewWillLeave() { - this.ionViewWillLeaveCallbacks.forEach((cb) => cb()); + this.ionViewWillLeaveCallbacks.forEach((cb) => { + const destructor = cb(); + if (cb.id) { + this.ionViewWillLeaveDestructorCallbacks.push({ id: cb.id, destructor }); + } + }); } onIonViewDidLeave(callback: LifeCycleCallback) { @@ -115,7 +217,12 @@ export const DefaultIonLifeCycleContext = class implements IonLifeCycleContextIn } ionViewDidLeave() { - this.ionViewDidLeaveCallbacks.forEach((cb) => cb()); + this.ionViewDidLeaveCallbacks.forEach((cb) => { + const destructor = cb(); + if (cb.id) { + this.ionViewDidLeaveDestructorCallbacks.push({ id: cb.id, destructor }); + } + }); this.componentCanBeDestroyed(); } diff --git a/packages/react/src/lifecycle/hooks.ts b/packages/react/src/lifecycle/hooks.ts index edc97280e93..3adc039d8e1 100644 --- a/packages/react/src/lifecycle/hooks.ts +++ b/packages/react/src/lifecycle/hooks.ts @@ -10,6 +10,9 @@ export const useIonViewWillEnter = (callback: LifeCycleCallback, deps: any[] = [ useEffect(() => { callback.id = id.current!; context.onIonViewWillEnter(callback); + return () => { + context.cleanupIonViewWillEnter(callback); + }; }, deps); }; @@ -20,6 +23,9 @@ export const useIonViewDidEnter = (callback: LifeCycleCallback, deps: any[] = [] useEffect(() => { callback.id = id.current!; context.onIonViewDidEnter(callback); + return () => { + context.cleanupIonViewDidEnter(callback); + }; }, deps); }; @@ -30,6 +36,9 @@ export const useIonViewWillLeave = (callback: LifeCycleCallback, deps: any[] = [ useEffect(() => { callback.id = id.current!; context.onIonViewWillLeave(callback); + return () => { + context.cleanupIonViewWillLeave(callback); + }; }, deps); }; @@ -40,5 +49,8 @@ export const useIonViewDidLeave = (callback: LifeCycleCallback, deps: any[] = [] useEffect(() => { callback.id = id.current!; context.onIonViewDidLeave(callback); + return () => { + context.cleanupIonViewDidLeave(callback); + }; }, deps); }; From a66b1747b00cbf1467149af6324ba93f4f575645 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Mon, 9 Oct 2023 19:49:25 -0400 Subject: [PATCH 5/6] chore: export interface --- packages/react/src/contexts/IonLifeCycleContext.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/contexts/IonLifeCycleContext.tsx b/packages/react/src/contexts/IonLifeCycleContext.tsx index f915884ceca..f5adc61050f 100644 --- a/packages/react/src/contexts/IonLifeCycleContext.tsx +++ b/packages/react/src/contexts/IonLifeCycleContext.tsx @@ -59,7 +59,7 @@ export interface LifeCycleCallback { id?: number; } -interface LifeCycleDestructor { +export interface LifeCycleDestructor { id: number; destructor: ReturnType; } From 7eb58bd953b7a2505a0d3ad683c4671815a3a3fc Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Tue, 10 Oct 2023 12:40:10 -0400 Subject: [PATCH 6/6] chore: avoid unnecessary invoke of callback --- packages/react/src/contexts/IonLifeCycleContext.tsx | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/packages/react/src/contexts/IonLifeCycleContext.tsx b/packages/react/src/contexts/IonLifeCycleContext.tsx index f5adc61050f..c9343bf5b66 100644 --- a/packages/react/src/contexts/IonLifeCycleContext.tsx +++ b/packages/react/src/contexts/IonLifeCycleContext.tsx @@ -100,17 +100,6 @@ export const DefaultIonLifeCycleContext = class implements IonLifeCycleContextIn }); // Remove all matching items from the array callbacks = callbacks.filter((x) => x.id !== callback.id); - } else { - /** - * If the destructor isn't registered in the array, then the - * user-provided callback was never invoked. This is usually - * the result of the useEffect hook in dev mode. So we manually - * invoke the callback to get the destructor to tear it down. - */ - const destructor = callback(); - if (typeof destructor === 'function') { - destructor(); - } } }