bug: ion-loading race condition when toggled via bound prop

[troygilbert]

Bug Report

Ionic version:
[ ] 4.x
[x] 5.x

Current behavior:
If you show/hide an <ion-loading> element too quickly you will orphan the DOM element and a new one will be created (the old one will never go away).

Expected behavior:
The component should gracefully handle showing/hiding via a bound prop.

Steps to reproduce:
Use ionic start to create an empty starter project, select Vue, then replace Home.vue with:

<template>
  <ion-page>
    <ion-content>
      <ion-loading :is-open="loadingMessage" :message="loadingMessage"></ion-loading>
      <ion-button @click="startLoading">Start Loading</ion-button>
    </ion-content>
  </ion-page>
</template>

<script lang="ts">
  import { IonContent, IonPage, IonButton, IonLoading } from '@ionic/vue';
  import { defineComponent } from 'vue';

  export default defineComponent({
    name: 'Home',
    components: { IonContent, IonPage, IonButton, IonLoading },
    data() {
      return {
        loadingMessage: '',
      };
    },
    methods: {
      startLoading() {
        this.loadingMessage = 'A really long loading message so you can see it in the background...';
        setTimeout(() => {
          this.loadingMessage = '';
          setTimeout(() => {
            this.loadingMessage = 'Loading again...';
          });
        });
      },
    },
  });
</script>

Ionic info:

Ionic:

   Ionic CLI       : 6.12.3 (/Users/troygilbert/.nvm/versions/node/v14.15.2/lib/node_modules/@ionic/cli)
   Ionic Framework : @ionic/vue 5.5.4

Capacitor:

   Capacitor CLI   : 2.4.6
   @capacitor/core : 2.4.6

Utility:

   cordova-res (update available: 0.15.3) : 0.15.2
   native-run                             : 1.3.0

System:

   NodeJS : v14.15.2 (/Users/troygilbert/.nvm/versions/node/v14.15.2/bin/node)
   npm    : 6.14.10
   OS     : macOS Big Sur

[liamdebeasi]

Thanks for the issue. So there are a few things going on here:

