ionic-team / capacitor-plugins

Official plugins for Capacitor ⚡️
519 stars 585 forks source link

On web plugins, `adListener` only the last listener is added #1699

Closed miqmago closed 1 year ago

miqmago commented 1 year ago

Bug Report

When creating a capacitor plugin, some listeners don't fire. Multiple instances of the plugin are created each time addListener is called. The problem I think is with this pattern published in https://capacitorjs.com/docs/plugins/tutorial/web and also used in many plugins:

const ScreenOrientation = registerPlugin<ScreenOrientationPlugin>(
  'ScreenOrientation',
  {
    web: () => import('./web').then(m => new m.ScreenOrientationWeb()),
  },
);

In plugins that are first called on async componentWillLoad, it might create a new implementation of the plugin every time registerPlugin is called. I'm not sure why registerPlugin is called many times, but the fact is that it is. It might be by the async but I'm not sure.

The problem is that when attaching listeners, the listener will be added to any new instance of the plugin and other instances does not have the listener.

So for example, for LocalNotifications, if you have something like this when the app first starts (have tried without the maybeWeirdAwaitCouldAppearHere and behaviour is the same, but it could also happen):

export class AppRoot {
    async componentWillLoad() {
        await someMethod();

        LocalNotifications.addListener('localNotificationActionPerformed', (action) => {
            console.log('localNotificationActionPerformed', action);
        });

        await maybeWeirdAwaitCouldAppearHere();

        LocalNotifications.addListener('localNotificationReceived', (notif) => {
            console.log('localNotificationReceived', notif);
        });
    }
}

LocalNotifications will only have localNotificationReceived event attached as it is the last one. No calls to localNotificationActionPerformed will be received, as the second instance was a new instance without the first listener attached.

Possible solution

The solution I've found on the plugins I've developed is to change little bit the implementation of registration:

let ScreenOrientationWeb: any;

const ScreenOrientation = registerPlugin<ScreenOrientationPlugin>('ScreenOrientation', {
    web: () => import('./web').then(m => {
        if (!ScreenOrientationWeb) {
            ScreenOrientationWeb = new m.ScreenOrientationWeb()
        }
        return ScreenOrientationWeb;
    }),
});

I've also tried with LocalNotifications and it works fine. If this is the solution, it should also be posted on documentation: https://capacitorjs.com/docs/plugins/tutorial/web

Also all plugins using this registerPlugin pattern should be fixed.

Plugin(s)

So far I've seen this behaviour in @capacitor/local-notifications and in some plugins that I've designed myself.

I suspect that it affects to any plugin developed with web registerPlugin mentioned pattern.

Capacitor Version

💊   Capacitor Doctor  💊 

Latest Dependencies:

  @capacitor/cli: 5.2.2
  @capacitor/core: 5.2.2
  @capacitor/android: 5.2.2
  @capacitor/ios: 5.2.2

Installed Dependencies:

  @capacitor/cli: 5.2.1
  @capacitor/android: 5.2.1
  @capacitor/core: 5.2.1
  @capacitor/ios: 5.2.1

Platform(s)

Web

Ionitron commented 1 year ago

This issue needs more information before it can be addressed. In particular, the reporter needs to provide a minimal sample app that demonstrates the issue. If no sample app is provided within 15 days, the issue will be closed.

Please see the Contributing Guide for how to create a Sample App.

Thanks! Ionitron 💙

miqmago commented 1 year ago

please find the demo repo at https://github.com/miqmago/capacitor-plugins-issue-1699. This is a pure capacitor starter app with following changes:

npm init @capacitor/app -- my-app --app-id com.capacitor.plugin.issue.1699 --name capacitor-plugin-issue
cd my-app
npm i
npm install @capacitor/local-notifications
npm start

Then in src/js/capacitor-welcome.js

 import { SplashScreen } from '@capacitor/splash-screen';
 import { Camera } from '@capacitor/camera';
+import { LocalNotifications } from '@capacitor/local-notifications';
+
+const IS_WORKING_CASE = false;

 window.customElements.define(
   'capacitor-welcome',
@@ -89,7 +92,7 @@ window.customElements.define(
     `;
     }

-    connectedCallback() {
+    async connectedCallback() {
       const self = this;

       self.shadowRoot.querySelector('#take-photo').addEventListener('click', async function (e) {
@@ -108,6 +111,35 @@ window.customElements.define(
           console.warn('User cancelled', e);
         }
       });
+
+
+      LocalNotifications.addListener('localNotificationActionPerformed', async () => {
+        console.log('localNotificationActionPerformed');
+      });
+  
+      if (IS_WORKING_CASE) {
+        await new Promise(resolve => setTimeout(resolve, 1000));
+      }
+  
+      LocalNotifications.addListener('localNotificationReceived', () => {
+        console.log('localNotificationReceived');
+      });
+  
+      if (IS_WORKING_CASE) {
+        await new Promise(resolve => setTimeout(resolve, 1000));
+      }
+  
+      LocalNotifications.schedule({
+        notifications: [{
+          title: 'Hey',
+          body: 'Test notification',
+          id: new Date().getTime(),
+          schedule: {
+            at: new Date(new Date().getTime() + 1000),
+          },
+          actionTypeId: 'received',
+        }],
+      });
     }
   }
 );

In this line there is a variable to make it work or to make it fail.

When IS_WORKING_CASE is set to true, console will display the logs for the events, meaning that the listeners have been successfully attached, when set to false, no console logs will be shown.

Tested in Chrome 116.0.5845.96 (Compilació oficial) (arm64).

jcesarmobile commented 1 year ago

Thanks for the issue, I've verified it and created a new issue in capacitor repository since I think the problem is in @capacitor/core. While it could be possible to fix it plugin per plugin, I think we should fix it in core so plugins don't need to be updated. So going to close this one, if you are interested, subscribe to the new issue https://github.com/ionic-team/capacitor/issues/6938

As workaround, you can use the plugins with await (as documented), when using await the issue is not present.

miqmago commented 1 year ago

Hi @jcesarmobile thanks for reporting on the right repo!!

Just to clarify, I'm not sure to understand how to use await on the plugin, could you point me to the documentation?

Do you mean to place await on addListener? In the following code the behaviour is the same, the first listener will never be fired and I'm not sure of where else should be used the await:

export class AppRoot {
    async componentWillLoad() {
        await someMethod();

       LocalNotifications.addListener('localNotificationActionPerformed', (action) => {
            console.log('localNotificationActionPerformed', action);
        });

        await maybeWeirdAwaitCouldAppearHere();

        LocalNotifications.addListener('localNotificationReceived', (notif) => {
            console.log('localNotificationReceived', notif);
        });
    }
}
jcesarmobile commented 1 year ago

putting an await before every plugin call

await LocalNotifications.addListener('localNotificationActionPerformed', (action) => {
            console.log('localNotificationActionPerformed', action);
        });

await LocalNotifications.addListener('localNotificationReceived', (notif) => {
            console.log('localNotificationReceived', notif);
        })
ionitron-bot[bot] commented 1 year ago

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 the plugin, please create a new issue and ensure the template is fully filled out.