Skip to content

fix(react): cleanup functions are execute for lifecycle hooks - #28319

Merged
sean-perkins merged 6 commits into
mainfrom
sp/FW-5340
Oct 12, 2023
Merged

fix(react): cleanup functions are execute for lifecycle hooks#28319
sean-perkins merged 6 commits into
mainfrom
sp/FW-5340

Conversation

@sean-perkins

@sean-perkins sean-perkins commented Oct 10, 2023

Copy link
Copy Markdown
Contributor

Issue number: Resolves #28186


What is the current behavior?

Ionic lifecycle hooks do not execute a cleanup function when the underlying useEffect is unmounted.

useEffect(() => {
  return () => {
     console.log('cleanup'); // called
   };
});

useIonViewWillEnter(() => {
  return () => {
     console.log('cleanup'); // never called
  };
});

Ionic's implementation registers the lifecycle callback to be handled at a later time, by the page managers. However, it does not keep a reference to the returned callback, so it cannot execute it when the useEffect is unmounted.

What is the new behavior?

  • Ionic lifecycle hooks execute dev-specified cleanup functions

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev-build: 7.4.4-dev.11696956070.1faa3cfe

This PR builds on the changes in #28316.

Co-authored-by: Maria Hutt <maria@ionic.io>
@github-actions github-actions Bot added the package: react @ionic/react package label Oct 10, 2023
Comment thread packages/react/src/contexts/IonLifeCycleContext.tsx Outdated
@sean-perkins
sean-perkins marked this pull request as ready for review October 10, 2023 17:11
@sean-perkins
sean-perkins requested a review from thetaPC October 10, 2023 17:11
@sean-perkins sean-perkins changed the title fix(react): lifecycle events are removed on page unmount fix(react): cleanup functions are execute for lifecycle hooks Oct 10, 2023
Comment on lines +144 to +145
const destructor = cb();
if (cb.id) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to create the variable, destructor inside the if statement? That way there isn't an unused variable if cb.id doesn't exist.

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following up on why I did this - took me a second to recall.

The callback may not have an id, but we still want to invoke the callback regardless. Otherwise the developers implementation within the lifecycle hook will never fire.

We only want to store the destructor callback if the callback has an id - which is assigned from the lifecycle hooks.

This was intentional.

@sean-perkins
sean-perkins requested a review from thetaPC October 12, 2023 16:14

@thetaPC thetaPC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@sean-perkins
sean-perkins added this pull request to the merge queue Oct 12, 2023
Merged via the queue into main with commit 1ba9973 Oct 12, 2023
@sean-perkins
sean-perkins deleted the sp/FW-5340 branch October 12, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: react @ionic/react package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Ionic React lifecycle hooks should execute cleanup function when unmounted

2 participants