Opened 4 years ago

Closed 3 years ago

#3692 closed change (fixed)

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

Reported by: juliandoucette Assignee: kzar
Priority: P2 Milestone:
Module: Sitescripts Keywords:
Cc: fhd, saroyanm, greiner, trev, erikvold, sebastian, kvas, kzar Blocked By:
Blocking: #4864, #4871, #4878, #4993 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29346539/
https://codereview.adblockplus.org/29374555/
https://codereview.adblockplus.org/29376565/
https://codereview.adblockplus.org/29376668/
https://codereview.adblockplus.org/29376671/
https://codereview.adblockplus.org/29376719/
https://codereview.adblockplus.org/29377779/
https://codereview.adblockplus.org/29377837/

Description (last modified by kzar)

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.

Change History (59)

comment:1 Changed 4 years ago by juliandoucette

  • Summary changed from Create ESLint config to Create standard ESLint config for ABP projects

comment:2 Changed 4 years ago by juliandoucette

  • Description modified (diff)

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by 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.

comment:5 Changed 3 years ago by juliandoucette

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

comment:6 Changed 3 years ago by fhd

  • Cc sebastian added

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.

comment:7 follow-up: Changed 3 years ago by 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.

comment:8 Changed 3 years ago by sebastian

  • Cc kvas kzar added

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.

comment:9 in reply to: ↑ 7 Changed 3 years ago by erikvold

Replying to trev:

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

+1 for this

comment:10 Changed 3 years ago by juliandoucette

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

comment:11 Changed 3 years ago by 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 */

comment:12 Changed 3 years ago by juliandoucette

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:13 Changed 3 years ago by 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.

Last edited 3 years ago by juliandoucette (previous) (diff)

comment:14 Changed 3 years ago by juliandoucette

  • Resolution set to rejected
  • Status changed from reviewing to closed

comment:15 Changed 3 years ago by 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.

Last edited 3 years ago by kzar (previous) (diff)

comment:16 Changed 3 years ago by kzar

  • Component changed from Unknown to Sitescripts
  • Description modified (diff)
  • Owner set to kzar
  • Summary changed from Create standard ESLint config for ABP projects to Create standard ESLint config for Adblock Plus projects

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.

comment:17 Changed 3 years ago by kzar

  • Resolution rejected deleted
  • Status changed from closed to reopened

comment:18 Changed 3 years ago by kvas

  • Priority changed from Unknown to P2
  • Ready set

comment:19 Changed 3 years ago by kzar

  • Component changed from Sitescripts to Build-and-Release-Tools
  • Description modified (diff)
  • Summary changed from Create standard ESLint config for Adblock Plus projects to Create ESLint configuration for core Adblock Plus extension repositories

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

comment:20 Changed 3 years ago by 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.

comment:21 Changed 3 years ago by 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).

comment:22 Changed 3 years ago by kzar

  • Blocking 4864 added

comment:23 Changed 3 years ago by kzar

  • Blocking 4871 added

comment:24 Changed 3 years ago by kzar

  • Component changed from Build-and-Release-Tools to Unknown
  • Ready unset

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

comment:25 Changed 3 years ago by kzar

  • Component changed from Unknown to Sitescripts
  • Description modified (diff)
  • Ready set
  • Summary changed from Create ESLint configuration for core Adblock Plus extension repositories to Create sharable ESLint configuration defining our (modern) JavaScript coding practices

comment:26 Changed 3 years ago by 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?

comment:27 follow-up: Changed 3 years ago by kzar

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

comment:28 in reply to: ↑ 27 Changed 3 years ago by 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.

comment:29 Changed 3 years ago by 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.

Last edited 3 years ago by sebastian (previous) (diff)

comment:30 Changed 3 years ago by kzar

  • Description modified (diff)

comment:31 Changed 3 years ago by 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.

comment:32 Changed 3 years ago by kzar

  • Blocking 4878 added

comment:33 Changed 3 years ago by abpbot

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

comment:34 Changed 3 years ago by kzar

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:35 Changed 3 years ago by kzar

  • Resolution fixed deleted
  • Status changed from closed to reopened

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"}]
Last edited 3 years ago by kzar (previous) (diff)

comment:36 Changed 3 years ago by 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.

comment:37 Changed 3 years ago by 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.

Last edited 3 years ago by sebastian (previous) (diff)

comment:38 Changed 3 years ago by sebastian

Last edited 3 years ago by sebastian (previous) (diff)

comment:39 Changed 3 years ago by juliandoucette

  • Review URL(s) modified (diff)

comment:40 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

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.

comment:41 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)

comment:42 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)

comment:43 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)

comment:44 Changed 3 years ago by 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.

comment:46 Changed 3 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:47 Changed 3 years ago by kzar

  • Resolution fixed deleted
  • Review URL(s) modified (diff)
  • Status changed from closed to reopened

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

comment:48 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)

comment:50 follow-up: Changed 3 years ago by kzar

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

comment:51 in reply to: ↑ 50 Changed 3 years ago by 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.

comment:52 Changed 3 years ago by 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.

comment:53 Changed 3 years ago by 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.

comment:54 Changed 3 years ago by 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.

comment:55 Changed 3 years ago by kzar

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

Last edited 3 years ago by kzar (previous) (diff)

comment:56 Changed 3 years ago by kzar

  • Resolution fixed deleted
  • Review URL(s) modified (diff)
  • Status changed from closed to reopened

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

comment:57 Changed 3 years ago by kzar

  • Status changed from reopened to reviewing

comment:58 Changed 3 years ago by kzar

  • Blocking 4993 added

comment:59 Changed 3 years ago by kzar

  • Resolution set to fixed
  • Review URL(s) modified (diff)
  • Status changed from reviewing to closed

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

Note: See TracTickets for help on using tickets.