Skip to content
This repository was archived by the owner on May 20, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CodePush.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ failCallback:(void (^)(NSError *err))failCallback;
@interface CodePushPackage : NSObject

+ (void)installPackage:(NSDictionary *)updatePackage
removePendingUpdate:(BOOL)removePendingUpdate
error:(NSError **)error;

+ (NSDictionary *)getCurrentPackage:(NSError **)error;
Expand Down
49 changes: 44 additions & 5 deletions CodePush.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,20 @@ function log(message) {
console.log(`[CodePush] ${message}`)
}

async function notifyApplicationReady() {
// This ensures that notifyApplicationReadyInternal is only called once
// in the lifetime of this module instance.
const notifyApplicationReady = (() => {
let notifyApplicationReadyPromise;
return () => {
if (!notifyApplicationReadyPromise) {
notifyApplicationReadyPromise = notifyApplicationReadyInternal();
}

return notifyApplicationReadyPromise;
};
})();

async function notifyApplicationReadyInternal() {
await NativeCodePush.notifyApplicationReady();
const statusReport = await NativeCodePush.getNewStatusReport();
if (statusReport) {
Expand Down Expand Up @@ -170,16 +183,41 @@ function setUpTestDependencies(testSdk, providedTestConfig, testNativeBridge) {
if (testNativeBridge) NativeCodePush = testNativeBridge;
}

// This function allows only one syncInternal operation to proceed at any given time.
// Parallel calls to sync() while one is ongoing yields CodePush.SyncStatus.SYNC_IN_PROGRESS.
const sync = (() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure how I feel about this. I think the fix you made to notifyApplicationReady and the change you made to remove a pending update when installing a new update addressed two core issues, but I'm still not convinced that we should allow queuing concurrent calls to sync as opposed to rejecting the second with a new status (e.g. SYNC_IN_PROGRESS). If a sync call was setup to display a UI when it was checking or finished checking for the update, it wouldn't add any value to run both, since that would be a poor user experience. Since we don't know when sync will be called and how it will be presented, I think it's a much safer and simpler solution to just not allow sync to be called while another call is in progress.

Can you think of a reason why calling it twice in parallel would actually be valuable? Calling sync twice within a single user session makes total sense, which is why we need the logic to clear a pending update when downloading, but I don't see the value in allowing calling sync concurrently, especially since the side effects would be unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely don't see any point in allowing parallel requests. And I also think rejecting the sync is better. The metaphor I'm thinking of is when an email client is syncing with the email server, when you do a pull-to-refresh in the client app for example, it would block you from doing the pull-to-refresh again until the existing one is completed. Since we are using the "sync" metaphor, by definition, it doesn't make sense to "sync in parallel", you should be either syncing, in sync, or out of sync.

Is SYNC_IN_PROGRESS the status we want to return? It sounds pretty okay to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that metaphor / rationale :) 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm cool with SYNC_IN_PROGRESS. I can't think of anything much better :)

let syncInProgress = false;
const setSyncCompleted = () => { syncInProgress = false; };

return (options = {}, syncStatusChangeCallback, downloadProgressCallback) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do "return (...args) {" and then do "syncInternal(...args)" below? That way we don't have to define the signature/default values twice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, never mind, you need access to the sync callback, so what you have is probably easier.

if (syncInProgress) {
typeof syncStatusChangeCallback === "function"
? syncStatusChangeCallback(CodePush.SyncStatus.SYNC_IN_PROGRESS)
: log("Sync already in progress.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer always logging this, instead of only when there isn't a sync callback. That way, basic diagnostics could still be done via the console as opposed to having to set break points or expect the dev to add their own logging.

return Promise.resolve(CodePush.SyncStatus.SYNC_IN_PROGRESS);
}

syncInProgress = true;
const syncPromise = syncInternal(options, syncStatusChangeCallback, downloadProgressCallback);
syncPromise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to assign the result of this expression back to "syncPromise" before returning it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I see what you're doing now :)

.then(setSyncCompleted)
.catch(setSyncCompleted)
.done();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't calling done here end the Promise chain so that devs couldn't call .then on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thats why i didn't do the assignment as per your previous comment. The one im returning is the un-doned promise

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, got it. Thanks for clarifying that!


return syncPromise;
};
})();

/*
* The sync method provides a simple, one-line experience for
* The syncInternal method provides a simple, one-line experience for
* incorporating the check, download and application of an update.
*
* It simply composes the existing API methods together and adds additional
* support for respecting mandatory updates, ignoring previously failed
* releases, and displaying a standard confirmation UI to the end-user
* when an update is available.
*/
async function sync(options = {}, syncStatusChangeCallback, downloadProgressCallback) {
async function syncInternal(options = {}, syncStatusChangeCallback, downloadProgressCallback) {
const syncOptions = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this "syncInternal"?


deploymentKey: null,
Expand All @@ -190,7 +228,7 @@ async function sync(options = {}, syncStatusChangeCallback, downloadProgressCall
...options
};

syncStatusChangeCallback = typeof syncStatusChangeCallback == "function"
syncStatusChangeCallback = typeof syncStatusChangeCallback === "function"
? syncStatusChangeCallback
: (syncStatus) => {
switch(syncStatus) {
Expand Down Expand Up @@ -229,7 +267,7 @@ async function sync(options = {}, syncStatusChangeCallback, downloadProgressCall
}
};

downloadProgressCallback = typeof downloadProgressCallback == "function"
downloadProgressCallback = typeof downloadProgressCallback === "function"
? downloadProgressCallback
: (downloadProgress) => {
log(`Expecting ${downloadProgress.totalBytes} bytes, received ${downloadProgress.receivedBytes} bytes.`);
Expand Down Expand Up @@ -338,6 +376,7 @@ const CodePush = {
UP_TO_DATE: 4, // The running app is up-to-date
UPDATE_IGNORED: 5, // The app had an optional update and the end-user chose to ignore it
UPDATE_INSTALLED: 6, // The app had an optional/mandatory update that was successfully downloaded and is about to be installed.
SYNC_IN_PROGRESS: 7, // There is an ongoing "sync" operation in progress.
UNKNOWN_ERROR: -1
},
DEFAULT_UPDATE_DIALOG: {
Expand Down
1 change: 1 addition & 0 deletions CodePush.m
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ - (void)savePendingUpdate:(NSString *)packageHash
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
NSError *error;
[CodePushPackage installPackage:updatePackage
removePendingUpdate:[self isPendingUpdate:nil]
error:&error];

if (error) {
Expand Down
36 changes: 25 additions & 11 deletions CodePushPackage.m
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,8 @@ + (void)copyEntriesInFolder:(NSString *)sourceFolder
}

+ (void)installPackage:(NSDictionary *)updatePackage
error:(NSError **)error
removePendingUpdate:(BOOL)removePendingUpdate
error:(NSError **)error
{
NSString *packageHash = updatePackage[@"packageHash"];
NSMutableDictionary *info = [self getCurrentPackageInfo:error];
Expand All @@ -458,19 +459,32 @@ + (void)installPackage:(NSDictionary *)updatePackage
return;
}

NSString *previousPackageHash = [self getPreviousPackageHash:error];
if (!*error && previousPackageHash && ![previousPackageHash isEqualToString:packageHash]) {
NSString *previousPackageFolderPath = [self getPackageFolderPath:previousPackageHash];
// Error in deleting old package will not cause the entire operation to fail.
NSError *deleteError;
[[NSFileManager defaultManager] removeItemAtPath:previousPackageFolderPath
error:&deleteError];
if (deleteError) {
NSLog(@"Error deleting old package: %@", deleteError);
if (removePendingUpdate) {
NSString *currentPackageFolderPath = [self getCurrentPackageFolderPath:error];
if (!*error && currentPackageFolderPath) {
// Error in deleting pending package will not cause the entire operation to fail.
NSError *deleteError;
[[NSFileManager defaultManager] removeItemAtPath:currentPackageFolderPath
error:&deleteError];
if (deleteError) {
NSLog(@"Error deleting pending package: %@", deleteError);
}
}
} else {
NSString *previousPackageHash = [self getPreviousPackageHash:error];
if (!*error && previousPackageHash && ![previousPackageHash isEqualToString:packageHash]) {
NSString *previousPackageFolderPath = [self getPackageFolderPath:previousPackageHash];
// Error in deleting old package will not cause the entire operation to fail.
NSError *deleteError;
[[NSFileManager defaultManager] removeItemAtPath:previousPackageFolderPath
error:&deleteError];
if (deleteError) {
NSLog(@"Error deleting old package: %@", deleteError);
}
}
[info setValue:info[@"currentPackage"] forKey:@"previousPackage"];
}

[info setValue:info[@"currentPackage"] forKey:@"previousPackage"];
[info setValue:packageHash forKey:@"currentPackage"];

[self updateCurrentPackageInfo:info
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ This method returns a `Promise` which is resolved to a `SyncStatus` code that in

* __codePush.SyncStatus.UPDATE_INSTALLED__ *(6)* - The update has been installed and will be run either immediately after the `syncStatusChangedCallback` function returns or the next time the app resumes/restarts, depending on the `InstallMode` specified in `SyncOptions`.

If the update check and/or the subsequent download fails for any reason, the `Promise` object returned by `sync` will be rejected with the reason.
* __codePush.SyncStatus.SYNC_IN_PROGRESS__ *(7)* - There is an ongoing `sync` operation running which prevents the current call from being executed.

The `sync` method can be called anywhere you'd like to check for an update. That could be in the `componentWillMount` lifecycle event of your root component, the onPress handler of a `<TouchableHighlight>` component, in the callback of a periodic timer, or whatever else makes sense for your needs. Just like the `checkForUpdate` method, it will perform the network request to check for an update in the background, so it won't impact your UI thread and/or JavaScript thread's responsiveness.

Expand Down Expand Up @@ -622,6 +622,7 @@ This enum is provided to the `syncStatusChangedCallback` function that can be pa
* __codePush.SyncStatus.UP_TO_DATE__ *(4)* - The app is fully up-to-date with the configured deployment.
* __codePush.SyncStatus.UPDATE_IGNORED__ *(5)* - The app has an optional update, which the end user chose to ignore. (This is only applicable when the `updateDialog` is used)
* __codePush.SyncStatus.UPDATE_INSTALLED__ *(6)* - An available update has been installed and will be run either immediately after the `syncStatusChangedCallback` function returns or the next time the app resumes/restarts, depending on the `InstallMode` specified in `SyncOptions`.
* __codePush.SyncStatus.SYNC_IN_PROGRESS__ *(7)* - There is an ongoing `sync` operation running which prevents the current call from being executed.
* __codePush.SyncStatus.UNKNOWN_ERROR__ *(-1)* - The sync operation encountered an unknown error.

### Objective-C API Reference (iOS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ public void installUpdate(final ReadableMap updatePackage, final int installMode
@Override
protected Void doInBackground(Object... params) {
try {
codePushPackage.installPackage(updatePackage);
codePushPackage.installPackage(updatePackage, isPendingUpdate(null));

String pendingHash = CodePushUtils.tryGetString(updatePackage, codePushPackage.PACKAGE_HASH_KEY);
if (pendingHash == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,15 +259,23 @@ public void downloadPackage(Context applicationContext, ReadableMap updatePackag
CodePushUtils.writeReadableMapToFile(updatePackage, bundlePath);
}

public void installPackage(ReadableMap updatePackage) throws IOException {
public void installPackage(ReadableMap updatePackage, boolean removePendingUpdate) throws IOException {
String packageHash = CodePushUtils.tryGetString(updatePackage, PACKAGE_HASH_KEY);
WritableMap info = getCurrentPackageInfo();
String previousPackageHash = getPreviousPackageHash();
if (previousPackageHash != null && !previousPackageHash.equals(packageHash)) {
FileUtils.deleteDirectoryAtPath(getPackageFolderPath(previousPackageHash));
if (removePendingUpdate) {
String currentPackageFolderPath = getCurrentPackageFolderPath();
if (currentPackageFolderPath != null) {
FileUtils.deleteDirectoryAtPath(currentPackageFolderPath);
}
} else {
String previousPackageHash = getPreviousPackageHash();
if (previousPackageHash != null && !previousPackageHash.equals(packageHash)) {
FileUtils.deleteDirectoryAtPath(getPackageFolderPath(previousPackageHash));
}

info.putString(PREVIOUS_PACKAGE_KEY, CodePushUtils.tryGetString(info, CURRENT_PACKAGE_KEY));
}

info.putString(PREVIOUS_PACKAGE_KEY, CodePushUtils.tryGetString(info, CURRENT_PACKAGE_KEY));
info.putString(CURRENT_PACKAGE_KEY, packageHash);
updateCurrentPackageInfo(info);
}
Expand Down