@Niedzielski @Krinkle Have added the ones written down, there's at least one argument left out on the .eslintignore advantages, please update the description accordingly.

Change 457958 had a related patch set uploaded (by Jdlrobson; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: move ESLint ignores to distinct file

https://gerrit.wikimedia.org/r/457958

The informal decision in that meeting was to use .eslintignore everywhere but we also realise attendees today were low so please chip in if you have opinions on this matter.

We should also make sure this is recorded on http://mediawiki.org/wiki/Coding%20conventions

Change 457958 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: move ESLint ignores to distinct file

https://gerrit.wikimedia.org/r/457958

Most of the projects I have setup locally in put the ignores in Gruntfile.js, not package.json (about 50 of them).

Removing them from Gruntfile.js leads to the odd situation where eslint ignores are in a separate file, but stylelint and jsonlint ignores are still in package.json. If a new ignoreable folder is added I now have two places to update (or three if we create stylelint/jsonlint ignore files) instead of just one:

https://github.com/wikimedia/mediawiki-extensions-Cite/blob/master/Gruntfile.js#L19

Removing them from Gruntfile.js leads to the odd situation where eslint ignores are in a separate file, but stylelint and jsonlint ignores are still in package.json.

This example highlights one of @Volker_E's summary of package.json advantages: "one place for dev tooling". The tradeoff is that package.json may become a huge collection of things.

Having less files in the root folder is more hygiene than some lines in a file not looking often at, because there are many linters or tools wanting such config files

You are reverting T179195, please see there for more arguments

@Umherirrender sure this is the trade off.

I'd argue more files is better than a large package.json. I expect package.json to carry metadata - such as stability (version), complexity (number of dependencies) and if all our linters were to live inside package.json then it means its also the reference point for config.

When I imagine all configs inside package.json it gives me a headache - for my extensions that would be stylelint,eslint, eslintignore - all of which are large - that files no longer going to be easy to manage.

I actually think having .eslintignores etc is highly beneficial to a repo, as with a single glance I know exactly what tooling is in place. Given files prefixed with "." are hidden from my view, it feels pretty clean to me (and if this is really about files in the root directory, we could easily move these files to a subfolder. e.g. .eslintignore could live inside the resources folder), so that seems like a solveable problem to me.

Thanks for sharing the phab ticket, I hadn't seen that, but I also only see input from @WMDE-leszek @thiemowmde @Florian so it makes sense to chat about this now we have a larger audience!

Thanks for the ping @Umherirrender.
From my personal perspective, I share @Jdlrobson's view. Thanks for neatly summarizing this, in the other ticket I failed to get to the point that nicely.

That was my personal perspective. Will get some WMDE dev to chime in here in too, if this would be of any help.

Having one config file per tool is okay, so having the ignores in .eslintrc.json would be okay.
But I have no idea if eslint supports such a key in the config or is that okay for the concept of that file

In T204176#4579966, @Esanders wrote:

Most of the projects I have setup locally in put the ignores in Gruntfile.js, not package.json (about 50 of them).

Yeah, the problem is somewhat misrepresented. We were just in the middle of moving away from configuring ESLint inside Gruntfile, that has only just begun, with a handful of repos moving to package.json and other to .eslintignore. The majority of repos is still configured with the pattern I started around 2012 (explained below).

I believe that the specific aspect of the "ignore" attributes, and the two choices mentioned here, are really just a matter of personal taste. I do think there exist (minor) rational reasons, but they exist on both sides, and so far no reason seems strong enough by itself to make one obviously better than the other. If we are trying to decide on that alone, we should pick whichever and be consistent. But, I think it's worth looking at the bigger picture because I see some an inter-related aspect of "file" and "rules" attributes that are currently also quite inconsistent. I don't think we can realistically aim to have all software configured the same way (eslint, stylelint, etc.). But we should at least decide on what strategic goals we want to achieve with how we configure them and be consistent with how we configure a single software package. That is, we should pick a single strategy for the "config", "files", and "ignore" attributes of ESLint.

Bigger picture

While it's important to be consistent in small details. It's also important to be consistent in decisions that do have a strategic value.

I'll use the status quo of repos still using Gruntfile as starting point and hope to show how our status quo came to be, and why I believe it should change.

The way we configure Grunt and grunt-eslint today, matches how we configured JSHint (grunt-contrib-jshint) in the past, which in turn was copied to most of our extensions from mediawiki/core, where I placed it around 2012.

At the time, we just wanted to run JSHint from the command-line together with some other stuff, so we used Grunt, and grunt-contrib-jshint, and we'd run it locally and in CI using grunt test (with grunt-cli). With that as the only objective, the path of least resistance was (in my view, at the time), to put everything in one file and keep everything together, in an executable file specific to Grunt. There was nothing else in the picture that we had to be concerned with.

Fast-forward to 2018, and we've got plugin integrations using ESLint in IDEs, text editors, code-review tools, and more - as well as a growing set of command-line tools, including built-in commands such as eslint --fix.

As such, it makes sense to put configure ESLint in a standard and declarative format that ESLint discovers automatically, instead of passing it imperatively to eslint's Node.js API using require('eslint').CLIEngine, as grunt-eslint does.

We do this for our rule configuration currently already, which benefits many developers by having their IDEs pick up configuration automatically and provide real-time feedback. However, currently this only gets us half of the way there toward tooling integration, because while we provide rules configuration in the standard way, we don't for files/ignores.

I believe we should either follow the standard or not, but not half-way.

Options

ESLint has three standard interfaces:

• Declarative dotfiles. – https://eslint.org/docs/user-guide/configuring
• Declarative properties in package.json. – https://eslint.org/docs/user-guide/configuring
• Imperative, programmatic interface via its Node.js API, using require('eslint').CLIEngine. – https://eslint.org/docs/developer-guide/nodejs-api#cliengine

1) Declarative in .eslint* files

