ArtSabintsev / Siren

Notify users when a new version of your app is available and prompt them to upgrade.
http://sabintsev.com/
MIT License
4.24k stars 407 forks source link

Feature request: add hourly UpdatePromptFrequency option #257

Closed jyounus closed 5 years ago

jyounus commented 5 years ago

Hey there,

It would be great if there was an option to check once every x hours, instead of just immediately, daily or weekly.

Something like UpdatePromptFrequency.hourly(6) this would then check once every 6 hours.

What do you think?

ArtSabintsev commented 5 years ago

Hi,

Thanks for the suggestion. Is there any reason daily isn't a good enough measure of time? In what scenario would you want to use smaller units of time for version checks / alert presentations?

jyounus commented 5 years ago

I'm currently using Siren as a force update feature. I do this whenever a new major version is available. In this scenario, a major version update means there are some critical changes or breaking changes and need my users to be on the latest version asap.

Using the .immediately parameter is currently how I'm doing this. I make the check on app start as well as when the app comes back to the foreground.

Using a daily check just isn't frequent enough. If I release a new major version, it would be for an important reason. Having a more frequent check would be better for me. Using .immediately is a bit too excessive for my use case, as it currently makes an API call every time the user launches/comes back into the app.

This brings me to my second issue for my use case. I currently need to use the .immediately option when the user returns back to the app, so they can't continue using the app if there's supposed to be a force update.

Is there any way you could make it so that if it detects we're forcing a release, it doesn't need to make an API call every time? Instead it can check locally with the previously fetched data? This could be a separate method or an additional parameter (something like offlineOnly: Bool), I just want to be as efficient as possible while also making sure users can't use the app anymore if I need to force update them.

ArtSabintsev commented 5 years ago

Thanks for the detailed feedback.

The reason for choosing a day as a lowest common delimiter of time was to account for the issues discussed here (https://github.com/ArtSabintsev/Siren#words-of-caution). The only option I can think of to make use of right now, would be the following:

RulesManager(majorUpdateRules: .critical, showAlertAfterCurrentVersionHasBeenReleasedForDays: 0)

That is equivalent to setting:

UpdatePromptFrequency = 0
AlertType = immediately
showAlertAfterCurrentVersionHasBeenReleasedForDays = 0

However, it may cause an endless loop between your App and the App Store if the update has not propagated.

As for caching calls and referencing them, I may add that in as an option.

Let me mull this over a solution that will work for the majority of users without breaking the existing API too much.

ArtSabintsev commented 5 years ago

Also, you may not want to do two checks, especially one on foreground. As of Siren v4.0.0, I use the didBecomeActiveNotification to automatically initiate version checks. All of this is discussed here: https://github.com/ArtSabintsev/Siren#implementation-examples

jyounus commented 5 years ago

We're currently still on Swift 4.1, which is why we're not using Siren v4 yet. We should be updating our project to Swift 4.2 soon though, so I'll keep that in mind about didBecomeActiveNotification.

As for the issue above, we're happy to keep the showAlertAfterCurrentVersionHasBeenReleasedForDays option set to the default 24 hours. But after that, we'd like to be able to fine tune the time when it'll make another check. So we still want to wait the initial 24 hours, but then have the option to set it for example to 3 or 6 hours.

This of course assumes that the API call returns a timestamp when the new update was made available. Then work out timestamp + 24 hours and then use something like UpdatePromptFrequency.hourly(6) to figure out when to make another API call/show the dialog.

Adding that caching option would be great, thanks for that!

ArtSabintsev commented 5 years ago

Got it. The API does return a timestamp, which is what Siren uses to calculate showAlertAfterCurrentVersionHasBeenReleasedForDays.

That's a very very specific change that you want. It does make sense, but the bigger issue in my mind, is what the appropriate default setting should be and how to convey the ability to customize that value.

I'll think it all over, however, any change I do make, will be to the to the Swift 4.2 version of Siren.

jyounus commented 5 years ago

That sounds reasonable, thank you for that!

ArtSabintsev commented 5 years ago

Alright, I've been thinking about this all week.

First off, in Siren v4.0.0, the rules governing how Siren works have moved from:

Should an API request be performed?

to:

Should an alert be presented?

This means that an API request is always performed when Siren's code-path is entered, and once the response is validated to contain a new version, the complex rules governing alert presentation, including the showAlertAfterCurrentVersionHasBeenReleasedForDays value, are evaluated before presenting an alert. This is a fundamental difference in how the swift4.1 branch of Siren operates. So, in your scenario, after an update is live int he App Store for 24 hours, an alert will always be presented unless you have rules in place that suppress that alert.

With that in mind, I should inform you that I follow the 80% rule to software development, which is an obscure form of the Pareto Principle. What this means is that the mainline version of Siren (e.g., my master fork) contains solutions that apply to 80% of my intended consumers. My recommendation to people who want very custom tailored solutions is to fork Siren and build it themselves. While not ideal for those people, it has allowed me to maintain my OSS repos without obscure features that stretch my OSS repos beyond their intended goal.

Alright, so what does that mean for you and your case? It means that I will not be adding the caching change, since Siren's whole goal is to check the App Store for a new version every time the app is launched. Based on what it finds, and based on the presentation rules, and alert may be presented. Caching a response subverts Siren's core functionality by returning cached responses for a given period of time that may not be valid for the length of time for which the the response is cached. Second, I will not be adding the ability to perform an initial version checks after X hours vs the current X days. Why? Because trial and error over the last ~7 years has taught me that the iTunes Lookup API response updates ~6-24 hours faster than the binary is made available. I implemented the default 1 day wait period to protect users from getting into an infinite loop with the App Store when developers who consume Siren use the alertType = .force setting, since users who launch the app before the binary is made available cannot update their app, nor can they use the existing version of the app. This right here is why your proposed 6 hour solution is not something I want to implement.

I have mulled over this decision for 5 days and this is my final conclusion to your suggestions.

I am closing this issue, but not locking it, meaning you are welcome to follow up, and I will happily continue this conversation, but I doubt I will be swayed from my current stance.

Thank you and have a great weekend!

jyounus commented 5 years ago

Fair enough, I can understand that. I'll just either implement a custom solution or fork off your branch to customise the functionality and suit our project's needs.

Thank you anyways!

ArtSabintsev commented 5 years ago

My pleasure! Best of luck and thanks for understanding!

Sent with GitHawk