Issue 5079 - Turn elemHideEmulation into a CommonJS module

lib/content/elemHideEmulation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+/* eslint-env browser */

[kzar, 2017/06/12 12:50:13]

Why is this necessary?

[hub, 2017/06/12 13:26:33]

The mistake here is that I should have the proper .eslintrc instead. Will fix
that.

We definitely need the browser env.

[kzar, 2017/06/12 13:35:12]

Sorry, but I still don't understand why it's necessary?

[hub, 2017/06/12 14:03:14]

This change is gone in the current version of the patch I use the .eslintrc.json
that was in chrome/content

It is necessary as we run in the browser environment where the `document` and
`console` globals are defined. This is just carrying over from the existing
code.

[kzar, 2017/06/12 14:12:44]

Right now I understand, previously this file was ignored by ESLint but it has
been renamed. I guess the old name should be removed from .eslintignore now.

Also I seem to remember Wladimir disagreeing that the browser env should be
added globally, but I can't remember why. I'll ask in IRC.

[hub, 2017/06/12 15:21:50]

This is now totally gone in the current patch since the ancestor patches
actually address the problem (an actual bug)

 /* globals filterToRegExp */
 
 "use strict";
 
 const abpSelectorRegexp = /:-abp-([\w-]+)\(/i;
 
 function splitSelector(selector)
 {
   if (selector.indexOf(",") == -1)
     return [selector];
@@ skipping 417 matching lines @@
 
       if (this.patterns.length > 0)
       {
         let {document} = this.window;
         this.addSelectors(document.styleSheets);
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
     });
   }
 };
+
+if (typeof exports != "undefined")

[kzar, 2017/06/12 12:50:13]

Don't we expect exports to always exist like for the other modules?

[hub, 2017/06/12 13:26:34]

Sadly no, we can't expect that yet.

-in the tests, this is not loaded using require() at least until we fix this.
-common.js does it for the same reason.

[kzar, 2017/06/12 13:35:12]

Ah right, well I think we need to fix that at the same time as we make this
change. See my other reply.

[hub, 2017/06/12 14:03:13]

agreed.

[kzar, 2017/07/07 13:42:20]

After reading through this review again I think removing this check and the
similar one from common.js is what still needs doing. Could you remove those,
fixing the related stuff you mentioned above?

[hub, 2017/08/10 14:46:21]

Acknowledged.

+{
+  exports.ElemHideEmulation = ElemHideEmulation;
+  exports.splitSelector = splitSelector;
+}