Use `overrides` for a single `.eslintrc.js`. #7443

rwjblue · 2017-11-15T00:03:14Z

Update the .eslintrc.js used by apps and addons, to avoid needing separate "sprinkles" of eslint configuration throughout the projects codebase.

Compatible with eslint@4.1.0.

Additional reading:

rwjblue · 2017-11-15T00:04:12Z

I will follow up and make sure that ember-cli-eslint's blueprint is in sync once we settle on this...

kellyselden · 2017-11-15T00:23:31Z

blueprints/app/files/.eslintrc.js

@@ -2,12 +2,39 @@ module.exports = {
  root: true,
  parserOptions: {
    ecmaVersion: 2017,
-    sourceType: 'module'
+    sourceType: 'module',


seems strange to only have trailing commas in this file. I vote for removal.

😭 but ok 😄

I agree, we should add them to all files...

Ya, agreed. Though I’m happy to do this separately.

Turbo87 · 2017-11-15T11:27:37Z

blueprints/app/files/.eslintrc.js

+      files: [
+        './ember-cli-build.js',
+        './testem.js',
+        './index.js',


should we use ./*.js instead?

Interesting. I hadn’t thought of that. Should work nicely.

Sadly, this doesn't work. The main issue is that in the implementation (eslint/eslint#8081) when the files and excludedFiles are passed in to minimatch (here) they also pass { matchBase: true } as options.

From the minimatch README about matchBase:

If set, then patterns without slashes will be matched against the basename of the path if it contains slashes. For example, a?b would match the path /xyz/123/acb, but not /xyz/acb/123.

tl;dr Due to the usage of matchBase it is not possible to use globs to match files in the project root. If we use *.js it matches all files in the repo, if we use ./*.js it matches no files, if we use /*.js it throws an error....

😭 😭 😭

Turbo87 · 2017-11-15T11:28:31Z

blueprints/app/files/.eslintrc.js

+        ecmaVersion: 2015,
+      },
+      env: {
+        browser: false,


I think this has to be browser: null or something like that. iirc true/false only defines whether the global can be overwritten or something like that

Really!?!?! Weird! I’ll look it up. I have used this on a few other projects, will test and update as needed.

I just realized I might be confusing this with the stuff in the globals object, but we should verify just in case.

Ya, I was looking through the docs on configuring and I think you are thinking of the globals stuff. For easy cross referencing:

https://eslint.org/docs/user-guide/configuring#specifying-environments <- Docs on these env flags (no discussion of clobbering)

https://eslint.org/docs/user-guide/configuring#specifying-globals <- Docs on globals (includes discussion on true / false flag to allow clobbering)

Turbo87 · 2017-11-15T11:29:07Z

blueprints/app/files/.eslintrc.js

+    // test files
+    {
+      files: ['tests/**/*.js'],
+        env: {


indentation

Turbo87 · 2017-11-15T11:29:58Z

blueprints/app/files/.eslintrc.js

+        './ember-cli-build.js',
+        './testem.js',
+        './index.js',
+        'config/**/*.js',


what about tests/dummy/config/**/*.js?

👍 - good idea

Turbo87 · 2017-11-15T11:30:17Z

blueprints/app/files/.eslintrc.js

+
+    // test files
+    {
+      files: ['tests/**/*.js'],


can we exclude tests/dummy/?

it's okay if we can't since we haven't done it before either, but would be nice though

Yep, added and confirmed. Works nicely!

kellyselden · 2017-11-15T17:57:54Z

I would like to hold off merging until our meeting. I'm not 100% in favor of this. I like getting rid of the first line sprinkles of node files, but I'm not convinced that removing our tests/.eslintrc.js is a good idea. Everybody's documentation and blueprint injection touch that file (globals and test helpers), and it seems to me that it is in it's correct place.

rwjblue · 2017-11-15T18:45:27Z

I would like to hold off merging until our meeting.

Seems good, happy to discuss...

I like getting rid of the first line sprinkles of node files, but I'm not convinced that removing our tests/.eslintrc.js is a good idea.

This is where we disagree. The current system of having many .eslintrc files all over makes it very difficult to understand what the current ruleset is at any given point (a number of times I've resorted to using eslint --print-config some/file/path.js just to figure out what the heck is running). Another issue is that (other than simple things like env) its not possible by adding a .eslintrc file to handle directories that contain mixed rulesets .

Everybody's documentation and blueprint injection touch that file (globals and test helpers), and it seems to me that it is in it's correct place.

I think this is actually a separate concern from the others. We should choose the right set of defaults, and recommend them while also providing time and assistance to addons that may need to make modifications. Modifying a single file (.eslintrc.js) in general, is easier than having to modify many potential locations...

kellyselden · 2017-11-16T22:22:25Z

We decided in the core meeting that this is the way forward. With module unification, test files will be strewn about a project, so wildcard paths inside a single eslint config is the future (TM).

rwjblue · 2017-11-18T17:22:54Z

Rebased and addressed all outstanding comments...

Update the `.eslintrc.js` used by apps and addons, to avoid needing separate "sprinkles" of eslint configuration throughout the projects codebase. Compatible with `eslint@4.1.0`.

Turbo87 · 2017-11-20T12:25:48Z

@rwjblue this shouldn't block, but can you verify the behavior for in-repo-addons?

rwjblue · 2017-11-20T13:40:48Z

@Turbo87 - Good point, I will do some testing. I believe that we might need to add lib/*/index.js to the "node" config...

rwjblue requested a review from Turbo87 November 15, 2017 00:04

kellyselden reviewed Nov 15, 2017

View reviewed changes

Turbo87 reviewed Nov 15, 2017

View reviewed changes

Turbo87 added the enhancement label Nov 15, 2017

Turbo87 added the Needs Team Discussion label Nov 15, 2017

rwjblue mentioned this pull request Nov 16, 2017

Ember-CLI Meeting Notes 16 November 2017 emberjs/core-notes#219

Closed

kellyselden removed the Needs Team Discussion label Nov 17, 2017

rwjblue force-pushed the better-eslint-config branch from 29afbef to a367e98 Compare November 18, 2017 17:22

Use overrides for a single .eslintrc.js.

6b5c068

Update the `.eslintrc.js` used by apps and addons, to avoid needing separate "sprinkles" of eslint configuration throughout the projects codebase. Compatible with `eslint@4.1.0`.

rwjblue force-pushed the better-eslint-config branch from a367e98 to 6b5c068 Compare November 18, 2017 17:51

Turbo87 approved these changes Nov 20, 2017

View reviewed changes

rwjblue merged commit 4dfbd37 into ember-cli:master Nov 20, 2017

Turbo87 mentioned this pull request Nov 20, 2017

Use ESLint overrides instead of nested config mainmatter/eslint-config-simplabs#54

Open

rwjblue deleted the better-eslint-config branch November 20, 2017 13:40

rwjblue mentioned this pull request Nov 20, 2017

Add prettier and eslint-plugin-ember to linting config. #7450

Merged

Turbo87 mentioned this pull request Dec 19, 2017

Ember 2.18 Release Blog Post emberjs/website#3092

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `overrides` for a single `.eslintrc.js`. #7443

Use `overrides` for a single `.eslintrc.js`. #7443

rwjblue commented Nov 15, 2017

rwjblue commented Nov 15, 2017

kellyselden Nov 15, 2017

rwjblue Nov 15, 2017

Turbo87 Nov 15, 2017

rwjblue Nov 15, 2017

Turbo87 Nov 15, 2017

rwjblue Nov 15, 2017

rwjblue Nov 18, 2017

Turbo87 Nov 15, 2017

rwjblue Nov 15, 2017

Turbo87 Nov 15, 2017

rwjblue Nov 15, 2017

Turbo87 Nov 15, 2017

Turbo87 Nov 15, 2017

rwjblue Nov 15, 2017

Turbo87 Nov 15, 2017

rwjblue Nov 18, 2017

kellyselden commented Nov 15, 2017

rwjblue commented Nov 15, 2017

kellyselden commented Nov 16, 2017

rwjblue commented Nov 18, 2017

Turbo87 commented Nov 20, 2017

rwjblue commented Nov 20, 2017

Use overrides for a single .eslintrc.js. #7443

Use overrides for a single .eslintrc.js. #7443

Conversation

rwjblue commented Nov 15, 2017

rwjblue commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyselden commented Nov 15, 2017

rwjblue commented Nov 15, 2017

kellyselden commented Nov 16, 2017

rwjblue commented Nov 18, 2017

Turbo87 commented Nov 20, 2017

rwjblue commented Nov 20, 2017

Use `overrides` for a single `.eslintrc.js`. #7443

Use `overrides` for a single `.eslintrc.js`. #7443