ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.98k stars 13.52k forks source link

Ionic lazy loading breaks LocalNotifications native plugin #13525

Closed gregd closed 5 years ago

gregd commented 6 years ago

Ionic version: (check one with "x") [ ] 2.x [x] 3.x [ ] 4.x

I'm submitting a ... (check one with "x") [x] bug report [ ] feature request

Current behavior: After app boot click and other callbacks aren't called. localNotifications.on("click", (notification: ILocalNotification) => { ... });

Expected behavior: Registered click handler is called as expected.

Steps to reproduce:

  1. Create a standard tabs app
  2. Add Cordova android platfrom
  3. Add LocalNotifications plugin http://ionicframework.com/docs/native/local-notifications/
  4. Add a button with click handler which will add a notification like this
    this.localNotifications.schedule({
      id: 123,
      at: new Date(new Date().getTime() + 1000 * 60),
      title: "notification title",
      text: `abc 123`
    });
  5. Log a message when the notification is clicked

    constructor(platform: Platform, statusBar: StatusBar, splashScreen: SplashScreen, notifications: LocalNotifications) {
    console.log("MyApp constructor");
    platform.ready().then(() => {
      console.log("MyApp platform ready");
      statusBar.styleDefault();
      setTimeout(() => { splashScreen.hide(); }, 1);
    
      notifications.on("click", (notification: ILocalNotification) => {
        console.log("MyApp notification click " + notification.id);
        // other actions
      });
    });
    }
  6. Run the app, schedule the notification, go to Android home screen, kill the app (the Recent apps screen plus x button) , wait 60 seconds and click the notification. The app will boot and the on click handler will log the message "MyApp notification click 123". So far so good.
  7. Add Ionic Lazy Loading to all pages as described in docs, something like this
    rootPage:any = "TabsPage";
    ...
    @IonicPage()
    ...
    @NgModule({
    declarations: [
    TabsPage,
    ],
    imports: [
    IonicPageModule.forChild(TabsPage),
    ],
    entryComponents: [
    TabsPage
    ]})
    export class TabsModule {}
  8. Repeat step 6, the app will boot correctly but the notification on click handler won't be called. And that's the bug.

Ionic lazy loading code somehow breaks Cordova deviceready callback order. LocalNotifications has this code

// Called after 'deviceready' event
channel.deviceready.subscribe(function () {
    // Device is ready now, the listeners are registered
    // and all queued events can be executed.
    exec(null, null, 'LocalNotification', 'deviceready', []);
});

which triggers callbacks like the click to fire. However Ionic platform.ready().then(() => { ... } isn't executed and the click notification isn't registered. It seems that lazy loading delays execution of platform ready callback and in this way breaks native plugins like LocalNotifications.

kensodemann commented 6 years ago

Hello! Thank you for opening an issue with us! Would you be able to provide a sample application via GitHub that demonstrates the issue you are having? Thanks for using Ionic!

gregd commented 6 years ago

Here is the app https://github.com/gregd/ionic-bug-report-1 Checkout 7b3af860f426f5c9c132437d2014446b06a5bc63 to see the correct behavior. Checkout master to see how lazy loading affects Cordova callbacks and the bug.

ThorvaldAagaard commented 6 years ago

I have investigated the problem, and the reason is that you are loading two different instances of LocalNotifications. In home.ts where you create the notification, and another instance in app.component.ts, so when the notification is clicked, the listener is never activated.

I am not sure how the proper way of loading the same instance in both modules are supposed to be, but I found the following solution:

In both modules I instatiate LocalNotifications with this

 this.notification = AppModule.injector.get(LocalNotifications);

insted of wiring it up in the constructor.

To get this to work you have to add

export class AppModule {
  // Allows for retrieving singletons using 'AppModule.injector.get(MyService)' later in the code

  static injector: Injector;

  constructor(injector: Injector) {
    AppModule.injector = injector;
  }
}

in app.module.ts

So I think the proper solution to this is to change the lazy loading so the same instance can be loaded in multiple modules

ionitron-bot[bot] commented 5 years ago

Thanks for the issue! We have moved the source code and issues for Ionic 3 into a separate repository. I am moving this issue to the repository for Ionic 3. Please track this issue over there.

Thank you for using Ionic!

ionitron-bot[bot] commented 5 years ago

Issue moved to: https://github.com/ionic-team/ionic-v3/issues/840