This repository has been archived by the owner on Jan 17, 2023. It is now read-only.
Attempt to make frontend-to-addon comms more robust #1102
Merged
meandavejustice
merged 1 commit into
mozilla:master
from
lmorchard:1101-more-robust-addon-messaging
Jul 21, 2016
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,39 +42,62 @@ app.extend({ | |
app.me = new Me(); | ||
app.experiments = new ExperimentsCollection(); | ||
|
||
Promise.all([ | ||
app.me.fetch(), | ||
app.experiments.fetch() | ||
]).then(() => { | ||
app.me.updateEnabledExperiments(app.experiments); | ||
app.pageManager = new PageManager({ | ||
pageContainer: document.querySelector('[data-hook=page-container]') | ||
}); | ||
|
||
app.pageManager = new PageManager({ | ||
pageContainer: document.querySelector('[data-hook=page-container]') | ||
}); | ||
if (!app.router.history.started()) { | ||
app.router.history.start(); | ||
// HACK for Issue #124 - sometimes popstate doesn't fire on navigation, | ||
// but pageshow does. But, we just want to know if the URL changed. | ||
addEventListener('pageshow', app.router.history.checkUrl, false); | ||
} | ||
|
||
if (!app.router.history.started()) { | ||
app.router.history.start(); | ||
// HACK for Issue #124 - sometimes popstate doesn't fire on navigation, | ||
// but pageshow does. But, we just want to know if the URL changed. | ||
addEventListener('pageshow', app.router.history.checkUrl, false); | ||
} | ||
app.experiments.fetch().then(() => app.me.fetch()).then(() => { | ||
app.me.updateEnabledExperiments(app.experiments); | ||
}).catch((err) => { | ||
// for now, log the error in the console & do nothing in the UI | ||
console && console.error(err); // eslint-disable-line no-console | ||
app.router.redirectTo('error'); | ||
}); | ||
}, | ||
|
||
// Send webChannel message to addon, use a Promise to wait for the answer. | ||
waitForMessage(type, data, timeout = 10000) { | ||
waitForMessage(type, data, timeout = 5000, retries = 3) { | ||
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. FWIW, reduced timeout to 5 seconds from 10 - because the retries should actually lead to a total timeout of 15 seconds before rejection. |
||
return new Promise((resolve, reject) => { | ||
const rejectTimer = setTimeout(() => { | ||
reject('waitForMessage timeout: ' + type); | ||
}, timeout); | ||
this.once('webChannel:' + type + '-result', (result) => { | ||
clearTimeout(rejectTimer); | ||
let attempts = 0; | ||
let retryTimer; | ||
|
||
// On timeout, try again or reject with an error if out of retries. | ||
const timeoutHandler = () => { | ||
if (attempts++ < retries) { | ||
attemptFn(); // eslint-disable-line no-use-before-define | ||
} else { | ||
reject('waitForMessage timeout: ' + type); | ||
} | ||
}; | ||
|
||
// On message response, resolve the promise with the result. | ||
const resultHandler = (result) => { | ||
clearTimeout(retryTimer); | ||
resolve(result); | ||
}); | ||
this.webChannel.sendMessage(type, data); | ||
}; | ||
|
||
const attemptFn = () => { | ||
// Un-register the event handler before registering it, in case this is | ||
// a retry. We don't want to accidentally handle it multiple times. | ||
const resultMessageName = 'webChannel:' + type + '-result'; | ||
this.off(resultMessageName, resultHandler); | ||
this.once(resultMessageName, resultHandler); | ||
|
||
// Try sending the message. | ||
this.webChannel.sendMessage(type, data); | ||
|
||
// Set up a timer to trigger retry / reject | ||
retryTimer = setTimeout(timeoutHandler, timeout); | ||
}; | ||
|
||
// Finally, kick off our first attempt. | ||
attemptFn(); | ||
}); | ||
}, | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Also, moving page manager & router init outside of the experiments fetch means we can show the error page.
Side effect will be that the page will start to render before the API actually returns results, so typically a sub-second lag until filling in with experiments. Previously, the whole page would just stay white until the experiments came in.