feature #687 Remove ESLint user-related config (Kocal, weaverryan)

This PR was merged into the master branch. Discussion ---------- Remove ESLint user-related config Hi ✋ This PR is a proposal for #657 implementation and should fix issues encountered in #473, #574, #656, #659, and #685. As discussed in #657, it would be better if Encore didn't configure ESLint itself. It should be done by the end user in an configuration file (e.g.: `.eslintrc.js`) ESLint's `parser` option is not configured by default anymore, and `babel-eslint` requirement has been removed. We can considering this as a breaking change, end users should now configure `parser: 'babel-eslint` themselves. Method `Encore.enableEslintLoader('extends-name')` has been deprecated and undocumented, in order to prevent end users to configure ESLint through Encore. A nice message is also displayed when no ESLint configuration is found: ![Capture d’écran de 2020-01-12 11-15-09](https://user-images.githubusercontent.com/2103975/72217430-dfa2a480-352d-11ea-96e3-0e77236127d6.png) I couldn't use `FriendlyErrorsPlugin` for this because error is not coming from Webpack. If someone has a better idea... 😕 **Pros:** - end users have now full control hover ESLint configuration **by default** - no need to `delete options.parser` when trying to use [`eslint-plugin-vue`](https://github.com/vuejs/eslint-plugin-vue) or [`@typescript-eslint/parser`](https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/parser) - IDEs will be able to integrate ESLint (if they support it) **Cons:** - end users should now configure `parser` option to `babel-eslint` themselves - end users will have to move their config to external config file, but it's ok What do you think? Thanks. **EDIT:** tests failures are unrelated I think. Commits ------- 9d3b02f tweaking wording and order d23982a Display message if no ESLint configuration is found c828b32 Deprecate `Encore.enableEslintLoader('extends-name')` 9f31d95 Add test for ESLint with external configuration (and babel-eslint usage) 3ba6cf2 Remove babel-eslint from ESLint configuration
symfony · Mar 20, 2020 · 829d84a · 829d84a
2 parents 28086b1 + 9d3b02f
commit 829d84a
Show file tree

Hide file tree

Showing 10 changed files with 123 additions and 12 deletions.
diff --git a/fixtures/js/eslint-es2018.js b/fixtures/js/eslint-es2018.js
@@ -0,0 +1,2 @@
+  const a = { x: 0, y: 1, z: 2 };
+  const { x, ...b } = a;
diff --git a/index.js b/index.js
@@ -1070,18 +1070,14 @@ class Encore {
      * // enables the eslint loaded using the default eslint configuration.
      * Encore.enableEslintLoader();
      *
-     * // Optionally, you can pass in the configuration eslint should extend.
-     * Encore.enableEslintLoader('airbnb');
-     *
      * // You can also pass in an object of options
      * // that will be passed on to the eslint-loader
      * Encore.enableEslintLoader({
-     *     extends: 'airbnb',
      *     emitWarning: false
      * });
      *
      * // For a more advanced usage you can pass in a callback
-     * // https://github.com/MoOx/eslint-loader#options
+     * // https://github.com/webpack-contrib/eslint-loader#options
      * Encore.enableEslintLoader((options) => {
      *      options.extends = 'airbnb';
      *      options.emitWarning = false;

diff --git a/lib/WebpackConfig.js b/lib/WebpackConfig.js
@@ -686,6 +686,8 @@ class WebpackConfig {
         }
 
         if (typeof eslintLoaderOptionsOrCallback === 'string') {
+            logger.deprecation('enableEslintLoader: Extending from a configuration is deprecated, please use a configuration file instead. See https://eslint.org/docs/user-guide/configuring for more information.');
+
             this.eslintLoaderOptionsCallback = (options) => {
                 options.extends = eslintLoaderOptionsOrCallback;
             };

diff --git a/lib/features.js b/lib/features.js
@@ -103,7 +103,6 @@ const features = {
         packages: [
             { name: 'eslint' },
             { name: 'eslint-loader', enforce_version: true },
-            { name: 'babel-eslint', enforce_version: true },
         ],
         description: 'Enable ESLint checks'
     },

diff --git a/lib/loaders/eslint.js b/lib/loaders/eslint.js
@@ -13,6 +13,14 @@ const WebpackConfig = require('../WebpackConfig'); //eslint-disable-line no-unus
 const loaderFeatures = require('../features');
 const applyOptionsCallback = require('../utils/apply-options-callback');
 
+function isMissingConfigError(e) {
+    if (!e.message || !e.message.includes('No ESLint configuration found')) {
+        return false;
+    }
+
+    return true;
+}
+
 module.exports = {
     /**
      * @param {WebpackConfig} webpackConfig
@@ -21,9 +29,42 @@ module.exports = {
     getOptions(webpackConfig) {
         loaderFeatures.ensurePackagesExistAndAreCorrectVersion('eslint');
 
+        const eslint = require('eslint'); // eslint-disable-line node/no-unpublished-require
+        const engine = new eslint.CLIEngine({
+            cwd: webpackConfig.runtimeConfig.context,
+        });
+
+        try {
+            engine.config.getConfigHierarchy(webpackConfig.runtimeConfig.context);
+        } catch (e) {
+            if (isMissingConfigError(e)) {
+                const chalk = require('chalk').default;
+                const packageHelper = require('../package-helper');
+
+                const message = `No ESLint configration has been found.
+
+${chalk.bgGreen.black('', 'FIX', '')} Run command ${chalk.yellow('./node_modules/.bin/eslint --init')} or manually create a ${chalk.yellow('.eslintrc.js')} file at the root of your project.
+
+If you prefer to create a ${chalk.yellow('.eslintrc.js')} file by yourself, here is an example to get you started:
+
+${chalk.yellow(`// .eslintrc.js
+module.exports = {
+    parser: 'babel-eslint',
+    extends: ['eslint:recommended'],
+}
+`)}
+
+Install ${chalk.yellow('babel-eslint')} to prevent potential parsing issues: ${packageHelper.getInstallCommand([[{ name: 'babel-eslint' }]])}
+
+`;
+                throw new Error(message);
+            }
+
+            throw e;
+        }
+
         const eslintLoaderOptions = {
             cache: true,
-            parser: 'babel-eslint',
             emitWarning: true
         };
 

diff --git a/lib/package-helper.js b/lib/package-helper.js
@@ -195,4 +195,5 @@ module.exports = {
     getMissingPackageRecommendations,
     getInvalidPackageVersionRecommendations,
     addPackagesVersionConstraint,
+    getInstallCommand,
 };
diff --git a/test/config-generator.js b/test/config-generator.js
@@ -372,6 +372,10 @@ describe('The config-generator function', () => {
         });
 
         it('enableEslintLoader("extends-name")', () => {
+            before(() => {
+                logger.reset();
+            });
+
             const config = createConfig();
             config.addEntry('main', './main');
             config.publicPath = '/';
@@ -380,6 +384,7 @@ describe('The config-generator function', () => {
 
             const actualConfig = configGenerator(config);
 
+            expect(JSON.stringify(logger.getMessages().deprecation)).to.contain('enableEslintLoader: Extending from a configuration is deprecated, please use a configuration file instead. See https://eslint.org/docs/user-guide/configuring for more information.');
             expect(JSON.stringify(actualConfig.module.rules)).to.contain('eslint-loader');
             expect(JSON.stringify(actualConfig.module.rules)).to.contain('extends-name');
         });

diff --git a/test/functional.js b/test/functional.js
@@ -9,6 +9,7 @@
 
 'use strict';
 
+const os = require('os');
 const chai = require('chai');
 chai.use(require('chai-fs'));
 const expect = chai.expect;
@@ -1721,6 +1722,73 @@ module.exports = {
             }, true);
         });
 
+        it('When enabled, eslint checks for linting errors by using configuration from file', (done) => {
+            const cwd = process.cwd();
+            after(() => {
+                process.chdir(cwd);
+            });
+
+            const appDir = testSetup.createTestAppDir();
+            const config = testSetup.createWebpackConfig(appDir, 'www/build', 'dev');
+            config.setPublicPath('/build');
+            config.addEntry('main', './js/eslint-es2018');
+            config.enableEslintLoader({
+                // Force eslint-loader to output errors instead of sometimes
+                // using warnings (see: https://github.com/MoOx/eslint-loader#errors-and-warning)
+                emitError: true,
+            });
+            fs.writeFileSync(
+                path.join(appDir, '.eslintrc.js'),
+                `
+module.exports = {
+    parser: 'babel-eslint',
+    rules: {
+        'indent': ['error', 2],
+        'no-unused-vars': ['error', { 'args': 'all' }]
+    }
+}                            `
+            );
+
+            process.chdir(appDir);
+
+            testSetup.runWebpack(config, (webpackAssert, stats) => {
+                const eslintErrors = stats.toJson().errors[0];
+
+                expect(eslintErrors).not.to.contain('Parsing error: Unexpected token ..');
+                expect(eslintErrors).to.contain('Expected indentation of 0 spaces but found 2');
+                expect(eslintErrors).to.contain('\'x\' is assigned a value but never used');
+                expect(eslintErrors).to.contain('\'b\' is assigned a value but never used');
+
+                done();
+            }, true);
+        });
+
+        it('When enabled and without any configuration, ESLint will throw an error and a nice message should be displayed', (done) => {
+            const cwd = process.cwd();
+
+            this.timeout(5000);
+            setTimeout(() => {
+                process.chdir(cwd);
+                done();
+            }, 4000);
+
+            const appDir = testSetup.createTestAppDir(os.tmpdir()); // to prevent issue with Encore's .eslintrc.js
+            const config = testSetup.createWebpackConfig(appDir, 'www/build', 'dev');
+            config.setPublicPath('/build');
+            config.addEntry('main', './js/eslint');
+            config.enableEslintLoader({
+                // Force eslint-loader to output errors instead of sometimes
+                // using warnings (see: https://github.com/MoOx/eslint-loader#errors-and-warning)
+                emitError: true,
+            });
+
+            process.chdir(appDir);
+
+            expect(() => {
+                testSetup.runWebpack(config, (webpackAssert, stats) => {});
+            }).to.throw('No ESLint configration has been found.');
+        });
+
         it('Code splitting with dynamic import', (done) => {
             const config = createWebpackConfig('www/build', 'dev');
             config.setPublicPath('/build');

diff --git a/test/helpers/setup.js b/test/helpers/setup.js
@@ -25,8 +25,8 @@ const testFixturesDir = path.join(__dirname, '../', '../', 'fixtures');
 
 let servers = [];
 
-function createTestAppDir() {
-    const testAppDir = path.join(tmpDir, Math.random().toString(36).substring(7));
+function createTestAppDir(rootDir = tmpDir) {
+    const testAppDir = path.join(rootDir, Math.random().toString(36).substring(7));
 
     // copy the fixtures into this new directory
     fs.copySync(testFixturesDir, testAppDir);

diff --git a/test/loaders/eslint.js b/test/loaders/eslint.js
@@ -30,7 +30,6 @@ describe('loaders/eslint', () => {
 
         expect(actualOptions).to.deep.equal({
             cache: true,
-            parser: 'babel-eslint',
             emitWarning: true
         });
     });
@@ -45,7 +44,6 @@ describe('loaders/eslint', () => {
 
         expect(actualOptions).to.deep.equal({
             cache: true,
-            parser: 'babel-eslint',
             emitWarning: true,
             extends: 'airbnb'
         });
@@ -61,7 +59,6 @@ describe('loaders/eslint', () => {
 
         expect(actualOptions).to.deep.equal({
             cache: true,
-            parser: 'babel-eslint',
             emitWarning: false
         });
     });