Sync Twice issue #187
Sync Twice issue #187
Conversation
| @@ -179,7 +191,7 @@ function setUpTestDependencies(testSdk, providedTestConfig, testNativeBridge) { | |||
| * releases, and displaying a standard confirmation UI to the end-user | |||
| * when an update is available. | |||
| */ | |||
| async function sync(options = {}, syncStatusChangeCallback, downloadProgressCallback) { | |||
| async function syncOperation(options = {}, syncStatusChangeCallback, downloadProgressCallback) { | |||
| const syncOptions = { | |||
There was a problem hiding this comment.
Maybe call this "syncInternal"?
| error:&deleteError]; | ||
| if (deleteError) { | ||
| NSLog(@"Error deleting old package: %@", deleteError); | ||
| if (!removePendingUpdate) { |
There was a problem hiding this comment.
I'm confused by this because it looks like the "removePendingUpdate" argument is set to true if there is a pending update, but you're only deleting the current update if there isn't a pending update. Does this need to be changed to "if (removePendingUpdate)" or am I reading it wrong?
Is the "!" operator a typo? The same logic in Java doesn't have it and seems correct as far as my understanding goes.
| @@ -170,8 +170,20 @@ function setUpTestDependencies(testSdk, providedTestConfig, testNativeBridge) { | |||
| if (testNativeBridge) NativeCodePush = testNativeBridge; | |||
| } | |||
|
|
|||
| // This function allows calls to syncInternal to be chained on to each other so that they do not | |||
| // interleave in the event that sync() is called multiple times. | |||
| const sync = (() => { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like that metaphor / rationale :) 👍
There was a problem hiding this comment.
I'm cool with SYNC_IN_PROGRESS. I can't think of anything much better :)
|
|
||
| syncInProgress = true; | ||
| const syncPromise = syncInternal(options, syncStatusChangeCallback, downloadProgressCallback); | ||
| syncPromise |
There was a problem hiding this comment.
Don't you need to assign the result of this expression back to "syncPromise" before returning it?
There was a problem hiding this comment.
Never mind, I see what you're doing now :)
| if (syncInProgress) { | ||
| typeof syncStatusChangeCallback == "function" | ||
| ? syncStatusChangeCallback(CodePush.SyncStatus.SYNC_IN_PROGRESS) | ||
| : log("Sync already in progress."); |
There was a problem hiding this comment.
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.
|
LGTM! |
|
Sorry! I accidentally fat fingered the close button on my phone :/ |
|
Can you also update the docs in two places:
|
|
Awesome! Let's merge this baby :) You can disregard my previous comment about the docs. What you have is totally fine. |
Fixes #110