Enable usage via ProvidePlugin

[fregante]

• Actually fixes Document how to use webextension-polyfill with webpack.ProvidePlugin #156

Rereading my last attempt at dissuading webpack developers to fix ProvidePlugin’s awkward self-providing behavior, I realized that there must be a way to fool webpack into ignoring this replacement.

plugins: [
  new ProvidePlugin({
    browser: "webextension-polyfill"
  })
]

ProvidePlugin looks for the raw browser variable and replaces it; Turning it into a window property access should break it — if not, typeof window["brow" + "ser"] certainly will.

This change should be non-breaking, as an alternative solution to #350 — rather, as a stopgap solution to it.

[rpl]

@fregante this change breaks the expected behavior when running on Firefox (the CI job failure is triggered by the "the browser API object should not be changed on Firefox" assertion, which was added on purpose to prevent to missing that kind of issue).

We discussed about #350 during our last triage and we agreed that building an additional version of the polyfill as a pure ESM version (in addition to the UMD one, we can evaluate to remove support for the older kinds of module loading in future version, separately from better supporting ESM module style) would be reasonable from our perspective.

would that be a reasonable solution to #350 also from your perspective?

[codecov]

Codecov Report

Merging #351 (d58fa73) into master (2d75968) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #351   +/-   ##
=======================================
  Coverage   91.03%   91.03%           
=======================================
  Files           1        1           
  Lines         145      145           
=======================================
  Hits          132      132           
  Misses         13       13           

Impacted Files	Coverage Δ
src/browser-polyfill.js	91.03% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d75968...d58fa73. Read the comment docs.

[fregante]

this change breaks the expected behavior when running on Firefox

I don't think it does, it's more likely that the tests are broken. The conditions are unchanged except for an additional window.browser check, which should not affect this.

Also the tests pass locally so I'm not sure why it's failing on CI. Could you take a look at it again? You can see that it's working correctly because the same test (browser API object should not be changed on Firefox) passes in the background page, so the logic should be sound.

I'm not sure about what it takes to fix the tests though or why the content script tests are failing specifically.

we agreed that building an additional version of the polyfill as a pure ESM version

🎊

would that be a reasonable solution to #350 also from your perspective?

#350 yes, but this PR is for #156. It only affects #350 insofar as allowing the usage via webpack as a regular polyfill.

Edit: Actually no, #350 would not be resolved by just adding an ESM version because neither one would be a polyfill, still — Unless the ESM version also polyfills, e.g.

export default browser;
if (! current browser detection logic) {
	globalThis.browser = browser;
}

[fregante]

I tested this on Refined GitHub and it worked in both Firefox and Chrome.

The last commit was required to make it work in Firefox because the free-range browser variable at the end of the file was still being replaced by ProvidePlugin.

[fregante]

Should this be merged? It's been open for a while

[rpl]

Should this be merged? It's been open for a while

yeah, sorry about that, I have not been able to get back to this repo during December, I've been trying to schedule some time to spend for some maintenance work over the github repos once I got back but didn't got to it yet.

Anyway, the change in the final version of this patch is still minimal and the existing tests are now all passing, this should be ok to merge and then release in a new webextension-polyfill version, in the meantime I'm going to approve and merge this change in a minute.

I should be able to spend some more time and cut a new release in the next few days (hopefully by the end of the week).

[fregante]

In a future modernization effort, these could be replaced by:

Suggested change

	if (typeof globalThis != "object" || typeof chrome != "object" || !chrome || !chrome.runtime || !chrome.runtime.id) {
	throw new Error("This script should only be loaded in a browser extension.");
	}
	if (typeof globalThis.browser === "undefined" || Object.getPrototypeOf(globalThis.browser) !== Object.prototype) {
	if (!globalThis.chrome?.runtime?.id) {
	throw new Error("This script should only be loaded in a browser extension.");
	}
	if (!globalThis.browser?.runtime?.id) {