Implementation:

• This means rules go in .eslintrc.
• This means ignores go in .eslintignore.
• This means files is redundant/implicit as the project directory ..

Benefits:

• We only have a blacklist (ignores), not also a whitelist (files). Which helps prevent files from accidentally and silently becoming un-linted after refactoring or addition of new code in a repository e.g. due to an outdated whitelist.
• Tooling integration in IDEs for real-time linting based on the project config.
• Tooling integration on the CLI for e.g. eslint --fix ..
• Easy to find for developers familiar with ESLint, and/or upon checking ESLint docs.

Drawbacks:

• Two extra files in the project root.
• The logical entry point is separate from the configuration. That is, the eslint . command in package.json#scripts.test, or grunt.initConfig({ eslint: '.' }) in Gruntfile, with config in dotfiles instead.

# .eslintrc
{
  "extends": preset,
  "rules": overrides
}

# .eslintignore
excludes/

# package.json
{
  "scripts": {
    "test": "eslint . || grunt test"
  }
}

# Gruntfile.js (optional)
grunt.initConfig( {
  eslint: {
    all: '.'
  }
} );

2) Declarative in package.json

Implementation:

• This means rules go in package.json#eslintConfig.
• This means ignores go in package.json#eslintIgnore.
• This means files is redundant/implicit as the project directory ..

Benefits:

• We only have a blacklist (ignores), not also a whitelist (files).
• Tooling integration in IDEs.
• Tooling integration on the CLI for e.g. eslint --fix ..
• Easy to find for developers familiar with ESLint, and/or upon checking ESLint docs.
• Less files in the project root (compared to 1)
• Logical entry points are together with configuration (if using package.json#scripts.test).

Drawbacks:

• The logical entry point is separate from the configuration (if using grunt-eslint).
• More code inside package.json.

# package.json
{
  "scripts": {
    "test": "eslint . || grunt test"
  },
  "eslintConfig": {
    "extends": preset,
    "rules": overrides
  },
  "eslintIgnore": [
    "excludes/"
  }
}

# Gruntfile.js (optional)
grunt.initConfig( {
  eslint: {
    all: '.'
  }
} );

3) Imperative in Gruntfile.js

Implementation:

• This means "rules" are set in Gruntfile, using grunt.initConfig( { eslint: { baseConfig: ... } } );. This is something we've never done before, but would be consistent if we don't want to have the IDE integrations and just want things together in Gruntfile.
• This means "ignores" don't exist explicitly, implied based on the files not matched by the minimatch pattern in Gruntfile.
• This means "files" is set in Gruntfile.

Benefits:

• Less files in the project root (compared to 1)
• Logical entry points are together with configuration.

Drawbacks:

• This means the minimatch pattern will double as both a blacklist and whitelist. Creates risk of files being implicitly excluded, because only files first included and not later excluded are linted. The only way to avoid this is to start with **/*.js and then exclude, which is problematic for performance (requires full recursion into including node_modules, and then to exclude things with more recursion). This might be fixable upstream.
• No tooling integration with IDEs.
• No tooling integration with CLI for e.g. eslint --fix ..
• Workflow is tied to Gruntfile, which means project not using grunt, still need to pick one of the above.

# package.json
{
  "scripts": {
    "test": "grunt test"
}

# Gruntfile.js (optional)
grunt.initConfig( {
  eslint: {
    options: {
      baseConfig: {
        extends: preset,
        rules: overrides
      }
    },
    target: [
      '{src,test}/**/*.js',
      '*.js',
      '!excludes/'
    ]
} );

Thoughts?

I have nothing to add to this discussion except to say that @Krinkle's overview was entirely thorough.

In today's FSG I brought up this pending decision and stated that either eslintrc/eslintignore or package.json would be positive step forward.

I favoured eslintignore slightly because I believe it would be the least surprising to existing and new developers with experience outside WMF projects.

Having said that, it seems a number of newer projects and tools in the JS ecoystem strongly favour package.json. Some tools exclusively allow configuration through that method, which means this may lead to inconsistencies in the long term, where eslintrc and eslintignore files may appear old-fashioned. But, that's something we can revisit later.

The four attendees were unanimously positive (2/4) or neutral (2/4) on the option of eslintignore/eslintrc.

Volker proposed we leave it here for at least week, to make a final call on in the next meeting.

