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

Update README(added an example) #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nirajniroula
Copy link

Added a basic working code for both types of update implementation.

Added a basic working code for both types of update implementation.
@SudoPlz
Copy link
Owner

SudoPlz commented Oct 26, 2021

Thanks for the PR, but I believe this needs to be addressed properly by fixing the actual example files in the project, instead of adding more instructions on the readme.

If you want to you can add that as a second example, and point to it from the readme.

Can you also please remove the react-native-snackbar references and code?

I'd be grateful!

Thanks

@nirajniroula
Copy link
Author

Sure, I can add a second example in a separate file, like the current one, and point to it from the README.

But regarding react-native-snackbar usages, I just wanted to implement the flexible updates as illustrated in the android docs believing it to be one of the right approaches. If you want me to remove that, I can do that too but I think that should not be an issue, at least not for an example.

Thank you for the quick response.

@nirajniroula
Copy link
Author

With snack bar, the update flow somewhat becomes like this:

  1. a dialog appears saying "Xyz recommends that you update to the latest version. You can keep using this app while downloading the update."

  2. user presses UPDATE button, download starts in the background, the user continues using the app

  3. snack bar appears after the update is downloaded with an INSTALL button (user can complete what he is about to do in the app, and install the new update afterwards i.e. the user won't be interrupted by the sudden installation process )

But again with this approach, there is one more thing that should be taken care of.
What if a user downloads a new update but never installs it!
There should be a way to get the preserved update state(DOWNLOADED in this case) from the native side(methods) so that on every launch, the user gets reminded about the update that was downloaded earlier and if he/she wants to installs it, via snack bar. I remember handling this with a listener while integrating it in a native android app. I shall look into it, if I get some free time.
But for now, in this example, I have used async-storage to store the update state so that if a user has already downloaded but not installed the new update, he/she gets notified about the downloaded update on every app launch.

Just trying to clarify the example. Hope I made it a little clearer.

@SudoPlz
Copy link
Owner

SudoPlz commented Oct 27, 2021

Cool, let's keep the snackbar, I think it's a-ok.
About FLEXIBLE builds I was under the impression that's handled automatically when startUpdate... fires up again for the second time (after a binary has already been downloaded) but feel free to double check since, I admit we're not using that and I haven't tested that scenario thoroughly.

@nirajniroula
Copy link
Author

nirajniroula commented Oct 27, 2021

It won't reach up to startUpdate for the second time.

 useEffect(() => {
      inAppUpdates.addStatusUpdateListener(status => onStatusUpdate(status));
  }, []);

...
...

   inAppUpdates
      .checkNeedsUpdate()
      .then(result => {
        if (result.shouldUpdate) {
          let updateOptions = {...};

          inAppUpdates.startUpdate(updateOptions);
        }
      })
      .catch(err => console.log("IAU Error: ", err));

Looks like result.shouldUpdate is false for the second time. I'm not sure about it but I can confirm that, in the code block above, the onStatusUpdate(status) method didn't execute the second time.

@jbaudanza
Copy link

I think right way to handle previously downloaded builds it to check for the UpdateAvailability.DEVELOPER_TRIGGERED_UPDATE_IN_PROGRESS state in updateAvailability. This should be more reliable than using the AsyncStorage method.

I implemented something like this in a fork:
Hilokal@5dc7471

And then in the client I do something like this:

const result = await inAppUpdates.checkNeedsUpdate();

if (result.other.installStatus === IAUInstallStatus.DOWNLOADED) {
  inAppUpdates.installUpdate();
} else {
  inAppUpdates.startUpdate({ updateType: IAUUpdateKind.FLEXIBLE });
}

@SudoPlz
Copy link
Owner

SudoPlz commented Mar 11, 2022

@jbaudanza can you open a PR with your addition? Or @nirajniroula do you think you can add it to this PR?

@nirajniroula
Copy link
Author

Since this PR was only about adding an example (to achieve a better UX) in README, I think it would be better if @jbaudanza, you could open a separate PR from Hilokal@5dc7471.
And maybe for this PR, as suggested by @SudoPlz earlier, I can add the second example in a separate file, like the current one, and point to it from the README.
I'm not being able to get back to it due to my occupation mainly in web development lately. I will do it whenever I get a chance to test the example thoroughly once again and maybe I shall also remove the AsyncStorage part from the example in the next push to this PR if the above fork gets merged to the master by then.

@jbaudanza
Copy link

I actually went in a different direction in my branch. I removed all the dependencies and everything that wasn't a direct binding to the InAppUpdates API. This made it much easier for me to integrate into my app.

I think the Apple App store API is different enough that trying to make a common API for both ends up being awkward.

Also siren brings in a lot of dependencies (apisauce, axios) for what is just a simple JSON fetch. I removed it and replaced with with a simple fetch() call and Alert directly in my app.

This is everything I removed: Hilokal/sp-react-native-in-app-updates@master...hilokal

If you're interested in going in this direction, I can prepare a PR.

Otherwise, if you only want the DEVELOPER_TRIGGERED_UPDATE_IN_PROGRESS, we should discuss what the API would look like. Maybe adding a shouldInstall boolean in addition to the shouldUpdate boolean would work. But, personally, I'd be in favor of just trimming down everything and letting the consumer of this module handle all that logic directly.

@GoldenSoju
Copy link

@SudoPlz
Would there still be interest into implementing @jbaudanza 's solution?

@SudoPlz
Copy link
Owner

SudoPlz commented May 9, 2024

@GoldenSoju I would love to find out what the community thinks about that.

Both make sense to me, I agree that siren does add some unnecessary dependencies but on the other hand it's a set and forget thing where you install once, and it should work on both platforms.

I think in an ideal world we would strip down this library so that

  • it doesn't include any other dependencies like siren, but then
  • add a module on top of it so that it works for iOS too (and then it would include siren too) but I'm
  • not sure how to structure that and make it easy enough for users while at the same time
  • make it obvious that this will be a breaking change if they choose to migrate to that new version which separates concerns

@jbaudanza
Copy link

@GoldenSoju @SudoPlz In my opinion, the user flows between Android In-App-Updates and iOS app store checks are so different, that it's not worth trying to unify into a single API. The react-native module should provide a clean Javascript interface to the Android In-App-Updates API, and it should be the role of the app developer to navigate the differences in the user flows in a way that makes sense to their app.

It's also been 2 years since I looked at this, so it's possible my memory is faded. But, I remember trying to make things work in siren to be more difficult than it was worth. In particular, siren has hardcoded the strategy for comparing version numbers:
https://github.com/GantMan/react-native-siren/blob/1bb4945d102a2b577dfee51742dbe8d0e358c63d/index.js#L83

In our app, we ended up replacing siren with a simple fetch() call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants