use different eslint config for es6 and es5 #11794

ctxhou · 2017-12-07T13:18:25Z

This pr uses different eslint config to solve issues #11783.

The following is the updated part:

share es path: I move the es path pattern to a shared file scripts/shared/esPath.js, so that prettier and eslint can use same es path pattern
separate lint call: https://github.com/ctxhou/react/blob/c6b94daaabe89d7ac0b08a9c608ff734ced271d2/scripts/tasks/eslint.js#L13-L14
judge es version: For lint changed, because we need to define which changed files are es5 and others are es6, I create a file scripts/shared/judgeJSFilesVersion.js to get this info.

So now, when we run yarn lint or yarn linc, it will lint es5 and es6 separately.

gaearon · 2017-12-07T18:59:47Z

scripts/eslint/baseConfig.js

+
+module.exports = {
+  config: eslintrc,
+  OFF,


I think this is a bit confusing. Let's just copy and paste these constants, and completely remove this file :-) ESLint also actually accepts strings so we can later remove them altogether.

gaearon · 2017-12-07T19:00:57Z

scripts/eslint/es5Config.js

@@ -0,0 +1,3 @@
+const baseConfig = require('./baseConfig');
+
+module.exports = baseConfig.config;


I think there's more to it. I think we want to have ecmaVersion set to 6 for the ES6 config and to 5 for the ES5 config. It's not just about var, but about all the other features too.

ok, I add it on new commit.

gaearon · 2017-12-07T19:04:00Z

scripts/shared/judgeJSFilesVersion.js

+    const file = jsFiles[index];
+    for (let pathIndex in es5Path) {
+      const pattern = es5Path[pathIndex];
+      const isES5 = minimatch(file, pattern);


Is this efficient? What's the full yarn lint time before and after the change?

I also want to refactor it. But currently I don't think of other refactor way.
But it doesn't affect too much efficiency, because this script will only be used on yarn linc to distinguish es5 and es6 files: here.

For yarn lint, because the es5, and es6 pattern is known, we don't need to judge files' ecma version.

Here is the yarn lint run time after the change:

Before: 22.50s After: 17.86s.

The reason for runtime decrease is because, before the change, the filePattern was [.] (ref), after the change we use specific es path.

Maybe there’s something clever we can do to have ESLint do the filtering instead of us? I’m not sure but since Prettier can do this, maybe you can find a way with ESLint API too.

gaearon · 2017-12-08T08:21:10Z

scripts/prettier/index.js

-      'packages/*/src/**/*.js',
-      'packages/shared/**/*.js',
-    ],
+    patterns: es6Path,
    ignore: ['**/node_modules/**'],


Should ignore patterns also be a part of the respective configs? That would make sense to me. The fact that they’re currently duplicated in eslintignore is a bit confusing.

OK, I'll move it to respective config.
Does it mean we should put all the eslintignore setting to the shared config file?

I guess that could mess with the editor integrations people on the team might be using. So maybe we shouldn’t do it. What do you think?

I think we should keep the .eslintrc to make editor work as usual.
If we want to avoid ignore pattern duplicate, maybe we can write a script to read .eslintrc to use in prettier ignore part.

That sounds a bit convoluted.

How about:

We use top level eslintignore/prettierignore for things that should never be touched (like build products and third party code)

For ES5/ES6, we always negate the other pattern to get a list of extra ignores. For example ES5 = everything in ES5 patterns except anything that is ES6. And vice versa.

It’s even simpler.

ES6 doesn’t need a pattern. It should just be “everything”. The global ignores (in ESLint and Prettier top lebel ignore files) should filter out build artefacts and dependencies.

ES5 needs a pattern that just says “these files are ES5”. It’s a subset of all files.

You run ES5 passing it as “include” set.

Then you run ES6 by passing ES5 as “exclude” set. Because everything else is ES6 (or already excluded by global config).

In either case ES5 is the only pattern that needs to be explicitly specified.

Cool! It's more clear. I will correct it in this way.

BTW, for the lint change, eslint provide a ignorePattern params.
So when linting es5 files, we can pass es6 path as ignore pattern, vice versa.

However, it will cause this warning when the file is ignored: (related issue: eslint/eslint#5623)

0:0 warning File ignored because of a matching ignore pattern. Use "--no-ignore" to override

To turn off this warning is to set quiet as true, but it will quiet "all warning". I think it's inappropriate.
Therefore, I think write the filter to distinguish es5 files and es6 files is needed. I'll try a cleaner method to do it.

I think we can just filter this particular warning from the output (and maybe raise an issue to ask for an option to completely disable this warning).

gaearon · 2017-12-08T08:24:32Z

scripts/shared/judgeJSFilesVersion.js

+const minimatch = require('minimatch');
+const {es5Path} = require('./esPath');
+
+module.exports = function(jsFiles) {


If this is only used in linc task let’s just move it and inline it there. I wouldn’t worry too much about having a test for this file.

gaearon · 2017-12-08T08:25:52Z

scripts/shared/judgeJSFilesVersion.js

+    const file = jsFiles[index];
+    for (let pathIndex in es5Path) {
+      const pattern = es5Path[pathIndex];
+      const isES5 = minimatch(file, pattern);


Maybe there’s something clever we can do to have ESLint do the filtering instead of us? I’m not sure but since Prettier can do this, maybe you can find a way with ESLint API too.

ctxhou · 2017-12-08T12:38:13Z

@gaearon, this new commit I use eslint params ignorePattern to filter out another es version's file.

In the earlier discussion, we talk about use .prettierignore file to list ignore files. But currently, prettier's node api doesn't suppport read .prettierignore, it only support pass source content.
It means that we still need to keep current method passing the needed content to prettier. So that's why I didn't add .prettierignore file.

gaearon · 2017-12-08T14:16:26Z

scripts/eslint/index.js

@@ -8,12 +8,30 @@
 'use strict';

 const CLIEngine = require('eslint').CLIEngine;
-const cli = new CLIEngine();
-const formatter = cli.getFormatter();
+const eslintPath = './scripts/eslint';


This is called __dirname in Node :-)

gaearon · 2017-12-08T14:16:40Z

scripts/eslint/es6Config.js

+var ERROR = 2;
+
+module.exports = Object.assign({}, eslintrc, {
+  parseOptions: {


parserOptions, not parseOptions.

gaearon · 2017-12-08T14:17:49Z

scripts/eslint/es6Config.js

@@ -0,0 +1,16 @@
+const eslintrc = require('../../.eslintrc');


I don't see how this could possibly work. By the pattern, this file should be treated as ES5. But then const should not parse. However it lints just fine.

This brings up a separate point: we're actually okay with treating our Node /scripts/ as ES6. It's just that we don't want trailing commas in them. So maybe we need three different file types (ESnext+JSX, ES6, and ES5).

So the main different lint setting of ES6 and ESnext is we turn off comma-dangle rule at es6?
Currently, I can't find other different of ESnext and ES6 config.

JSX, Flow should also be disabled in ES6. And it should use the espree parser.

OK, so there are some files in scripts should be ignored.

Like:
/scripts/flow/environment.js
/scripts/rollup/shims/react-native/NativeMethodsMixin.js

All shims should be treated as source (ESNext). Good catch.

gaearon · 2017-12-08T14:43:58Z

My impression is that ESLint CLIEngine ignores the explicit parserOptions configuration in favor of what the base config (fbjs) says. I don't know why this happens. It might be a bug.

gaearon · 2017-12-08T15:06:44Z

Seems like these are the options we need to use to force ESLint to treat file as ES5:

    parser: 'espree',
    parserOptions: {
      ecmaVersion: 5,
      sourceType: 'script'
    },

This won't work for our Node scripts, so as I mentioned above, we need to divide files into three different sets:

ESNext + JSX (source)
ES6-only files (scripts)
ES5 files (npm entry points)

And then supply different options above depending on the types.

ctxhou · 2017-12-08T19:39:33Z

@gaearon, I've pushed new commits to solve it.
Really sorry for some wrong commits because I didn't notice the prettier config.

As what we discussed, I separate the es path to ESNext, ES6, ES5, and those have independent eslint setting.
For prettier, default is ESNextPath, and scripts is ES5Path and ES6Path.

const config = {
  default: {
    patterns: esNextPath,
    ignore: ['**/node_modules/**'],
  },
  scripts: {
    patterns: [...es5Path, ...es6Path],
    ignore: [
      '**/node_modules/**',
      // Built files and React repo clone
      'scripts/bench/benchmarks/**',
      // shims & flow treat as ESNext
      'scripts/flow/*.js',
      'scripts/rollup/shims/**/*.js',
    ],
    options: {
      trailingComma: 'es5',
    },
  },
};

gaearon · 2017-12-08T19:53:45Z

scripts/bench/server.js

@@ -21,7 +21,7 @@ function createHTTP2Server(benchmark) {
      __dirname,
      'benchmarks',
      benchmark,
-      request.url
+      request.url,


This won't work. Everything inside scripts/bench (and scripts in general, except scripts/rollup/shims) should be normal ES6 without trailing commas in function calls. Otherwise it won't run on Node.

gaearon · 2017-12-08T19:55:06Z

scripts/bench/benchmarks/hacker-news/generate.js

 const stories = 50;

 async function getStory(id) {
-  const storyRes = await fetch(`https://hacker-news.firebaseio.com/v0/item/${id}.json`);
+  const storyRes = await fetch(
+    `https://hacker-news.firebaseio.com/v0/item/${id}.json`,


I don't see how this could work. It should be ES6 (which doesn't support trailing commas).

It's my fault. On previous commit, I was uncareful to ignore benchmark, so it ran prettier on those files.
I've reverted it on latest commit.

gaearon · 2017-12-09T10:01:32Z

scripts/eslint/es6Config.js

+    sourceType: 'module',
+    ecmaFeatures: {
+      jsx: true,
+      experimentalObjectRestSpread: true,


Why do we need these features for ES6? I’d expect both to be false.

Cause this files scripts/release/build-commands/parse-build-parameters.js use this syntax.
So we don't allow this syntax?

Why does it work? Did it get enabled by default in some Node version?

In this case let’s leave it on. But JSX shouldn’t be.

I think that's because our devEngines is required node v8.x, so this syntax works.

gaearon

This is going in the right direction. Let's make a few changes.

Rename ESLint configs to eslintrc.source.js (ESNext), eslintrc.node.js (ES6) and eslintrc.npm.js (ES5).
The eslintrc.node.js config should not enable JSX ~~or object spread~~. It also should have 'script' rather than 'module' type. Additionally, it should include a lint rule that requires 'use strict' to be specified in every file.
The eslintrc.npm.js config should disallow JSX.
fixtures/**/*.js should be ES6 (aka "node" in new naming), rather than ES5.
After these changes are done, please revert any changes to files that were automatic. Then rebase this PR on top of master. Then reapply the automatic changes. Otherwise this isn't up to date.

…s5, es6

ctxhou · 2017-12-11T08:32:33Z

@gaearon, I've updated the config naming, revert automatic changed files and rebase to master 😄

gaearon · 2017-12-11T11:23:16Z

yarn.lock

@@ -3358,7 +3358,7 @@ minimatch@^2.0.3:

 minimatch@^3.0.4:
  version "3.0.4"
-  resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-3.0.4.tgz#5166e286457f03306064be5497e8dbb0c3d32083"
+  resolved "https://registry.npmjs.org/minimatch/-/minimatch-3.0.4.tgz#5166e286457f03306064be5497e8dbb0c3d32083"


Unintentional?

yarn seems to have a bug where it occasionally alternates between registries on some packages; not sure what causes it, but I've seen it once in a while

I just mean we can safely omit this change from a PR.

ok, I'll revert it.

gaearon · 2017-12-11T11:25:01Z

scripts/prettier/index.js

    ignore: [
      '**/node_modules/**',
      // Built files and React repo clone
      'scripts/bench/benchmarks/**',
+      // shims & flow treat as ESNext
+      'scripts/flow/*.js',
+      'scripts/rollup/shims/**/*.js',


It feels confusing that we still have to explicitly ignore some paths here but ESLint is fully covered by just “patterns” config.

gaearon · 2017-12-11T11:26:47Z

scripts/eslint/index.js

@@ -8,12 +8,54 @@
 'use strict';

 const CLIEngine = require('eslint').CLIEngine;
-const cli = new CLIEngine();
-const formatter = cli.getFormatter();
+const {npmPath, nodePath, sourcePath} = require('../shared/esPath');


Let’s rename that file to filesByLanguageVersion?

ctxhou · 2017-12-11T14:08:52Z

scripts/prettier/index.js

-      'scripts/flow/*.js',
-      'scripts/rollup/shims/**/*.js',
-    ],
+    ignore: ['**/node_modules/**'],


I think remove the scripts in ignore will cause other issues. Some patterns are covered (like 'scripts/flow/*.js') will run on both prettier process.

Because we run prettier twice (default -> scripts), the results seem work as our expect, but actually some changes on default prettier is covered by scripts prettier.

Maybe we can expand the scripts pattern to make it more clear?

const nodePath = [ 'scripts/babel/*.js', 'scripts/circleci/*.js', 'scripts/error-codes/*.js', ... ];

I see. Let me look at that.

I think I can take this PR from here, thanks a lot!

You're welcome. Thanks for your patience.

jquense · 2017-12-11T14:16:07Z

sorry to jump in late and snipe the PR! Eslint now supports overriding and configuration by file glob in the eslintrc. (example here) that may help simplify this a bit and reduce the extra code needed for the different paths

eslint docs: https://eslint.org/docs/user-guide/configuring#configuration-based-on-glob-patterns

gaearon · 2017-12-11T14:17:10Z

Oh nice. I think we'll get a version of this in first, but then file another issue to refactor it :-)

gaearon · 2017-12-11T15:53:09Z

This is great. Thank you!

facebook-github-bot added the CLA Signed label Dec 7, 2017

ctxhou mentioned this pull request Dec 7, 2017

RFC: Remove vars follow up #11783

Closed

gaearon reviewed Dec 7, 2017

View reviewed changes

gaearon reviewed Dec 8, 2017

View reviewed changes

gaearon requested changes Dec 8, 2017

View reviewed changes

gaearon reviewed Dec 8, 2017

View reviewed changes

gaearon reviewed Dec 9, 2017

View reviewed changes

gaearon requested changes Dec 9, 2017

View reviewed changes

ctxhou added 5 commits December 11, 2017 16:08

use different eslint config for es6 and es5

864711a

remove confusing eslint/baseConfig.js & add more eslint setting for e…

589fb39

…s5, es6

more clear way to run eslint on es5 & es6 file

241dc69

seperate ESNext, ES6, ES6 path, and use different lint config

730a586

rename eslint config file & update eslint rules

adb36b7

ctxhou force-pushed the eslint-different-version branch from efa53c8 to adb36b7 Compare December 11, 2017 08:19

gaearon reviewed Dec 11, 2017

View reviewed changes

gaearon added 3 commits December 11, 2017 13:43

Undo yarn.lock changes

7bacfc3

Rename a file

e4e4153

Remove unnecessary exceptions

1e1c5fb

ctxhou commented Dec 11, 2017

View reviewed changes

gaearon added 2 commits December 11, 2017 14:21

Refactor a little bit

f87f278

Refactor and tweak the logic

680398c

Minor issues

a7f3021

gaearon approved these changes Dec 11, 2017

View reviewed changes

gaearon merged commit e8e62eb into facebook:master Dec 11, 2017

NMinhNguyen mentioned this pull request Oct 21, 2018

Refactor ESLint configuration to enable better IDE integration #13914

Merged

This was referenced Dec 18, 2020

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 cajonemsft/react#48

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 heshamelmasry77/react#38

Open

This was referenced Sep 16, 2021

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 heshamelmasry77/react#48

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 cajonemsft/react#78

Open

		@@ -0,0 +1,3 @@
		const baseConfig = require('./baseConfig');

		module.exports = baseConfig.config;

		@@ -0,0 +1,16 @@
		const eslintrc = require('../../.eslintrc');

use different eslint config for es6 and es5 #11794

use different eslint config for es6 and es5 #11794

Conversation

ctxhou commented Dec 7, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctxhou commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

ctxhou Dec 8, 2017 • edited Loading

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

gaearon commented Dec 8, 2017

gaearon commented Dec 8, 2017

ctxhou commented Dec 8, 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

ctxhou Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon left a comment • edited Loading

Choose a reason for hiding this comment

ctxhou commented Dec 11, 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

jquense commented Dec 11, 2017 • edited Loading

gaearon commented Dec 11, 2017

gaearon commented Dec 11, 2017 • edited Loading

ctxhou commented Dec 7, 2017 •

edited

Loading

gaearon Dec 8, 2017 •

edited

Loading

ctxhou Dec 8, 2017 •

edited

Loading

ctxhou Dec 10, 2017 •

edited

Loading

gaearon left a comment •

edited

Loading

jquense commented Dec 11, 2017 •

edited

Loading

gaearon commented Dec 11, 2017 •

edited

Loading