Fixed issue for iOS when package folder was not cleared properly

[max-mironov]

This changes should fix #207. Tried to use more robust solution for determining correct build time for the app, Also add logic to also verify if the app version was changed before clearing deployments. Also moved repeated code for getting the app version to utility function.

[msftclas]

Hi @max-mironov, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Maxim Mironov (Akvelon)). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

[tatatananana]

Hi @max-mironov,

Please fix 3 calls from: [Utilities getAppVersion] to [Utilities getApplicationVersion]
Ive tested it and It solves issue #207

[max-mironov]

@tatatananana, thank you so much for pointing at this, changes are committed.

[richardhuaaa]

Hey @max-mironov, the fix for getting the application build time looks good, but are you sure we need to add the app version check here? I may have misunderstood completely, but I'm confused about two things:

1. Assuming that the native timestamps are now correct, the logic before this PR was made should be correct. We are checking the updates we have on disk, and comparing the binary timestamp of the client when the package was downloaded to the binary timestamp of the client now. In other words, we're checking to see if the binary has been overwritten since we installed this package. If it has been overwritten, then we don't want this package, because the javascript that got deployed with the binary is probably newer. Note that, if the binary has not been updated, there is no possible way for the app version to mismatch (the package would not be installed in the first place).
2. I don't think checking the app version is robust here. Firstly, the app version can be the same even if the binary has been updated (the dev is testing new builds being published to the simulator without updating the app version for now). Secondly, the app version can be different even if the binary has not been updated. This is because the version on the downloaded update can be a range, e.g. >= 1.0.0, whereas the version on the binary is always a fixed semver, e.g. 1.0.0.

Does this make sense, or have I misunderstood something? Also, are the native timestamps coming through correctly?

[richardhuaaa]

Is it possible that this change fixed the problem, without the need for the appVersion check in CodePush.m?

[max-mironov]

@Silhouettes I agree that it looks reasonable because in this way we will have some inconsistency in implementation this logic for iOS and Android (because we do not check for app version change on Android).

Check for appVersion is needed because the timestamp will not be updated unless we have some changes in native code, so if we bump app version in plist and rebuild than the timestamp won't be updated. I imagine this can be a problem because this way the app will not clear old deployments.

So you said that

Secondly, the app version can be different even if the binary has not been updated. This is because the version on the downloaded update can be a range, e.g. >= 1.0.0, whereas the version on the binary is always a fixed semver, e.g. 1.0.0.

I've double checked and it looks like app version is written to metadata here: https://github.com/Microsoft/cordova-plugin-code-push/blob/master/www/localPackage.ts#L177 (writeNewPackageMetadata function) and is acquired from native side by calling the same getAppVersion method so the app version couldn't be a range and check for appVersion will always make sense. Please correct me if I'm wrong.

[richardhuaaa]

Thanks for the explanation @max-mironov, what you said makes sense. I'm sorry to be so nitpicky, but it seems like this fix doesn't fully cover the problem - if the dev makes a script-only change and deploys it to the simulator or device without bumping their app version, then it won't apply to them. Hopefully my understanding is correct here? It seems a bit much to ask people to bump their app version every time they make a script change to their app just so that they're able to test it.

It seems to me that the real issue here is, as you said, the timestamp will not be updated unless we have some changes in native code. Just brainstorming here - does this mean the logic in the React Native (iOS) plugin is also broken?

https://github.com/Microsoft/react-native-code-push/blob/e42485243d9466fb6c00937bd9f67cee0d96de53/ios/CodePush/CodePush.m#L172
https://github.com/Microsoft/react-native-code-push/blob/bb04535fbef4cb580da4c29ae628452cc0774fc8/ios/CodePush/CodePushUpdateUtils.m#L229

Or is there something being done differently there? This is not just for the sake of robustly addressing this issue, but also because it would be good to try and converge the behavior of the Cordova and React Native plugins as we will be attempting to unify the native codebase in the future.

Alternatively, could we somehow copy the mechanism of the React Native Android plugin?

https://github.com/Microsoft/react-native-code-push/pull/683/files

Will that work? It may be that your current approach is the only way to address the issue, but I just want to make sure we exhaust other options first.

[richardhuaaa]

(Updated the above comment)

[max-mironov]

@Silhouettes thank you, I've got your point!
Digging deeper into this issue I've found out that we can use ios pre build script to touch file and this way timestamp will be updated on build every time despite we do not change any native code.
Shell script in PBXShellScriptBuildPhase section could be something like this:
touch -m ${SRCROOT}/${PROJECT_NAME}/plugins/cordova-plugin-code-push/Utilities.m

We can add Cordova "after_plugin_install" hook for ios in config.xml and it will execute script that will modify pbxproj file and insert BuildPhase run script section to it.

Please let me know what do you think about this solution?
If it works we can also apply the same solution for React Native iOS plugin.

[richardhuaaa]

@max-mironov That's a good idea! Will that cause a re-compilation of the binary, and will it be a significant time cost? Also, would it work the same way if we just touched the binary after it's already been built?

[richardhuaaa]

Okay, so I've been looking at this again, and I feel like there are two parts here that I didn't sort through clearly before:

1. App Version Check - My apologies, I do think it is good to include this now. This is partly for consistency with the RN plugin, which does the same check, and partly just because this is 'insurance' for apps in production, as it absolutely guarantees they can never suffer this problem even if the timestamps are wrong (because the app version is always bumped in production).
2. Timestamp - I think it's important that this timestamp changes for both native and script changes, so that devs don't get weird behavior as they deploy to their simulator or device, when they've only changed one or the other.

Also, if the timestamp is not updating correctly here, it is likely also not updating correctly in the React Native plugin. Probably the reason we haven't seen any issues on that is because it is mostly masked by the app version check (to repro this, they'd have to be developing against a real device on 10.2 or up). So, once there is a solution for (2), it might be good to copy and paste the same fix there.

[richardhuaaa]

Oh! I have another idea. I don't know whether or not it is better than your idea of touching native files, I'll leave it up to your judgement. What if we did all three of the following:

1. Check __DATE__ and __TIME__, which changes if native changes have been made. Can we double-check that this is correct on physical devices running >= iOS 10.2, which is where we've originally reproduced the failing timestamps?
2. Check the binaryHash (hash of the JS that shipped with the binary) of the current binary, vs the binaryHash of the binary when the update was installed. This will change if JS changes have been made.
3. Check the appVersion (this is not necessary, but we include it as insurance)

In other words, I think that the { __DATE__, __TIME__, binaryHash } tuple uniquely identifies a given binary. If the dev makes changes to either native or JS and then deploys their app, this tuple will have a different value. Do you think so?

---

The only case where this will fail: If a dev deploys binary A to the simulator, and then update B is synced to the app. Then, without making any changes, they deploy binary A to the simulator again. When they boot the app, it will still be running binary B. I can't think of a way around this, unless we can actually get the timestamp of the JS bundle :( But maybe this is acceptable.

[max-mironov]

@Silhouettes, ok finally tested the latest solution for getting correct build time by looking at NSModificationDate of plist file in main bundle. Verified it is working on iOS10.2.1 and iOS8.1 devices and is updated each time on native build and through cordova run ios deployment. Also tested this is working with TestFlight deployment. This should also cover the latest case you described above.

[richardhuaaa]

Perfect :) Merging this for you seeing as you'll be out for a few days.