I'm generally okay with the .eslintignore approach (included in the meeting count above). Some drawbacks that should be mentioned:

• Most repos will also end up with .stylelintignore and .jsonlintignore files as well
• The ignore files only operate as blacklists, whereas Gruntfile can use a mixture of blacklisting and whitelisting. This may make some existing configs hard to convert, however ultimately this is probably a good thing as it is better to have new files linted by default. I've seen repos where newly added folders are not getting linted because they weren't whitelisted.

Hey. I object pretty strongly to the idea of using eslintignore files when using eslint via Grunt; I think this is just adding confusion and is a mistake. Comments on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralAuth/+/508916

@Jdforrester-WMF See T204176#4581960 for a previous overview that combines the different points that have been raised. Do you find points in there you disagree with and/or find inaccurate? Are there additional benefits and drawbacks we missed?

From the CentralAuth patch, you raised the point about multiple entry points. As I see it, the proposal to not specify ignores local to Gruntfile, we are reducing the number of entry points from 2 ways of calling ESLint to just one. The way we call it is eslint .. This can be done by IDEs, it can be done by developers when running commands like eslint --fix ., and is done by Grunt programmatically as eslint( … files: ['.'] …). The are all logically identical.

I further agree, that this means the grunt wrapper is effectively redundant. We already don't use Grunt as our entry point (our entry point is npm test). So I see it as mostly an internal detail hidden from day to day work whether npm test runs grunt eslint or eslint .. I see a change to that as outside the scope for this task, but would entirely agree with removing the Grunt wrapper as it indeed seems redundant. If you prefer to do those two changes together, let's do that. I don't think that would be controversial in any way.

In T204176#4818168, @Krinkle wrote:

Volker proposed we leave it here for at least week, to make a final call on in the next meeting.

Did a final call happen?

Obviously I'm very late (so please ignore if a decision was already made), but from a tooling perspective, I'm +1 to package.json, +0.5 to eslintignore, and -1 to Gruntfile.js. Having one file (package.json) hold the JS build config seems nice from a tooling perspective like LU. dotfiles are annoying for greps etc.. Also it seems like eslintignore is just .gitignore style and not JSON. I really like JSON :-)

In any case, once someone documents the decision, please file a task for LibUp to respect that.

Fine. I think this is a mistake (as eslint and stylelint can't share exclude lists), but so be it.

I propose that we close this with the outcome being that per-project developers can decide whether to use .eslintignore or package.json, based on local trade-offs.

I think the important thing is that we use a place to configure it that is standard to ESLint and thus discoverable to text editor plugins, and via command-line use of eslint such as eslint --fix for humans, or indeed via tools like LibUp. This also means new contributors can learn about them via upstream documentation of ESLint. It also helps in terms of consistency as it means simpler project that don't need a Gruntfile (but invoke eslint directly from npm test), will be configured in a similar way as other projects. And it reduces exposure to Gruntfile, which is yet another moving part to learn for new contributors (especially as Grunt becomes less popular).

Do we want to require all projects use the same method, or is it fine so long as it is something native to ESLint?

Is there a preferred default we can document in the coding conventions for new projects? I seem to have been in the minority in terms of package.json, and admit it's a minor difference. So, shall we go with .eslintignore as default?

The entire point of LibUp is to not make these decisions locally but to have a single standard template that runs the same everywhere.

I agree, but this doesn't affect LibUp. LibUp already worked well on the various Reading-Web projects that configure eslint ignores by other means.

We had to code support for Gruntfile in LibUp because its eslint CLI invocation was unable to find our excludes otherwise. When they are in either of the standard places, it all "just works". We can even remove this code from LibUp after that :)

In T204176#6003622, @Krinkle wrote:

I agree, but this doesn't affect LibUp. LibUp already worked well on the various Reading-Web projects that configure eslint ignores by other means.

We had to code support for Gruntfile in LibUp because its eslint CLI invocation was unable to find our excludes otherwise. When they are in either of the standard places, it all "just works". We can even remove this code from LibUp after that :)

Sure. If we pick a different standard, I'm happy to write the code for LibUp to do the migrations where possible. My complain is about your suggestion that "per-project developers can decide", which I think is an anti-pattern.

Avoiding a Gruntfile.js solution so that configuration can stay as data would be excellent.

In T204176#6003558, @Krinkle wrote:

Do we want to require all projects use the same method, or is it fine so long as it is something native to ESLint?

I agree with James that we should pick one. I would say:

1. we require that excludes are managed using a native eslint interface that doesn't require grunt
2. we strongly recommend that projects use .eslintignore
3. individual projects can use alternative methods (e.g. package.json) if they have a strong use case, but should expect that organizationwide tooling (e.g. libup) won't give them first-class support.

Is there a preferred default we can document in the coding conventions for new projects? I seem to have been in the minority in terms of package.json, and admit it's a minor difference. So, shall we go with .eslintignore as default?

I also prefer package.json just because I love JSON, but I'm fine with .eslintignore.

For completion, ESLint moves to a new config file format in JS .eslint.config.js doing away with .eslintrc and .eslintignore files.