phonegap / phonegap-plugin-contentsync

Download and cache remotely hosted content
Apache License 2.0
206 stars 98 forks source link

CopyRootApp issue with app store updates #157

Closed DhiaNHM closed 7 years ago

DhiaNHM commented 7 years ago

We've been having the same problems as in issues #73 and #88, where the copyRootApp functionality in the plugin only works on a fresh installation. So we're unable to update via app-store download (which is sometimes preferred for bigger releases etc.) and have to use the contentsync for our AEM apps.

Additionally things like plugin updates or additions or initialization script changes (code which runs before the contentsync sync process) can't be updated at all, which really limits how we can improve our apps over time. I've created a fork to better control the copyRootApp function to try to get round this, the main idea is that the 'if cache folder exists' test on the copyRootApp functionality is removed and replaced by a new argument (copyRootApp) which is set by our apps' timestamp comparisons between running APK/IPA and cache folder.

https://github.com/phonegap/phonegap-plugin-contentsync/compare/master...NaturalHistoryMuseum:master

This needs to be tested for our purposes but I'm posting this here to get some comments on this modification, would be great to know if we're missing something and there is a better option to handle app store updates alongside contentsync.

imhotep commented 7 years ago

@DhiaNHM how would you update plugins' native code without submitting to the app store? You should be able to update your updates/additions/initialization scripts or any other resources via contentsync.

DhiaNHM commented 7 years ago

Hi, With the current plugin code it seems impossible to use the app store to update an app (since the copyRootApp functionality only runs once on a fresh install).

i see the root of the problem being this logic (android sync.java v:1.3.0:

            if (type.equals(TYPE_LOCAL) && !dir.exists()) {
                if ("null".equals(src) && (copyRootApp || copyCordovaAssets)) {
                    if (copyRootApp) {
                        copyRootApp(outputDirectory, manifestFile);
                    }
                    if (copyCordovaAssets) {
                        copyCordovaAssets(outputDirectory);
                    }

Since we're always using type 'local' in the initialisiation, copyRootApp and copyCordovaAssets are never called again. That means a new store update cant be used to update the cordova plugins folder for example (since the plugins aren't part of the contentsync folder).

imhotep commented 7 years ago

@DhiaNHM Oh I think I see what you mean. Does this problem occur on iOS as well?

macdonst commented 7 years ago

@DhiaNHM @imhotep my alternative thought is for the content sync plugin to detect a change in the app version and then have copyRootApp and copyCordovaAssets to function if set?

imhotep commented 7 years ago

@macdonst I like that. We'd have to keep track of versions but I am fine with that.

DhiaNHM commented 7 years ago

Thanks both, my fork leaves the app version comparison to be handled in the main app's code not the plugin, will you be modifying the plugin to use an additional local storage variable to keep track of app versions?

macdonst commented 7 years ago

@DhiaNHM my thought would be for the plugin to store the app version number in whatever platforms permanent storage. I wouldn't use localStorage as it could be cleared. Probably won't get a chance to fix this until early next week.

DhiaNHM commented 7 years ago

@macdonst - thanks, that makes sense to me, it's probably as you'd expect it to behave in most situations.

For my fork i coded it for our specific environment where we have the possibility that the 'shell' contentsync package might actually be more recent than the last store version (i.e. we did some minor css/JS changes that didn't warrant an app store release and were instead released via contentsync only), In this situation my fork compares the timestamps for all 3 possible 'shell' versions (initial install, current app on device and latest shell contentsync) and then decides whether to do copyRootApp (everything), copyCordovaAssets (just the plugins) or nothing.

However both approaches have the ongoing development problem where the app content delivered through contentsync might be out of sync with the installed app version (i.e. there's a plugin update required for the latest content and the user chooses not to update their app via the store), but that's obviously an app specific issue.

macdonst commented 7 years ago

@DhiaNHM I've created a branch issue157 to fix the bug. I'm going with the approach of checking to see if the app version has been updated. That way local will do the copyRootApp/copyCordovaAssets steps again.

For your issue with the three possible shell versions we'll have to work with the AEM team on that one to make it easier.

DhiaNHM commented 7 years ago

@macdonst That's great, thanks for looking at it so quickly. Will this fix work for IOS as well?, noticed the IOS code hasnt been modified in the 157 branch. Also how will it behave on a current build where the app version hasnt been stored by the current version of the plugin?

macdonst commented 7 years ago

@DhiaNHM I've pushed up the changes for Android and iOS to the master branch. Can you give it a try on your end?

If they app version hasn't been stored then the test for hasAppBeenUpdated should return true and the local sync with copyRootApp will happen if set to true.

Let us know here how things go and if everything looks good to you we'll release a new version with the bug fix to NPM.

DhiaNHM commented 7 years ago

@macdonst - thanks again for this, We've tested it on our 2 apps and it's working great, so we're hoping to include it in our next version.

macdonst commented 7 years ago

@DhiaNHM okay great, I'm about to get on a plane but I will release a new version to NPM today and tell the AEM team to update their dependences.

macdonst commented 7 years ago

@DhiaNHM I pushed version 1.3.2 to NPM and I'm closing this issue.