Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: ion-loading is not removed from dom if never presented #28981

Closed
3 tasks done
aparajita opened this issue Feb 5, 2024 · 4 comments
Closed
3 tasks done

bug: ion-loading is not removed from dom if never presented #28981

aparajita opened this issue Feb 5, 2024 · 4 comments
Assignees
Labels

Comments

@aparajita
Copy link

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

Hey @liamdebeasi, when I dismiss an ion-loading created by a controller that has never been presented, it is not removed from the dom, leading to a fairly large memory leak.

Expected Behavior

One of two fixes:

  • When an ion-loading is dismissed, it should be removed from the dom. This is the ideal.
  • Clearly document that if present() is not called after loadingController.create(), dismiss() will be a no-op and there will be a memory leak.

Steps to Reproduce

  1. Create an ion-loading with loadingController.create(). An ion-loading is added to the dom.
  2. Do not call present().
  3. Call dismiss(). The ion-loading is not removed from the dom.

Code Reproduction URL

https://stackblitz.com/edit/a1fkix?file=src%2Fcomponents%2FExample.vue

Ionic Info

Ionic:

Ionic CLI : 7.2.0 (/monorepo/node_modules/.pnpm/@Ionic+cli@7.2.0/node_modules/@ionic/cli)

Capacitor:

Capacitor CLI : 5.6.0
@capacitor/android : 5.6.0 (/monorepo/node_modules/.pnpm/@capacitor+android@5.6.0_@capacitor+core@5.6.0/node_modules/@capacitor/android)
@capacitor/core : 5.6.0 (/monorepo/node_modules/.pnpm/@capacitor+core@5.6.0/node_modules/@capacitor/core)
@capacitor/ios : 5.6.0 (/monorepo/node_modules/.pnpm/@capacitor+ios@5.6.0_@capacitor+core@5.6.0/node_modules/@capacitor/ios)

Utility:

cordova-res : not installed globally
native-run : not installed globally

System:

NodeJS : v21.4.0 (/path/to/Library/pnpm/nodejs/21.4.0/bin/node)
npm : 10.2.4
OS : macOS Unknown

Additional Information

Here's the context:

When loading data from the back end, I call a useLoading function that does not display the ion-loading until a delay has elapsed, since there is no point in showing the loader if the data comes back quickly.

Currently I am creating the ion-loading and then only presenting it if the delay elapses. I can obviously work around the current behavior by changing my code so that the ion-loading is not creating until after the delay has elapsed. Probably a better approach anyway, since there is no point in executing all of the code to create the ion-loading, and updating the dom, if it will not be displayed.

@liamdebeasi
Copy link
Contributor

Have you tried calling loading.remove() instead of .dismiss()? This will explicitly remove the element from the DOM. Dismissing an overlay means to move the overlay from an open to closed state. Since the overlay is already in the closed state, nothing happens.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Feb 6, 2024
@ionitron-bot ionitron-bot bot removed the triage label Feb 6, 2024
@aparajita
Copy link
Author

Have you tried calling loading.remove() instead of .dismiss()?

No, that method is not documented, so I had no idea it existed. In any case I reworked my code to not create the loading until it is going to be presented. But perhaps the documentation should be updated to cover the issue I ran into and to surface the remove() method.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Feb 6, 2024
@liamdebeasi
Copy link
Contributor

remove is a browser API available on any Element so all Ionic components (and regular elements such as a <div>) automatically have access to this.

I'm going to close this since Ionic is working as intended here. However, I'll update the documentation to note this in the dismiss docs since I think it is helpful to call it out.

@liamdebeasi liamdebeasi closed this as not planned Won't fix, can't repro, duplicate, stale Feb 7, 2024
github-merge-queue bot pushed a commit that referenced this issue Feb 8, 2024
In #28981 there was
some confusion surrounding how to remove an overlay from the DOM if it
was never presented. The `dismiss` method will remove the overlay from
the DOM, but only if the overlay is visible. Otherwise, it's a no-op.

This PR updates the `dismiss` method docs for each overlay component to
note that developers can use the browser's remove method to remove the
element from the DOM.
Copy link

ionitron-bot bot commented Mar 8, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants