Create sharable ESLint configuration defining our (modern) JavaScript coding practices

[juliandoucette]

Background

We have quite a thorough ​coding style guide for JavaScript code and even more rules for when using ​modern JavaScript (ES6). (More still which are undocumented!)

This leads to friction in codereviews whilst new contributors are brought up to speed with how we do things. It wastes time and worse can upset people. It would be much better if - where possible - this work could be done automatically with a linter.

In order to make a start towards this we are looking to use ​ESLint to check the JavaScript code in some of our main Adblock Plus extensions repositories which all now target modern JavaScript. Namely adblockplus, adblockpluschrome, adblockpluscore and adblockplusui.

(Linting other repositories, especially those that have to support older browsers which don't support ES6 is out of scope for this issue.)

What to change

​Create a Node.js module called eslint-config-eyeo inside of the codingtools repository. Have that module act as a ​shareable ESLint configuration for use from our other projects.

[fhd]

Yes please!

That other project I mentioned in #3691 - we're using JSHint there for that purpose. Again for reference, here's what we have there:

{
  "esversion": 6,
  "freeze": true,
  "mocha": true,
  "node": true,
  "globals":
  {
    "window": true,
    "chrome": true
  },
  "maxdepth": 4,
  "maxstatements": 64,
  "noarg": true,
  "nocomma": true,
  "strict": "global",
  "latedef": true
}

Again, we can't use the same one (ES6 being one tiny problem :D), just for reference.

[trev]

For reference, my config under ​https://github.com/palant/easypasswords/blob/f8fd8e3/package.json#L31 is pretty much what we are using for ABP as well. It might be too strict however.

[juliandoucette]

Thanks @trev (and thanks for pointing me towards easypasswords too! :) )

[fhd]

I've got concerns about duplicating this in all our JS projects. What about distributing this via buildtools? We already have buildtools as a dependency in most repositories because it's needed for ensure_dependencies.py.

[trev]

The alternative would be sharable configs (​http://eslint.org/docs/developer-guide/shareable-configs). In projects where we don't use a Node environment I guess that ESLint is supposed to be installed globally - the shareable config can be installed globally as well then.

[sebastian]

Only few projects (i.e. only JavaScript extensions) have actually buildtools fully integrated (i.e. a build.py script). And some projects (i.e. all website projects) don't even have a dependency on the builtools repository.

So if we want to standardize JavaScript linting, buildtools seems the wrong place to do that. Let's just put the config up in some central place (rather than per repo) and link it from the coding style, similar to how we do it for Python/flake8 now. And once we have proper CI we can integrate it there.

[erikvold]

Replying to trev:

The alternative would be sharable configs (​http://eslint.org/docs/developer-guide/shareable-configs).

+1 for this

[juliandoucette]

I think we need separate configs for browser and plugins because of ES version and globals.

[trev]

Globals and environment don't need to be specified in this config, these are often different even for files within the same repository. It is possible to use inline configuration for that, e.g.:

/* eslint-env node */
/* global chrome */

[juliandoucette]

I separated them in the review. I still think that there (now, and probably in the future) enough differences between the rules for plugins/browser that it justifies separate configs.

[kzar]

Why was this issue closed as rejected? I think having an ESLint config would be a good thing, it would help with code reviews and perhaps with continuous integration in the future.

[kzar]

After discussions with Julian and Felix I'm going to have a try at moving this forward. I'm assuming this will live in the codingtools repository and so I'm assigning the sitescripts module.

I'll start off with what Julian's come up with and run it on our codebase to see what it catches / misses. Then I'll upload a new codereview when I have something I'm happy with. (I think the commit should be in Julian's name when this lands since he's done mostly all the work here, and since he was kind of let down by me on the codereview.)

I agree with Felix that this should be kind of a priority since it will help with codereviews, especially as we add more members to the team. I would say this should be P2 but since I'm no longer a sitescripts peer I'll leave it as unknown.

[kzar]

(Whilst updating the description I realised that this really lives in buildtools, since we want to be able to lint at build time automatically.)

[juliandoucette]

(Whilst updating the description I realised that this really lives in buildtools, since we want to be able to lint at build time automatically.)

Build tools module or build tools repository?

If repository, I would suggest codingtools because we commonly lint as we code not after we code.

[kzar]

I meant the buildtools repository, I think that is where this ESLint configuration belongs.

All the repositories we're targeting (adblockplus, adblockpluschrome, adblockpluscore and adblockplusui) have buildtools as a dependency. This means we can have project specific configurations (like globals such as chrome and specific third-party files to ignore) in each repository which ​extend from the main configuration in buildtools. I've already been playing with that today and it's working pretty well.

Secondly it means that we can eventually integrate linting into the build process, so that the code is linted at the same time that the extensions are built (which includes during development, since we still have a build step there).

[kzar]

Unmarking this as ready since a discussion has now started in IRC about where the file should live and how it should be distributed.

[greiner]

Just a minor detail but wouldn't the name eslint-config-eyeo make more sense given that our coding style not only applies to Adblock Plus?

[kzar]

I chose eslint-config-abp since it's consistent with the Python linting configuration flake8-abp in the same repository.

[greiner]

Replying to kzar:

I chose eslint-config-abp since it's consistent with the Python linting configuration flake8-abp in the same repository.

What were the reasons for choosing flake8-abp instead of flake8-eyeo? Because it might make sense to rename it then in case it's used for Python code that's not directly related to Adblock Plus. Otherwise it sounds reasonable for the two names to be inconsistent given that they would apply on different scopes.

[sebastian]

If we go with eslint-config-eyeo (instead of eslint-config-abp), I agree, that renaming flake-abp to flake8-eyeo makes even more sense and should be done as well. The reason it didn't occur to me to name it flake8-eyeo in the first place, was that (until recently), all of our development (whether directly related to Adblock Plus, or not) happened under Adblock Plus brand; we use @adblockplus.org email addresses, the code is hosted on hg.adblockplus.org, our coding practices which those linters enforce are documented on ​https://adblockplus.org/coding-style, etc.

[kzar]

Discussion continued in IRC:

We've all agreed codingtools is a better place for this to live.

Most people seem to think making a proper Node.js module for this would be a good idea.

We've gone with eslint-config-eyeo rather than eslint-config-abp since Thomas preferred that and neither me nor Sebastian had a strong opinion. (We all agreed that renaming flake8-abp to flake8-eyeo for consistency would be nice but that's out of scope for this issue.) For reference here's the ​ESLint shareable configuration docs.

We attempted to distribute the Node.js via our source control (both hgweb and GitHub) using ​any method npm supported but unfortunately came up short. The sticking point was NPM provided no way to install a package from a sub-directory for either a tarball or a Git repository. So we've decide to instead publish the Node.js module on ​https://www.npmjs.com.

[abpbot]

A commit referencing this issue has landed:
​Issue 3692 - Add our shared ESLint configuration...

[kzar]

Reopening this since Sebastian pointed out ​I missed one bit of feedback regarding the ​no-unused-vars rule. We're considering to add this:

"no-unused-vars": ["error", {"vars": "local", "args": "none"}]

[kzar]

While the suggested no-unused-vars did ​catch a few (3) things in adblockpluschrome it also resulted in many more (25) false positives:

• utils.js: Services, Synchronizer, Utils, Prefs, FilterStorage, FilterNotifier, Subscription, DownloadableSubscription, Filter, BlockingFilter, defaultMatcher, E.
• lib/compat.js: importAll, onShutdown, Cc, Ci, Cr, XPCOMUtils, Services, FileUtils
• qunit/common.js: prepareFilterComponents, restoreFilterComponents, preparePrefs, restorePrefs, executeFirstRunActions

My opinion is that we should probably just leave the rule disabled, but we could enable it if you'd accept inline comments to disable it again for troublesome files.

[sebastian]

It seems you didn't use no-unused-vars with the options suggested above. The unused vars it is complaining about are global variables that aren't used in the same script (but potentially in other scripts). Settings vars to local, as suggested above, will get rid of those errors.

As concluded on IRC, it turned out that the problem is that enabling the commonjs environment overrules the no-unused-vars's vars option. This kinda makes sense, since if these scripts would really be CommonJS modules, their global variables wouldn't be visible to any other code, hence if unused these variables would be in fact redundant.

We could disable the commonjs environment for these scripts. But then we'd still have to list these globals in the scripts which use them in order to avoid no-undef errors there. However, as I already pointed out in regard to the boilerplate related to no-undef, we should rather turn these scripts into proper modules. These will improve our code structure, while getting rid of almost all inline configuration, altogether. After all, the no-unused-vars and no-undef errors are just symptoms of an actual issue with our code, that we have too many global variables and don't use modules consistently.

[kzar]

Yea it seems you're right, when I remove the commonjs environment the rule behaves properly. So if you're willing to avoid using commonjs in the project configurations I agree that adding the rule is useful. I already caught a few mistakes with it.

[sebastian]

Well, as I said, I'd prefer to get rid of those global variables in favor of modules. If we don't do this right ahead, I guess, it's still better to have no-unused-vars enabled, and as a workaround disable the commonjs environment and explicitly configure the require and exports globals instead, for now.

[abpbot]

Some commits referencing this issue have landed:

• ​Issue 3692 - Enable the no-unused-vars ESLint rule
• ​Issue 3692 - Disable no-constant-condition ESLint rule for loops
• ​Issue 3692 - Disable max-len ESLint rule for regexp literals
• ​Issue 3692 - Turn off the no-empty ESLint rule
• ​Issue 3692 - Release eslint-config-eyeo 1.0.1

[kzar]

Reopening this since we're not discussing adding the arrow-parens rule.

[abpbot]

Some commits referencing this issue have landed:

• ​Issue 3692 - Remove curly ESLint rule...
• ​Issue 3692 - Release eslint-config-eyeo 1.0.2

[kzar]

So in the end we agreed not to add the arrow-parens rule to the main configuration. We will likely juts add the rule to repositories such as adblockplusui that have their own convention.

Whilst discussion that the curly rule came up again. Whilst people agreed on enforcing braces for multiple line blocks, they did not agree on enforcing any rule for single line ones. It turns out ESLint provides no option for the rule that suits us and so we've removed it.

[trev]

Replying to kzar:

We will likely juts add the rule to repositories such as adblockplusui that have their own convention.

No, the idea is that all projects have uniform style requirements, so individual per-repository rules are undesirable. The per-repository configuration is only there to express the difference in environments.

[kzar]

In practice we have different arrow function paren conventions depending on the repository. In both adblockpluschrome and adblockpluscore I've been asked to remove redundant parenthesis, but in adblockplusui I've been asked to always use them for consistency.

I don't see how being explicit about those conventions in our ESLint configurations is a bad thing, and in fact it will help avoid wasting time in codereviews. Of course having one convention for all of our code would have been preferable but no one seemed to agree with me there.

[greiner]

Seems like this discussion is getting too emotional so let me outline what we have:

• Module owners might prefer certain syntax.
• Felix made clear that authors should decide how they want to write code as long as it doesn't violate our coding style.
• We agreed that we don't want to make our coding style too restrictive especially in cases where we don't need to.

Therefore it appears to boil down to those two options:

• Don't allow any additions to the ESLint configuration by code modules.
• Allow additions to the ESLint configuration but only if it makes them stricter, not looser.

Personally, I don't have a preference in that regard so I'd be happy with either result. For that I should also note that my comments in regards to consistency of arrow functions were made before Felix voiced his above mentioned stance on the issue.

[fhd]

Therefore it appears to boil down to those two options:

• Don't allow any additions to the ESLint configuration by code modules.
• Allow additions to the ESLint configuration but only if it makes them stricter, not looser.

The point is that we should _not_ have different arrow function paren conventions (etc) on a per module basis, we should have _one_ coding style. I feel rather strong about that, so does Wladimir apparently. If somebody doesn't agree with this part let's discuss, but so far I'm assuming that's our goal.

Assuming that's our goal, allowing modules to make the ESLint configuration stricter based on the module owner's preferences isn't actually an option.

In practice we have different arrow function paren conventions depending on the repository. In both adblockpluschrome and adblockpluscore I've been asked to remove redundant parenthesis, but in adblockplusui I've been asked to always use them for consistency.

I don't see how being explicit about those conventions in our ESLint configurations is a bad thing, and in fact it will help avoid wasting time in codereviews. Of course having one convention for all of our code would have been preferable but no one seemed to agree with me there.

I agree, but as I wrote above: I don't think we should have per repository/module conventions to begin with. So while we're working on a shared ESLint configuration, we should eliminate these IMO.

The fastest way to deal with this I can think of is to just ignore everything that is (deliberately) inconsistent for now and make it explicit that it is up to the _author_ of code (not the reviewer / module owner) to decide. As a consequence, these things will be inconsistent. Is that a problem? In the cases I'm aware of so far, I personally don't think so. If it causes readability issues or conflicts in practice, we can always make the global configuration more strict about these things later, on a case-by-case basis. The non-controversial parts should be a lot easier to land and will already help save a ton of time, I'm sure.

[kzar]

I will remove the arrow-parens rule from adblockplusui and other repositories.

[kzar]

Reopening since we might be changing our configuration of the operator-linebreak rule.

[kzar]

Resolving again, we've moved the operator-linebreak discussion to #4993.