Add support for browsers which don’t support Promises

[ExE-Boss]

Fixes #3
Closes #43 and closes #61

Also fixes #68 and closes #86

---

Depends on #139 (merged)

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

[ExE-Boss]

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

needinfo?(@rpl)

[rpl]

@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]

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]

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]

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()) {

[ExE-Boss]

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.

[fregante]

Just change ! to =

[fregante]

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

[ExE-Boss]

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

[ExE-Boss]

See #114 (comment):

Oh great, and now Firefox stopped working, again.

[ExE-Boss]

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

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

[fregante]

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

[ExE-Boss]

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]

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]

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---]

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

[PoziWorld]

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

[leonid-shevtsov]

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]

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]

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

[ExE-Boss]

@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]

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---]

@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]

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]

@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]

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.