feat: Shorebird code push could have a more friendly API

erickzanardo commented 3 months ago

Right now, shorebird_code_push plugin is very complete, having methods to check the state of a patch, the current install, etc.

This is great and all, but we don't have any "helper method" for common flows that users might want to implement.

For example, if I want to implement a routine that will check if there is a patch available to be downloaded and installed and prompt the user for the installation, I will need to use:

isNewPatchAvailableForDownload
isNewPatchReadyToInstall
downloadUpdateIfAvailable

To be sure that a patch was downloaded and will be applied on the next boot, the user would need to implement something like this:

  Future<void> _installUpdate() async {
    await shorebirdCodePush.downloadUpdateIfAvailable();

    var retries = 3;
    bool isNewPatchAvailableForInstall = false;

    while (!isNewPatchAvailableForInstall) {
      isNewPatchAvailableForInstall =
          await shorebirdCodePush.isNewPatchReadyToInstall();

      retries--;
      await Future.delayed(const Duration(milliseconds: 200));

      if (retries == 0) {
        throw Exception('Failed to install update!');
      }
    }
  }

Ideally we could abstract some of the complexities for the user in order to make their life easier when managing themselves how patches are downloaded/installed.

My suggestion would be to create two new methods:

isRunningLastPatch: Would return true if the current running app is running the latest code, it doesn't matter if it is running the release and there are no patches yet, or if the app is running from the latest patch. If the app is running from a patch that isn't the last, but the latest patch is already installed, this should return false, so this method would be an answer to the question: "Is the current running app, the last ever code that I sent for this release?"
ensureUpdate: Would download if needed, and ensure that the next patch is ready to be installed, pretty much something like my naive example above.

bryanoltman commented 3 months ago

Always supportive of improving the ergonomics of this package. Trying to take this point-by-point:

_installUpdate

I think are using some of the existing functions incorrectly (not your fault, the naming could be way better), so some questions/thoughts:

downloadUpdateIfAvailable completes once the patch has been downloaded and inflated. I don't think the retry logic you have is necessary – I think this does everything you want this _installUpdate function to do.
isNewPatchReadyToInstall returns true if the current patch number != the next patch number we'll boot from. This returns true if a new patch will be loaded when the app restarts.

It's likely that the solution for this problem is better documentation and function renaming. downloadUpdateIfAvailable could maybe be installNextPatchIfAvailable? I'm not sure I've got a better idea for isNewPatchReadyToInstall at the moment, but that could probably be improved too.

isRunningLastPatch

I like this idea – I imagine this would simply return false if isNewPatchAvailableForDownload() || isNewPatchReadyToInstall().

ensureUpdate

I'm a bit unclear on this. Maybe with the clarification above and some renaming, this wouldn't be needed? I think this is describing the current behavior of downloadUpdateIfAvailable (which, as named, isn't clear that it does everything necessary to install a patch).

Happy to chat more here or on VC for higher bandwidth.

erickzanardo commented 3 months ago

downloadUpdateIfAvailable completes once the patch has been downloaded and inflated. I don't think the retry logic you have is necessary

Interesting! While testing, I had the impression that downloadUpdateIfAvailable were async just for the fact that it is probably communicating with the updater through ffi? Because in my testing, it completed, I restarted the app, and the patch were not ready yet. Not sure if the download were not completed or not inflated.

I were able to only find a reliable routine once I combined the downloadUpdateIfAvailable and isNewPatchReadyToInstall. So maybe either I did something wrong or I might have stumbled in a bug?

ensureUpdate I'm a bit unclear on this. Maybe with the clarification above and some renaming, this wouldn't be needed? I think this is describing the current behavior of downloadUpdateIfAvailable (which, as named, isn't clear that it does everything necessary to install a patch).

The ensureUpdate suggestion were given because of the behaviour that I faced on my testing which I explained above.

bryanoltman commented 3 months ago

downloadUpdateIfAvailable ends up executing this function https://github.com/shorebirdtech/updater/blob/main/library/src/updater.rs#L214-L306 in the Updater. If downloadUpdateIfAvailable completed and the patch wasn't fully ready to install, that's a bug that we should look into.

erickzanardo commented 3 months ago

Got ya! Makes sense!

eseidel commented 3 months ago

I definitely think putting a nicer coat of paint on shorebird_code_push is in our future (possibly your future Erick). :) It was created just to expose our (raw) updater methods, but we should just rename/update and start from user journeys (as you're doing) and work backwards to APIs from those.

shorebirdtech / updater