Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

kai918
Copy link

@kai918 kai918 commented Jul 13, 2017

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

@Croydon
Copy link

Croydon commented Sep 20, 2017

What's up with this pull request?

@mi-g
Copy link

mi-g commented Sep 30, 2017

This change is dedicated to handling browsers (Edge) that offer a browser global but do not provide a native promise return in WebExtensions methods.
The detection of this case is done by calling a method that requires a callback (necessary because no promise returns). If the method generates an exception, it means it does not support promise and the webextension-polyfill code must do its job.
Unfortunately in this PR, the method chosen to test the promise availability is browser.runtime.getPlatformInfo() which indeed generates an exception on Edge, but primarily because this method is not implemented in this browser (see MDN documentation).
Using another method, like browser.window.getCurrent() to test the promise availability would be a better idea.

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;
Copy link
Contributor

@ExE-Boss ExE-Boss Apr 4, 2018

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;
Copy link
Contributor

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, " $&");
Copy link
Contributor

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/
Copy link
Contributor

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) {
Copy link
Contributor

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.

@rpl
Copy link
Member

rpl commented Jun 21, 2018

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).

@rpl rpl closed this Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants