Stop using private ESLint APIs for ESLint config merging (#1464)

In #1182, the previous broken ESLint config merging functionality was replaced by using an internal ESLint `merge` function from `config-ops`. Unfortunately in ESLint 6 that function no longer exists in a form that is easy for us to use, so in order to support ESLint 6, I have replaced it with simpler manual merges. These are virtually identical, with one difference - `rules` are now merged by a later rule entry completely replacing the first, rather than updating it. For example, merging these: `'foo': ['error', {'someSetting': false}]` `'foo': 'warn'` ...used to give: `'foo': ['warn', {'someSetting': false}]` ...but now results in: `'foo': 'warn'` I think this difference is a reasonable compromise for not having to rely on ESLint internals and/or implement our own more complex merging. I also believe hitting this will be rare, since: * it doesn't affect merging with rules defined inside an `extends` external file * it won't make a difference for rules that were already at default settings (or that were overridden with custom settings in the later rule definition being merged in) This more simplistic merging behaviour is also what's used by Neutrino 8 (#1182 landed for Neutrino 9 only) - and we still have the other bug fixes included in #1182 that were the primary motivation for refactoring in the first place. Refs #1423.
neutrinojs · Sep 9, 2019 · 79d4eda · 79d4eda
1 parent 5672c1b
commit 79d4eda
Show file tree

Hide file tree

Showing 11 changed files with 147 additions and 175 deletions.
diff --git a/packages/airbnb-base/index.js b/packages/airbnb-base/index.js
@@ -1,5 +1,4 @@
 const lint = require('@neutrinojs/eslint');
-const { merge: eslintMerge } = require('eslint/lib/config/config-ops');
 const {
   rules: airbnbBaseStyle,
 } = require('eslint-config-airbnb-base/rules/style');
@@ -8,35 +7,37 @@ const {
 } = require('eslint-config-airbnb-base/rules/best-practices');
 
 module.exports = ({ eslint = {}, ...opts } = {}) => neutrino => {
+  const baseConfig = eslint.baseConfig || {};
   neutrino.use(
     lint({
       ...opts,
       eslint: {
         ...eslint,
-        baseConfig: eslintMerge(
-          {
-            extends: [require.resolve('eslint-config-airbnb-base')],
-            rules: {
-              // Disable rules for which there are eslint-plugin-babel replacements:
-              // https://github.com/babel/eslint-plugin-babel#rules
-              'new-cap': 'off',
-              'no-invalid-this': 'off',
-              'object-curly-spacing': 'off',
-              semi: 'off',
-              'no-unused-expressions': 'off',
-              // Ensure the replacement rules use the options set by airbnb-base rather than ESLint defaults.
-              'babel/new-cap': airbnbBaseStyle['new-cap'],
-              'babel/no-invalid-this':
-                airbnbBaseBestPractices['no-invalid-this'],
-              'babel/object-curly-spacing':
-                airbnbBaseStyle['object-curly-spacing'],
-              'babel/semi': airbnbBaseStyle.semi,
-              'babel/no-unused-expressions':
-                airbnbBaseBestPractices['no-unused-expressions'],
-            },
+        baseConfig: {
+          ...baseConfig,
+          extends: [
+            require.resolve('eslint-config-airbnb-base'),
+            ...(baseConfig.extends || []),
+          ],
+          rules: {
+            // Disable rules for which there are eslint-plugin-babel replacements:
+            // https://github.com/babel/eslint-plugin-babel#rules
+            'new-cap': 'off',
+            'no-invalid-this': 'off',
+            'object-curly-spacing': 'off',
+            semi: 'off',
+            'no-unused-expressions': 'off',
+            // Ensure the replacement rules use the options set by airbnb-base rather than ESLint defaults.
+            'babel/new-cap': airbnbBaseStyle['new-cap'],
+            'babel/no-invalid-this': airbnbBaseBestPractices['no-invalid-this'],
+            'babel/object-curly-spacing':
+              airbnbBaseStyle['object-curly-spacing'],
+            'babel/semi': airbnbBaseStyle.semi,
+            'babel/no-unused-expressions':
+              airbnbBaseBestPractices['no-unused-expressions'],
+            ...baseConfig.rules,
           },
-          eslint.baseConfig || {},
-        ),
+        },
       },
     }),
   );

diff --git a/packages/airbnb-base/test/airbnb_test.js b/packages/airbnb-base/test/airbnb_test.js
@@ -77,7 +77,6 @@ test('sets defaults when no options passed', t => {
       globals: {
         process: true,
       },
-      overrides: [],
       parser: require.resolve('babel-eslint'),
       parserOptions: {
         ecmaVersion: 2018,
@@ -116,7 +115,6 @@ test('sets defaults when no options passed', t => {
         'object-curly-spacing': 'off',
         semi: 'off',
       },
-      settings: {},
     },
     cache: true,
     cwd: api.options.root,
@@ -172,7 +170,6 @@ test('merges options with defaults', t => {
         jQuery: true,
         process: true,
       },
-      overrides: [],
       parser: require.resolve('babel-eslint'),
       parserOptions: {
         ecmaVersion: 2018,
@@ -195,14 +192,7 @@ test('merges options with defaults', t => {
           },
         ],
         'babel/no-invalid-this': 'off',
-        'babel/no-unused-expressions': [
-          'warn',
-          {
-            allowShortCircuit: false,
-            allowTaggedTemplates: false,
-            allowTernary: false,
-          },
-        ],
+        'babel/no-unused-expressions': 'warn',
         'babel/object-curly-spacing': ['error', 'always'],
         'babel/semi': ['error', 'always'],
         'new-cap': 'off',

diff --git a/packages/airbnb/index.js b/packages/airbnb/index.js
@@ -1,5 +1,4 @@
 const lint = require('@neutrinojs/eslint');
-const { merge: eslintMerge } = require('eslint/lib/config/config-ops');
 const {
   rules: airbnbBaseStyle,
 } = require('eslint-config-airbnb-base/rules/style');
@@ -8,41 +7,41 @@ const {
 } = require('eslint-config-airbnb-base/rules/best-practices');
 
 module.exports = ({ eslint = {}, ...opts } = {}) => neutrino => {
+  const baseConfig = eslint.baseConfig || {};
   neutrino.use(
     lint({
       ...opts,
       eslint: {
         ...eslint,
-        baseConfig: eslintMerge(
-          {
-            extends: [
-              require.resolve('eslint-config-airbnb'),
-              require.resolve('eslint-config-airbnb/hooks'),
-            ],
-            rules: {
-              // Override AirBnB's configuration of 'always', since they only set that value due to
-              // babel-preset-airbnb not supporting class properties, whereas @neutrinojs/react does.
-              'react/state-in-constructor': ['error', 'never'],
-              // Disable rules for which there are eslint-plugin-babel replacements:
-              // https://github.com/babel/eslint-plugin-babel#rules
-              'new-cap': 'off',
-              'no-invalid-this': 'off',
-              'object-curly-spacing': 'off',
-              semi: 'off',
-              'no-unused-expressions': 'off',
-              // Ensure the replacement rules use the options set by airbnb rather than ESLint defaults.
-              'babel/new-cap': airbnbBaseStyle['new-cap'],
-              'babel/no-invalid-this':
-                airbnbBaseBestPractices['no-invalid-this'],
-              'babel/object-curly-spacing':
-                airbnbBaseStyle['object-curly-spacing'],
-              'babel/semi': airbnbBaseStyle.semi,
-              'babel/no-unused-expressions':
-                airbnbBaseBestPractices['no-unused-expressions'],
-            },
+        baseConfig: {
+          ...baseConfig,
+          extends: [
+            require.resolve('eslint-config-airbnb'),
+            require.resolve('eslint-config-airbnb/hooks'),
+            ...(baseConfig.extends || []),
+          ],
+          rules: {
+            // Override AirBnB's configuration of 'always', since they only set that value due to
+            // babel-preset-airbnb not supporting class properties, whereas @neutrinojs/react does.
+            'react/state-in-constructor': ['error', 'never'],
+            // Disable rules for which there are eslint-plugin-babel replacements:
+            // https://github.com/babel/eslint-plugin-babel#rules
+            'new-cap': 'off',
+            'no-invalid-this': 'off',
+            'object-curly-spacing': 'off',
+            semi: 'off',
+            'no-unused-expressions': 'off',
+            // Ensure the replacement rules use the options set by airbnb rather than ESLint defaults.
+            'babel/new-cap': airbnbBaseStyle['new-cap'],
+            'babel/no-invalid-this': airbnbBaseBestPractices['no-invalid-this'],
+            'babel/object-curly-spacing':
+              airbnbBaseStyle['object-curly-spacing'],
+            'babel/semi': airbnbBaseStyle.semi,
+            'babel/no-unused-expressions':
+              airbnbBaseBestPractices['no-unused-expressions'],
+            ...baseConfig.rules,
           },
-          eslint.baseConfig || {},
-        ),
+        },
       },
     }),
   );

diff --git a/packages/airbnb/test/airbnb_test.js b/packages/airbnb/test/airbnb_test.js
@@ -80,7 +80,6 @@ test('sets defaults when no options passed', t => {
       globals: {
         process: true,
       },
-      overrides: [],
       parser: require.resolve('babel-eslint'),
       parserOptions: {
         ecmaVersion: 2018,
@@ -120,7 +119,6 @@ test('sets defaults when no options passed', t => {
         'object-curly-spacing': 'off',
         semi: 'off',
       },
-      settings: {},
     },
     cache: true,
     cwd: api.options.root,
@@ -177,7 +175,6 @@ test('merges options with defaults', t => {
         jQuery: true,
         process: true,
       },
-      overrides: [],
       parser: require.resolve('babel-eslint'),
       parserOptions: {
         ecmaVersion: 2018,
@@ -201,14 +198,7 @@ test('merges options with defaults', t => {
           },
         ],
         'babel/no-invalid-this': 'off',
-        'babel/no-unused-expressions': [
-          'warn',
-          {
-            allowShortCircuit: false,
-            allowTaggedTemplates: false,
-            allowTernary: false,
-          },
-        ],
+        'babel/no-unused-expressions': 'warn',
         'babel/object-curly-spacing': ['error', 'always'],
         'babel/semi': ['error', 'always'],
         'new-cap': 'off',

diff --git a/packages/eslint/index.js b/packages/eslint/index.js
@@ -1,5 +1,3 @@
-const pick = require('lodash.pick');
-const { merge: eslintMerge } = require('eslint/lib/config/config-ops');
 const { ConfigurationError, DuplicateRuleError } = require('neutrino/errors');
 
 const arrayToObject = array =>
@@ -21,23 +19,38 @@ const eslintrc = neutrino => {
     );
   }
 
-  return eslintMerge(
+  const baseConfig = options.baseConfig || {};
+
+  return {
     // ESLint's CLIEngine (and thus eslint-loader) supports being passed a `baseConfig`
     // object which is roughly equivalent to the contents of an eslintrc config.
     // However `baseConfig` isn't supported in eslintrc so must be flattened.
-    options.baseConfig,
+    ...baseConfig,
     // In addition, the following top-level CLIEngine's options are equivalent to
     // options found inside `baseConfig`, and take precedence when using eslint-loader,
     // so we have to emulate that here by merging them in after `baseConfig`.
-    // All other options must not be ignored, since they are eslint-loader/CLIEngine specific.
-    {
-      ...pick(options, ['parser', 'parserOptions', 'plugins', 'rules']),
-      // These top level options are of type array, however their baseConfig/eslintrc
-      // equivalents must be objects (!?), so we have to convert first.
-      ...(options.envs && { env: arrayToObject(options.envs) }),
-      ...(options.globals && { globals: arrayToObject(options.globals) }),
+    // All other options must be ignored, since they are eslint-loader/CLIEngine specific.
+    // Note: The top level `envs` and `globals` options are of type array, however their
+    // baseConfig/eslintrc equivalents must be objects (!?), so we have to convert first.
+    env: {
+      ...baseConfig.env,
+      ...(options.envs && arrayToObject(options.envs)),
     },
-  );
+    globals: {
+      ...baseConfig.globals,
+      ...(options.globals && arrayToObject(options.globals)),
+    },
+    parser: options.parser || baseConfig.parser,
+    parserOptions: {
+      ...baseConfig.parserOptions,
+      ...options.parserOptions,
+    },
+    plugins: (baseConfig.plugins || []).concat(options.plugins || []),
+    rules: {
+      ...baseConfig.rules,
+      ...options.rules,
+    },
+  };
 };
 
 const validLoaderOptions = [
@@ -111,6 +124,8 @@ module.exports = ({ test, include, exclude, eslint = {} } = {}) => {
       throw new DuplicateRuleError('@neutrinojs/eslint', 'lint');
     }
 
+    const baseConfig = eslint.baseConfig || {};
+
     const loaderOptions = {
       // For supported options, see:
       // https://github.com/webpack-contrib/eslint-loader#options
@@ -137,30 +152,28 @@ module.exports = ({ test, include, exclude, eslint = {} } = {}) => {
       // the lint config will be inconsistent between standalone lint and eslint-loader.
       baseConfig: eslint.useEslintrc
         ? {}
-        : eslintMerge(
-            {
-              env: {
-                es6: true,
-              },
-              extends: [],
-              globals: {
-                process: true,
-              },
-              overrides: [],
-              parser: require.resolve('babel-eslint'),
-              parserOptions: {
-                ecmaVersion: 2018,
-                sourceType: 'module',
-              },
-              // Unfortunately we can't `require.resolve('eslint-plugin-babel')` due to:
-              // https://github.com/eslint/eslint/issues/6237
-              // ...so we have no choice but to rely on it being hoisted.
-              plugins: ['babel'],
-              root: true,
-              settings: {},
+        : {
+            parser: require.resolve('babel-eslint'),
+            root: true,
+            ...baseConfig,
+            env: {
+              es6: true,
+              ...baseConfig.env,
+            },
+            globals: {
+              process: true,
+              ...baseConfig.globals,
+            },
+            parserOptions: {
+              ecmaVersion: 2018,
+              sourceType: 'module',
+              ...baseConfig.parserOptions,
             },
-            eslint.baseConfig || {},
-          ),
+            // Unfortunately we can't `require.resolve('eslint-plugin-babel')` due to:
+            // https://github.com/eslint/eslint/issues/6237
+            // ...so we have no choice but to rely on it being hoisted.
+            plugins: ['babel', ...(baseConfig.plugins || [])],
+          },
     };
 
     neutrino.config.module

diff --git a/packages/eslint/package.json b/packages/eslint/package.json
@@ -25,8 +25,7 @@
   "dependencies": {
     "babel-eslint": "^10.0.2",
     "eslint-loader": "^3.0.0",
-    "eslint-plugin-babel": "^5.3.0",
-    "lodash.pick": "^4.4.0"
+    "eslint-plugin-babel": "^5.3.0"
   },
   "peerDependencies": {
     "eslint": "^5.0.0",

diff --git a/packages/eslint/test/middleware_test.js b/packages/eslint/test/middleware_test.js
@@ -134,19 +134,16 @@ test('sets defaults when no options passed', t => {
       env: {
         es6: true,
       },
-      extends: [],
       globals: {
         process: true,
       },
-      overrides: [],
       parser: require.resolve('babel-eslint'),
       parserOptions: {
         ecmaVersion: 2018,
         sourceType: 'module',
       },
       plugins: ['babel'],
       root: true,
-      settings: {},
     },
     cache: true,
     cwd: api.options.root,
@@ -161,19 +158,17 @@ test('sets defaults when no options passed', t => {
     env: {
       es6: true,
     },
-    extends: [],
     globals: {
       process: true,
     },
-    overrides: [],
     parser: require.resolve('babel-eslint'),
     parserOptions: {
       ecmaVersion: 2018,
       sourceType: 'module',
     },
     plugins: ['babel'],
     root: true,
-    settings: {},
+    rules: {},
   });
 });
 
@@ -319,7 +314,7 @@ test('merges options with defaults', t => {
     plugins: ['babel', 'react', 'jest'],
     root: true,
     rules: {
-      quotes: ['warn', 'single'],
+      quotes: 'warn',
     },
     settings: {
       react: {