-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Sync Twice issue #187
Sync Twice issue #187
Changes from all commits
35201d1
e34f514
2733af7
8cc305d
9b00cc9
02e556d
340bb7e
f42bd71
360fb33
2177a11
4ff77d8
f500e51
0b97f4b
b342b81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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 = (() => { | ||
let syncInProgress = false; | ||
const setSyncCompleted = () => { syncInProgress = false; }; | ||
|
||
return (options = {}, syncStatusChangeCallback, downloadProgressCallback) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe call this "syncInternal"? |
||
|
||
deploymentKey: null, | ||
|
@@ -190,7 +228,7 @@ async function sync(options = {}, syncStatusChangeCallback, downloadProgressCall | |
...options | ||
}; | ||
|
||
syncStatusChangeCallback = typeof syncStatusChangeCallback == "function" | ||
syncStatusChangeCallback = typeof syncStatusChangeCallback === "function" | ||
? syncStatusChangeCallback | ||
: (syncStatus) => { | ||
switch(syncStatus) { | ||
|
@@ -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.`); | ||
|
@@ -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: { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :) 👍
There was a problem hiding this comment.
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 :)