Add support for browsers which don’t support Promises by ExE-Boss · Pull Request #114 · mozilla/webextension-polyfill

ExE-Boss · 2018-05-13T11:11:07Z

Fixes #3
Closes #43 and closes #61

~~Also ~~fixes~~ #68 and ~~closes~~ #86~~

Depends on ~~#139~~ (merged)

review?(@Rob--W, @rpl)

ExE-Boss · 2018-06-04T12:59:22Z

The build seems to be failing for reasons unrelated to the patch.

needinfo?(@rpl)

rpl · 2018-06-16T20:57:24Z

@ExE-Boss it is unfortunate that the integration tests are not currently able to show the actual reason for the failure in this case, but the failure is definitely related to the changes applied by this PR.

Looking if browser is defined on the window object is not currently a viable option, because it breaks the detection of the existent browser API object on Firefox, on which window !== this in a content script (as I also mentioned in #115 (comment)), unfortunately the test suite is not currently able to catch the actual error which is:

TypeError: target is not an object or null  browser-polyfill.js:907:27

ExE-Boss · 2018-06-17T12:24:27Z

Oh great, what happened now?

~~At least I got it to work on my machine.~~ Or not.

Ok, b3609f7 broke Chrome, and it apparently wraps Firefox.

ExE-Boss · 2018-06-17T17:14:50Z

I got the build to pass on Travis 🎉

Thanks @rpl for your help 👍🏻

Ok, why does 73d5e4b work and 6fd999e doesn’t? (73d5e4b...6fd999e)

Edit: Right, this !== window on Firefox in content scripts.

fregante · 2018-06-17T21:45:14Z

src/browser-polyfill.js

+    }
+  }
+  return supportsPromises;
+})()) {


That's a bit wordy, how about this function instead?

function supportsPromises() { if (typeof browser === "undefined") { return false; } // If `browser.runtime.lastError` doesn’t exist, assume promises are supported. if (browser.runtime && typeof browser.runtime.lastError !== "undefined") { return true; } if (browser.extension && typeof browser.extension.lastError !== "undefined") { return true; } try { return isThenable(browser.runtime.getPlatformInfo()); } catch (error) {} try { return isThenable(browser.windows.get(browser.windows.WINDOW_ID_CURRENT)); } catch (error) {} return false; } if (supportsPromises()) {

browser.runtime && typeof browser.runtime.lastError !== "undefined" returns true if browser.runtime.lastError exists and false if it doesn’t.

Also, the check needs to check for the case when browser.runtime doesn’t exist, but browser.extension does, and consider the check failed if browser.runtime.lastError exists, but browser.extension.lastError doesn’t for whatever reason.

Just change ! to =

consider the check failed if browser.runtime.lastError exists, but browser.extension.lastError doesn’t for whatever reason.

That’s not what your code does. The ternary operator checks for one runtime and then discards extension

Well, yours discards the one in extension if none exists in runtime.

~~Also, yours fails the following test:~~

webextension-polyfill/test/test-browser-global.js

Lines 29 to 59 in 73d5e4b

it("wraps the global browser namespace if it doesn’t support promises", () => {

const fakePlatformInfo = {

os: "test",

arch: "test",

};

const fakeBrowser = {

runtime: {

lastError: null,

getPlatformInfo: (cb) => {

if (typeof cb !== "function") {

throw new Error(cb + " is not a callback");

}

cb(fakePlatformInfo);

},

},

};

return setupTestDOMWindow(undefined, fakeBrowser).then(window => {

console.log(window.browser.runtime.getPlatformInfo());

notEqual(window.browser, fakeBrowser,

"The existing browser has been wrapped");

return Promise.all([

window,

window.browser.runtime.getPlatformInfo(),

]);

}).then(([window, platformInfo]) => {

deepEqual(platformInfo, fakePlatformInfo,

"The wrapped browser returns the expected value");

});

});

Edit: Turns out I forgot to change the !==s to ===.

See #114 (comment):

Oh great, and now Firefox stopped working, again.

ExE-Boss · 2018-06-18T00:34:21Z

@bfred-it Oh great, and now Firefox stopped working, again.

Honestly, your profile picture perfectly summarises my current reaction to this:

fregante · 2018-06-18T02:14:17Z

src/browser-polyfill.js

+    return isThenable(browser.runtime.getPlatformInfo());
+  } catch (error) {
+    // Microsoft Edge doesn’t support `browser.runtime.getPlatformInfo()`
+    try {


There’s not need to nest this. If there are no errors, the function will return before the second try/catch

ExE-Boss · 2018-06-21T19:24:36Z

With the above changes, we are making sure that extensionAPIs is going to be the original "extensions APIs" object and so we will also need to change the two places where the polyfill is currently referring directly to chrome.runtime.lastError, so that it uses extensionAPIs.runtime.lastError

It already does that by making chrome a parameter to the wrapAPIs function:

webextension-polyfill/src/browser-polyfill.js

Line 60 in a62cf86

const wrapAPIs = chrome => {

rpl · 2018-06-21T19:35:27Z

It already does that by making chrome a parameter to the wrapAPIs function:

yeah I did notice that, but I'm thinking (aloud) that I would kind of prefer if we don't use chrome elsewhere inside the polyfill's "wrapping code" (besides the initial preamble), so that it cannot be mistaken as accessing the global chrome object while reading the code
(I mean, clearly it doesn't make any difference for the machines, but it may be helpful for humans ;-)).

ExE-Boss · 2018-07-01T11:30:53Z

It already does that by making chrome a parameter to the wrapAPIs function:

yeah I did notice that, but I'm thinking (aloud) that I would kind of prefer if we don't use chrome elsewhere inside the polyfill's "wrapping code" (besides the initial preamble), so that it cannot be mistaken as accessing the global chrome object while reading the code.

The reason why I didn’t rename chrome to extensionAPIs was so that other PRs wouldn’t need to be rebased and to minimise possible merge conflicts (I’ve opened PR #139 to make this change).

NN--- · 2018-07-11T06:10:48Z

Any updates?
This repository seems not so maintained what makes people to invent a different workarounds.

PoziWorld · 2018-07-27T04:28:25Z

Please consider an alternative solution to support Microsoft Edge.

leonid-shevtsov

This PR worked for our extension after removing the two checks for lastError. Polyfill established the correct API in Chrome and Edge (and Firefox, too).

src/browser-polyfill.js

leonid-shevtsov · 2018-09-17T12:58:47Z

Also, window.browser = require('browser-polyfill') messes up the proxying wrappers and produces weird errors. Which is fine - I just needed to stop using the global browser variable.

StigNygaard · 2018-11-18T13:34:17Z

Is it okay to ask if there's still development on this, and if we can expect "unofficial support" for MS Edge in a foreseeable future?

Gaelan · 2019-01-06T08:06:48Z

Edge is becoming a Chromium variant, so this might be pointless.

ExE-Boss · 2019-01-06T08:36:10Z

@Gaelan

Edge is becoming a Chromium variant, so this might be pointless.

This PR isn’t just for Microsoft Edge (even though that is the main reason why this PR got made).

This PR is to support browsers which define the browser namespace but don’t support using promises, and instead require callbacks.

Also, even if Edge will switch to Chromium, there’s still the fact that if Microsoft wants to be backwards compatible, they’ll have to keep defining the browser namespace.

In fact, Edge doesn’t support the chrome namespace at all, unlike Firefox, which supports it for compatibility purposes.

Rob--W · 2019-01-07T11:03:53Z

Since the announcement of Microsoft that Edge switches to Chromium, the effort to support Edge is put on hold until the exact details become available. It is quite likely for Edge to behave identical to Chrome, in which case we don't need any more modifications to support Edge.

This PR is to support browsers which define the browser namespace but don’t support using promises, and instead require callbacks.

I'm not aware of such a browser. Let's keep the main library simple; if anyone wants to develop an extension for non-Chromium Edge browsers, then they could create a build based on this pull request.

NN--- · 2019-01-07T11:17:09Z

@Rob--W The latest Windows 1809 is supported till May 11, 2021.
The library is needed for anyone using this version.
As well it is not clear whether WIndows 1903 will be released with Edge based Chromium moving support lifecycle even further.

Current solution/workaround is to use 'chrome' object and not 'browser'.
Write code with callbacks and not promises.
And for Edge just add one line: window.chrome = window.browser.
That way one can have one code base for Chrome,FF,Edge.

But this is not what people want. We want Promises API instead of callbacks which work in all browsers.

ExE-Boss · 2019-01-07T11:26:01Z

Since the announcement of Microsoft that Edge switches to Chromium, the effort to support Edge is put on hold until the exact details become available. It is quite likely for Edge to behave identical to Chrome, in which case we don't need any more modifications to support Edge.

Except that Chromium Edge will certainly include the browser namespace due to Microsoft’s obsession with backwards compatibility.

Also, Opera supports both chrome and opera namespaces for extensions, so the Chromium engine definitely supports adding more global variables to the extension globals.

This PR is to support browsers which define the browser namespace but don’t support using promises, and instead require callbacks.

I'm not aware of such a browser. Let's keep the main library simple; if anyone wants to develop an extension for non-Chromium Edge browsers, then they could create a build based on this pull request.

Well, Edge matches that description, and Chromium Edge most certainly will too since normal Chromium doesn’t yet use Promises for the extension API.

Rob--W · 2019-01-07T11:40:37Z

@Rob--W The latest Windows 1809 is supported till May 11, 2021.
The library is needed for anyone using this version.
As well it is not clear whether WIndows 1903 will be released with Edge based Chromium moving support lifecycle even further.

"Microsoft Edge will now be delivered and updated for all supported versions of Windows" (source). This implies that supported versions of Windows (including 1809) will receive a Chromium-based Edge browser.

Except that Chromium Edge will certainly include the browser namespace due to Microsoft’s obsession with backwards compatibility.

I'd like to see a reputable source that supports this claim.

Well, Edge matches that description, and Chromium Edge most certainly will too since normal Chromium doesn’t yet use Promises for the extension API.

By "I'm not aware of such a browser", I meant that there is no supported browser (Microsoft is discontinuing their current engine, so that doesn't count) that would get API support through this PR. The polyfill already supports Chromium-based browsers.

rpl · 2019-01-07T12:46:07Z

As @Rob--W already mentioned in his last round of comments, with the Microsoft announcement related to the MSEdge future there are even less compelling reasons to officially include in the polyfill any workarounds specific to MSEdge (and then maintaining them over the time).

There are no other browsers that exposes the browser global, and personally I was already not thrilled at all about adding to the polyfill "runtime detection of the Promised-based APIs".

MSEdge doesn't have a lot of extensions, and it is very likely that the extensions that they have are already been developed for Chrome, and so I'm not really sure that they need to define the browser global to keep any compatibility for them, in my personal opinion their best "compatibility"-move is more likely to just "try to don't be incompatible with existing Chrome and Firefox extensions".

For anyone that still have to work on MSEdge extensions before it is switched to the next Chromium-based version, a better bet may be to do the tweaks needed to make "MSEdge to look like Chromium (and allow the webextension-polyfill to load unmodified)" using the MSEdge specific --ms-preload manifest key and the Microsoft Edge Extension Toolkit's Chrome API bridges, I've just created the following github repository to explain this alternative approach in a bit more detail and provide a minimal test extension that shows how to put it together:

https://github.com/rpl/example-msedge-extension-with-webextension-polyfill

I'm going to close this PR, #163 and #3 as wontfix, for the reasons described above and in the last round of @Rob--W comments (and I'm not say this lightly, from my point of view it is quite sad that we are losing an additional indipendent implementation of the Extension APIs, and I was really willing to add MSEdge support as much as I could, e.g. that was the reason why I've been working on applying all the changes needed to run all our integration tests on MSEdge in #163), and I'm going to update the 'Supported Browsers' section from the README.md file in this repo accordingly.

ExE-Boss force-pushed the fix/ms-edge branch from 24af708 to dc836be Compare May 14, 2018 10:00

Add support for browsers which don’t support Promises

dc836be

ExE-Boss mentioned this pull request May 14, 2018

Release 0.3.0 #88

Closed

rpl mentioned this pull request May 14, 2018

test: Add cross browsers smoke tests and add support for sendMessage rejected promise behavior #115

Merged

Merge branch 'master' into fix/ms-edge

320371f

ExE-Boss force-pushed the fix/ms-edge branch from 21aabfc to 320371f Compare June 4, 2018 13:58

JimMorrison723 mentioned this pull request Jun 15, 2018

Make generated project fully compatible with Edge webextension-toolbox/generator-web-extension#20

Closed

ExE-Boss force-pushed the fix/ms-edge branch from 868d9c5 to ce7cfd6 Compare June 17, 2018 15:25

Merge branch 'master' into fix/ms-edge

a21cee5

ExE-Boss force-pushed the fix/ms-edge branch from ce7cfd6 to b3609f7 Compare June 17, 2018 16:22

Fix Firefox

b3609f7

ExE-Boss force-pushed the fix/ms-edge branch 3 times, most recently from 445e7e9 to 73d5e4b Compare June 17, 2018 17:13

Fix Chrome

73d5e4b

ExE-Boss mentioned this pull request Jun 17, 2018

Improve browser detection fix #68 #86

Closed

ExE-Boss force-pushed the fix/ms-edge branch from 6fd999e to 73d5e4b Compare June 17, 2018 17:29

This was referenced Jun 17, 2018

Different way of checking if browser is defined? #68

Closed

Compatibility with environments that have the browser namespace but don't support promises #3

Closed

fregante suggested changes Jun 17, 2018

View reviewed changes

Simplify the check for the lack of Promise support

7a4265b

fregante reviewed Jun 18, 2018

View reviewed changes

Un‑nest the browser.windows test

0cbd8b4

ExE-Boss mentioned this pull request Jun 18, 2018

Made compatible with Edge browser #61

Closed

Fix promise support check in content scripts

a62cf86

rpl mentioned this pull request Jun 21, 2018

Handle the browsers which don't support promises (eg. Edge) #43

Closed

ExE-Boss mentioned this pull request Jul 1, 2018

refactor: Make the wrapAPIs function take the chrome object as a parameter #139

Merged

Amndeep7 mentioned this pull request Jul 22, 2018

Edge support Amndeep7/ptw-extender#4

Closed

Smile4ever mentioned this pull request Aug 6, 2018

[REQ] port this addon/extension to MS Edge? Smile4ever/Neat-URL#153

Open

rpl mentioned this pull request Aug 16, 2018

Support Microsoft Edge #154

Closed

Merge branch 'master' into fix/ms-edge

27c9c10

balcsida mentioned this pull request Sep 9, 2018

[WIP] Make generated project fully compatible with Edge webextension-toolbox/generator-web-extension#26

Closed

leonid-shevtsov reviewed Sep 17, 2018

View reviewed changes

src/browser-polyfill.js Outdated Show resolved Hide resolved

danekja mentioned this pull request Oct 13, 2018

Document work required to support MS Edge kee-org/browser-addon#147

Closed

rpl mentioned this pull request Nov 26, 2018

test(integration): Add support for Edge to the integration test suite #163

Closed

balcsida mentioned this pull request Dec 1, 2018

webextension-polyfill should be included for Edge output webextension-toolbox/webextension-toolbox#105

Closed

Fix check that the lastError properties don’t exist

a3f8084

rpl added the state: wontfix label Jan 7, 2019

rpl closed this Jan 7, 2019

rpl mentioned this pull request Jan 7, 2019

Update MSEdge note in README.md #165

Closed

lidel mentioned this pull request Jan 8, 2019

Support for Microsoft Edge? ipfs/ipfs-companion#353

Closed

	it("wraps the global browser namespace if it doesn’t support promises", () => {
	const fakePlatformInfo = {
	os: "test",
	arch: "test",
	};

	const fakeBrowser = {
	runtime: {
	lastError: null,
	getPlatformInfo: (cb) => {
	if (typeof cb !== "function") {
	throw new Error(cb + " is not a callback");
	}
	cb(fakePlatformInfo);
	},
	},
	};

	return setupTestDOMWindow(undefined, fakeBrowser).then(window => {
	console.log(window.browser.runtime.getPlatformInfo());
	notEqual(window.browser, fakeBrowser,
	"The existing browser has been wrapped");
	return Promise.all([
	window,
	window.browser.runtime.getPlatformInfo(),
	]);
	}).then(([window, platformInfo]) => {
	deepEqual(platformInfo, fakePlatformInfo,
	"The wrapped browser returns the expected value");
	});
	});

Conversation

ExE-Boss commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ExE-Boss commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpl commented Jun 16, 2018

Uh oh!

ExE-Boss commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ExE-Boss commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante Jun 17, 2018

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Jun 17, 2018

Choose a reason for hiding this comment

Uh oh!

fregante Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ExE-Boss commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

ExE-Boss commented Jun 21, 2018

Uh oh!

rpl commented Jun 21, 2018

Uh oh!

ExE-Boss commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NN--- commented Jul 11, 2018

Uh oh!

PoziWorld commented Jul 27, 2018

Uh oh!

leonid-shevtsov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonid-shevtsov commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StigNygaard commented Nov 18, 2018

Uh oh!

Gaelan commented Jan 6, 2019

Uh oh!

ExE-Boss commented Jan 6, 2019

Uh oh!

Rob--W commented Jan 7, 2019

Uh oh!

NN--- commented Jan 7, 2019

Uh oh!

ExE-Boss commented Jan 7, 2019

Uh oh!

Rob--W commented Jan 7, 2019

Uh oh!

rpl commented Jan 7, 2019

Uh oh!

Reviewers

Assignees

Labels

ExE-Boss commented May 13, 2018 •

edited

Loading

ExE-Boss commented Jun 4, 2018 •

edited

Loading

ExE-Boss commented Jun 17, 2018 •

edited

Loading

ExE-Boss commented Jun 17, 2018 •

edited

Loading

ExE-Boss Jun 17, 2018 •

edited

Loading

fregante Jun 18, 2018 •

edited

Loading

ExE-Boss Jun 18, 2018 •

edited

Loading

ExE-Boss Jun 18, 2018 •

edited

Loading

ExE-Boss commented Jun 18, 2018 •

edited

Loading

ExE-Boss commented Jul 1, 2018 •

edited

Loading

leonid-shevtsov commented Sep 17, 2018 •

edited

Loading