-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle the browsers which don't support promises (eg. Edge) #43
Conversation
What's up with this pull request? |
This change is dedicated to handling browsers (Edge) that offer a |
content scripts (JavaScript included with your extension, that you will inject into web pages)
@@ -6,7 +6,16 @@ | |||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
"use strict"; | |||
|
|||
if (typeof browser === "undefined") { | |||
var _supportsPromises = false; |
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.
Use let
, not var
and see my next review.
} | ||
|
||
if (typeof browser === "undefined" || !_supportsPromises) { | ||
var _browser = window.browser || window.msBrowser || window.chrome || browser; |
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.
Use const
, not var
.
return grunt.file.read(filename) | ||
.replace(/\n$/, "") | ||
.replace(/^[^{]/gm, " $&"); | ||
return grunt.file.read(filename).replace(/\n$/, "").replace(/^[^{]/gm, " $&"); |
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.
Undo this.
@@ -4,3 +4,6 @@ dist/* | |||
## code coverage | |||
coverage/ | |||
.nyc_output/ | |||
/.project | |||
/dist/ | |||
/node_modules/ |
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.
This is unnecessary
console.log("promises not supported."); | ||
} | ||
|
||
if (typeof browser === "undefined" || !_supportsPromises) { |
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.
A better option would be to change this line to:
if (typeof browser === "undefined" || !(() => {
let supportsPromises = false;
try {
supportsPromises = browser.runtime.getPlatformInfo() instanceof Promise;
} catch (e) {
console.log("promises not supported.");
}
return supportsPromises;
})()) {
And remove the previous block.
Closing in favor of #114 (which is still exploring how we may allow the polyfill to work on Edge, and it is also taking advantage of the recently added integration tests to ensure that the additional changes needed are not going to break the polyfill behaviors expected on Chrome and Firefox). |
Compatibility with environments that have the browser namespace but do not support promises.
I was inspired by Snoak's answer in the following issue: #3