1. The isOpen prop accepts a boolean (such as ref(true)). Because non-empty string values are truthy, a loading dialog will always attempt to open even when you set loadingMessage to the empty string (see: bug: ion-loading race condition when toggled via bound prop #22880 (comment)). To fix this, you should be passing in a ref with a boolean value instead to open/close the loading dialog.

2. Overlay dialogs such as ion-loading are not currently reactive, so updating message several times in a row normally should not do anything after setting the first value.

3. Internally, Ionic Vue is not checking the previous values of the props which is why multiple loading components are even allowed to open on the same instance in the first place.

I made a dev build that fixes the 3rd issue if you would like to give that a try after updating your app for the first two issues:

npm install @ionic/vue@5.6.0-dev.202102051547.3ede130 @ionic/vue-router@5.6.0-dev.202102051547.3ede130

[troygilbert]

I'll check out the dev build. The other two issues I'm aware of, the code was written that way just to demonstrate the issue in the absolute minimal amount of code. I was not expecting the message to be reactive. And in my production code I was using a separate boolean value, but that being said, an empty string is not normally truthy in Javascript, is that a side-effect of the reactive wrapper?

[liamdebeasi]

Yes you are correct, sorry! I am not fully awake yet 😄 . The wrapper should work fine, though if you changed the message string from 'test1' to 'test2' I suppose you could run into issues which is why we recommend using a boolean.

[troygilbert]

Progress! Race condition still exists, but it manifests differently. Now it doesn't create two loaders, but the loader isn't closed then re-opened with the new message. The reason I call it a race condition is that if I change those timeouts to be something like 500ms (instead of 0ms), it does the expected behavior of showing, hiding, then showing again with a new message. Here's an updated version of Home.vue:

<template>
  <ion-page>
    <ion-content>
      <ion-loading :is-open="isLoading" :message="loadingMessage"></ion-loading>
      <ion-button @click="startLoading">Start Loading</ion-button>
    </ion-content>
  </ion-page>
</template>

<script lang="ts">
  import { IonContent, IonPage, IonButton, IonLoading } from '@ionic/vue';
  import { defineComponent } from 'vue';

  export default defineComponent({
    name: 'Home',
    components: { IonContent, IonPage, IonButton, IonLoading },
    data() {
      return {
        isLoading: false,
        loadingMessage: '',
      };
    },
    methods: {
      startLoading() {
        const delay = 0;
        this.isLoading = true;
        this.loadingMessage = 'A really long loading message so you can see it in the background...';
        setTimeout(() => {
          this.isLoading = false;
          setTimeout(() => {
            this.isLoading = true;
            this.loadingMessage = 'Loading again...';
          }, delay);
        }, delay);
      },
    },
  });
</script>

As you adjust the delay constant from 0 to 500ms you'll see that the behavior changes. I would expect the component's behavior to not be affected by the amount of time between the delays. It's particularly critical with the loading indicator, because if it is displayed then dismissed in less than 200ms (what I encountered in my production app) then it doesn't dismiss!

[liamdebeasi]

Thanks for testing. So I think this is one of those scenarios where it's better to use the loadingController module directly rather than use the ion-loading component.

The issue here is that each overlay component (modal, loading, popover, etc) is created asynchronously, so when you try to present and immediately dismiss the loading component, the dismiss will not happen because the underlying loading component has not even been created yet. <ion-loading> merely acts as a wrapper that triggers the creation of the overlay loading screen.

By using the loadingController import directly, you can ensure that the component is created before firing off requests that may result in the loader being dismissed:

import { loadingController } from '@ionic/vue';

...

const startLoading = async () => {
  // As long as you await this line, you can dismiss as soon as you want.
  const loading = await loadingController.create({ message: 'My Message' });
  await loading.present();
}

I will definitely ensure that the issue with orphaned overlays is resolved, but I am not sure that these components need to effectively queue up present/dismiss requests. Can you give the loadingController import a try and let me know if that resolves the issue?

[troygilbert]

I understand the async issues, and I appreciate, like any race condition, that it's difficult to solve cleanly and concisely. But if the framework is going to provide the component and have it triggered by a reactive prop, it's a bug as it stands now. I've not written any Vue components that have animated transitions, etc., but I'd assume (maybe incorrectly) that this is a common issue that has to be solved.

At a bare minimum, the potential of this issue needs to be called out clearly in the documentation for all the overlays as long as their wrappers are provided. I was fortunate when I encountered it: the process that triggered the loading and dismiss were completely within my control, and I could reproduce it consistently. If it had just been a raw network call that happened to complete faster than 200ms sometimes, I could've chased this intermittent bug for who knows how long before pinpointing the issue. (Even in my situation, I chased it for at least a whole day!)

Using the loadingController directly is a non-trivial solution to implement, as it will require everywhere in my codebase where it's called to be async-aware, which is a far cry from it's current fire-and-forget reactive prop. Bottom line, if Ionic doesn't implement a solution to the race condition inherent to the component, I will have to implement a solution. (Which I've done now by simply putting in an artificial delay in the one part of the code exhibiting the issue, but it's just a matter of time before a caching layer or other API returns a result in less than 200ms!)

[liamdebeasi]

I agree that we need to spell this out more clearly in the docs, but I would argue that your codebase should be async-aware when using these types of components. All of these components, even if they happened to be created synchronously, have animations that that take ~300ms so there is some level of async here. Having information on when to use a component vs when to use the controller I think would be helpful here.

If it had just been a raw network call that happened to complete faster than 200ms sometimes, I could've chased this intermittent bug for who knows how long before pinpointing the issue. (Even in my situation, I chased it for at least a whole day!)

This is a scenario where using the loadingController would make things a lot simpler:

const loading = await loadingController.create({ message: 'My Message' });
await loading.present();

// Fire off the network request
// Wait for response
// Process results, etc

await loading.dismiss();

To be fair, I do not see a huge value in using <ion-loading> in-template (for reasons you explained above), but the feature exists since all the other overlay components can be used via controller or in-template. Right now, our overlay components are not setup to queue commands. That could be an avenue for future improvements but we do not have this planned at the moment.

I will work on resolving the initial node orphaning issue and will close the issue out once the code has been merged.

[liamdebeasi]

I made some progress! Internally, we can handle waiting for any other present/dismiss operations to complete (without a queue). This likely will not work for all use cases, but for your use case it should be fine.

npm install @ionic/vue@5.6.0-dev.202102092029.cc5c8c6 @ionic/vue-router@5.6.0-dev.202102092029.cc5c8c6

[troygilbert]

Fantastic! That fixes it!