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

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

Closed
miqmago opened this issue Jul 24, 2023 · 6 comments
Closed

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

miqmago opened this issue Jul 24, 2023 · 6 comments

Comments

@miqmago
Copy link

miqmago commented Jul 24, 2023

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
Copy link
Collaborator

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 pushed a commit to miqmago/capacitor-plugins-issue-1699 that referenced this issue Aug 24, 2023
@miqmago
Copy link
Author

miqmago commented Aug 24, 2023

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
Copy link
Member

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
ionic-team/capacitor#6938

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

@jcesarmobile jcesarmobile closed this as not planned Won't fix, can't repro, duplicate, stale Sep 26, 2023
@miqmago
Copy link
Author

miqmago commented Sep 27, 2023

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
Copy link
Member

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
Copy link

ionitron-bot bot commented Oct 13, 2023

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.

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

No branches or pull requests

3 participants