Improve browser detection fix #68

[HaNdTriX]

By checking window.browser the webpack.ProvidePlugin won’t be confused.

[HaNdTriX]

@rpl is there anything I can help you with to get the PR merged?
What are the cons of using window.browser?

[HaNdTriX]

Just in case anyone is interested in a way to temporarily solve this issue without forking this package.
You can use the string-replace-loader module to monkey patch webextension-polyfill on the fly.

Just add the following loader to your webpack config:

[ExE-Boss]

Quoting myself from #68 (comment):

I personally prefer using:

if (!("browser" in window)) {
    // [...]
}

because using window.browser directly when testing if it exists tends to print dereferencing an undefined reference warnings in the console.

Edit: Turns out that that causes the tests to fail because of jsdom/jsdom#1622:

webextension-polyfill/test/setup.js

Lines 44 to 50 in 6f9cfdf

	// Set (or reset) the browser property.
	if (browserObject) {
	window.browser = browserObject;
	} else {
	// TODO: change into `delete window.browser` once tmpvar/jsdom#1622 has been fixed.
	window.browser = undefined;
	}

But using this function works:

const isDefined = (obj, key) => {
  if (typeof obj !== "object" || obj === null) {
    return false;
  }
  return key in obj && typeof obj[key] !== "undefined";
};

Edit 2: Turns out that that doesn’t work either because of #114 (comment) and #115 (comment)

[Rob--W]

The polyfill is supposed to be a no-op (not doing anything) in Firefox. In a content script, window.browser is undefined in Firefox, so this PR would incorrectly cause the global browser object to be replaced in Firefox.

Would #86 (comment), using this.browser instead of window.browser work? So that the output becomes something like this:

if (this.browser === undefined && window.browser === undefined) {
}

I'm closing the issue anyway, since there is a merge conflict and we intend to change the detection logic for Edge support (#3).

[vitalets]

So what is the suggested solution for using with webpack.ProvidePlugin?
The following does not work for me:

 plugins: [
    new webpack.ProvidePlugin({
      browser: ['webextension-polyfill'],
    }),
 ]

[HaNdTriX]

We are using the polyfill with webpack in the following way:
https://github.com/webextension-toolbox/webextension-toolbox/blob/master/src/webpack-config.js#L116-L134

[ExE-Boss]

@HaNdTriX

We are using the polyfill with webpack in the following way: https://github.com/webextension-toolbox/webextension-toolbox/blob/master/src/webpack-config.js#L116-L134

// The webextension-polyill doesn't work well with webpacks ProvidePlugin.
// So we need to monkey patch it on the fly
// More info: https://github.com/mozilla/webextension-polyfill/pull/86
config.module.rules.push({
  test: /webextension-polyfill[\\/]+dist[\\/]+browser-polyfill\.js$/,
  loader: require.resolve('string-replace-loader'),
  query: {
    search: 'typeof browser === "undefined"',
    replace: 'typeof window.browser === "undefined"'
  }
});

That won’t work on Firefox in content scripts, as in content scripts on Firefox, this !== window, it’s why #114 took me so long to get right.

See also b3609f7.

[Rob--W]

Just comparing with undefined is not solid. See #153 .

[vitalets]

@HaNdTriX Thanks for solution.

@Rob--W What about adding a short note about incompatibility with webpack.ProvidePlugin to the readme? It's not obvious and may take a long time to discover why it's not working. As from webpack docs Provide plugin is equivalent to require / import.

[Rob--W]

@vitalets The solution from @HaNdTriX should not be used, because 1) it causes the polyfill to load unconditionally in Firefox, and 2) it causes the polyfill to break in web pages that trigger registration of the "browser" global (see #153).

I don't have a solution ready yet, but for the time being I have opened #156 to track documentation of this feature.

[HaNdTriX]

@Rob--W Thanks for letting me know. Nevertheless we check inside webpack and exclude the polyfill for firefox anyway.

Here is our current workaround. Please keep in mind that on firefox these lines will be omitted.

[NN---]

@HaNdTriX

@rpl is there anything I can help you with to get the PR merged?
What are the cons of using window.browser?

In Firefox window.browser is undefined as opposed to browser.
https://bugzilla.mozilla.org/show_bug.cgi?id=1208775