microsoft / cordova-plugin-code-push

Cordova plugin for CodePush
http://appcenter.ms
Other
643 stars 332 forks source link

codePush.sync() unreliable when calling multiple times with different InstallMode #309

Closed christophmegusta closed 6 years ago

christophmegusta commented 6 years ago

what we do (same logic for mandatory and regular updates):

current behaviour: when we do sync() with nextresume and then immediate later due to manual button execution, it just does not work immediate. the refresh does not come. also sometimes its not installing any updates for a couple of app-close-reopens and process kills + cold starts. it seams that sync() remembers for a specific update with what parameter/installmode it has been started and will keep it through until the update is installed.

expected behaviour: sync() works after logic from last called parameter. so if i start update with nextresume, then call sync again during the process with immediate, it does do the restart.

or do we use it completely wrong? anything we need to respect?

EDIT: maybe our wrapper for the update logic could break it too. what we want to do on app start is to have the whole update to be installed, from fetching to app restart until it is finally installed. ALSO the splashscreen should be in foreground all the time while update is doing. we have this early in our bootstrap logic() so we do any other app logic only when the promise resolves.

// this is part of our application class
updateApp(installNow = false) { return new Promise((resolve,reject)=> {
    if(typeof codePush !== "undefined") {
      const syncStatus = function(status) {
          console.log("SyncStatus =", status);
          switch (status) {
              case SyncStatus.UP_TO_DATE:
                  console.log("SyncStatus.UP_TO_DATE");
                  resolve();
                  break;

              case SyncStatus.UPDATE_INSTALLED:
                  console.log("SyncStatus.UPDATE_INSTALLED");
                  codePush.restartApplication();
                  break;

              case SyncStatus.ERROR:
                  // might be offline? codepush server down?
                  console.log("SyncStatus.ERROR");
                  // but we wanna start the app anyways
                  // as it can open in offline mode
                  resolve();
                  break;
          }
      };

      if(installNow) {
        codePush.sync(syncStatus, {
          installMode: InstallMode.IMMEDIATE,
          mandatoryInstallMode: InstallMode.IMMEDIATE
        });
      }
      else {
        // install later, on app restart or when app comes from background
        codePush.sync(syncStatus, {
          installMode: InstallMode.ON_NEXT_RESUME,
          mandatoryInstallMode: InstallMode.ON_NEXT_RESUME
        });
      }
    }
    else {
      resolve();
    }   
  });}

// on boot in application class instance as pseudo code
this.updateApp(true).then(hideSplash).then(runFurtherApplogic);
ruslan-bikkinin commented 6 years ago

Hi @christophmegusta and thanks for filling the issue. Only one sync method could be active. As alternative you can use method codePush.restartApplication in case if you need to apply installed update immediately. Please let us know if you have any questions or any updates.

christophmegusta commented 6 years ago

Only one sync method could be active why? What happens when i call it multiple times? Is 2nd call just ignored? Does it mess with the previous one?

Restarting the application is no problem. Its just that i cannot figure out when the update is installed. In my code example from my original post i did create a function which should block until update is installed. I could just create my own implementation of sync() then of course its doable, but i hoped for not having the low level implementation to do.

ruslan-bikkinin commented 6 years ago

What happens when i call it multiple times? Is 2nd call just ignored? Does it mess with the previous one?

The reason why parallel syncs are not supported is to prevent racing between them and setting application into incorrect state.

Restarting the application is no problem. Its just that i cannot figure out when the update is installed. In my code example from my original post i did create a function which should block until update is installed. I could just create my own implementation of sync() then of course its doable, but i hoped for not having the low level implementation to do.

You can pass syncCallback to sync command to track syncStatus of current operation. Please refer to documentation section for the additional information.

christophmegusta commented 6 years ago

you mean like this?

const syncStatus = function(status) {
          console.log("SyncStatus =", status);
          switch (status) {
              case SyncStatus.UP_TO_DATE:
                  console.log("SyncStatus.UP_TO_DATE");
                  resolve();
                  break;

              case SyncStatus.UPDATE_INSTALLED:
                  console.log("SyncStatus.UPDATE_INSTALLED");
                  codePush.restartApplication();
                  break;

              case SyncStatus.ERROR:
                  // might be offline? codepush server down?
                  console.log("SyncStatus.ERROR");
                  // but we wanna start the app anyways
                  // as it can open in offline mode
                  resolve();
                  break;
          }
      };

        codePush.sync(syncStatus, .....);

?

sergey-akhalkov commented 6 years ago

Hi @christophmegusta, yes, SyncStatus.UPDATE_INSTALLED state is what you are looking for, but before calling codePush.restartApplication() method you should also verify if sync has been called with the IMMEDIATE installation mode - just put the last installation mode value somewhere in the memory or jsonstorage and use it further.

christophmegusta commented 6 years ago

alright then i hack it myself.

though it would be nice if in the future the sync() comes with some extra parameter where restart / installation behaviour could be changed on the fly. If its possible to do it manually i see no reason why sync() couldnt do it.

sergey-akhalkov commented 6 years ago

@christophmegusta, sounds good, but unfortunately we are very "backlogged" now and have no ability to implement this since your use case looks very specific, but we'll look for the similar feature requests in the future and in case there will be a lot of them we'll add this enhancement to our backlog and implement it. Please also feel free to contribute and publish a PR if you have a chance - we'll be very thankful!

christophmegusta commented 6 years ago

thanks a lot. If we can implement it generic enough with less time we sure will contribute.