RFC: Use .eslintrc.* files for ESLint configuration #657

Kocal · 2019-10-18T18:02:31Z

This issue is a following of the discussion from #574 and with @weaverryan on Slack.

The problem

Currently, Encore configure ESLint (eslint-loader) to use babel-eslint parser.
This is nice, because now ESLint uses Babel to parse files, and it's easy and transparent for the end-user.

However, this should not be configured by Encore itself. Here what the eslint-loader documentation says:

Note that the config option you provide will be passed to the CLIEngine. This is a different set of options than what you'd specify in package.json or .eslintrc.

Some options (like parser) are common to the two sets of options. However, I think that some of those options should not be passed eslint-loader options, but only in .eslintrc/package.json files, where the end-user configurations should belongs.

Normally forcing babel-eslint should not be a problem because it allows ESLint to parse more files, but it is when configuring ESLint to lint Vue files (#574) because eslint-plugin-vue use eslint-vue-parser for parser option, not babel-eslint, which then is configured in parserOptions object. See this link for more information.
It is also a problem when you are trying to lint TypeScript code which require @typescript-eslint/parser, see #685.

And since options passed to eslint-loader overrides the external configuration (.eslintrc.* files), then we have to do this to let ESLint lint Vue files properly:

Encore
  .enableEslintLoader(options => {
    delete options.parser;
  }, { 
    lintVue: true,
  })

This is clearly not ideal. Also, we can think But if lintVue is true, then Encore can delete options.parser itself? but no because we are again interfering with ESLint configuration that should be configured by the end-user.
What will happens if someone create a new ESLint plugin for Vue files but which required babel-eslint parser? (I don't think it's possible, but why not 😄).

We also think it's a better solution to configure ESLint outside Encore, with .eslintrc.* files, because it can be used by other tools like PhpStorm/VS Code which provide ESLint integration. Having an IDE ESLint integration is not possible when configuring ESLint with Encore.

A solution

With @weaverryan, we thought about doing the same with Babel in Encore.
When enableEslintLoader() is used, then we check if an external ESLint configuration (e.g.: in a .eslintrc.js file) exists.
If it's the case, then we use it. If it's not the case, then we throw an error.

We will try to mimic ESLint CLIEngine's behavior by loading configuration from files:

.eslintrc.js
.eslintrc.yaml
.eslintrc.yml
.eslintrc.json
.eslintrc
package.json, in eslintConfig property

We will probably check if file defined in options.configFile (enableEslintLoader()) exists.

If no external configuration is found, then we can display to the end-user a minimal .eslintrc.js file to use, with some installation/usage steps.

This is a small breaking change, but it can by easily fixed with yarn add -D babel-eslint and adding parser: 'babel-eslint' to a .babelrc.js file.

The text was updated successfully, but these errors were encountered:

Lyrkan · 2019-10-21T07:27:51Z

Hey @Kocal,

When enableEslintLoader() is used, then we check if an external ESLint configuration (e.g.: in a .eslintrc.js file) exists.
If it's the case, then we use it. If it's not the case, then we throw an error.

Should we really throw an error? I'm scared that we'll be missing some edge cases where a valid external config actually exists (for instance eslint even fallbacks to searching for one in the user's home dir).

We will try to mimic ESLint CLIEngine's behavior by loading configuration from files

If we don't define anything in the loader's option it should be loaded by the CLIEngine automatically, right? Or do you mean that we'll check if the following files exist?

In the latter case I'm not sure checking all the extensions is needed, we could probably look for a filename that matches ^\.eslintrc(\..+)?$.

Kocal · 2019-10-22T07:06:02Z

When enableEslintLoader() is used, then we check if an external ESLint configuration (e.g.: in a .eslintrc.js file) exists.
If it's the case, then we use it. If it's not the case, then we throw an error.

Should we really throw an error? I'm scared that we'll be missing some edge cases where a valid external config actually exists (for instance eslint even fallbacks to searching for one in the user's home dir).

Throwing an error is maybe too agressive for the end-user.

Maybe just display a warning with some installation intructions (yarn add -D babel-eslint) and displaying a minimal .eslintrc.js config file?

We will try to mimic ESLint CLIEngine's behavior by loading configuration from files

If we don't define anything in the loader's option it should be loaded by the CLIEngine automatically, right? Or do you mean that we'll check if the following files exist?

This wasn't very clear for me, this is a part of a Slack conversation with Ryan:

I've tried to run Encore with eslint-loader on a project, and effectively we don't need to specify options.configFile. It already load the configuration alone, without any Encore's help.

This is how I configure eslint-loader with Encore for now, the configuration from my .eslintrc is loaded automatically:

Encore
  .enableEslintLoader(options => {
    delete options.parser;
  })
  .configureLoaderRule('eslint', loader => {
    loader.test = /\.(jsx?|vue)$/;
  })

In fact, maybe we should do nothing:

don't try to fetch ESLint configuration
don't display errors nor warnings

And we let eslint-loader doing the job and that's all. 🤔

But we still need to remove parser configuration, we will have to notify end-users in the next release notes.

WDYT?

Lyrkan · 2019-10-24T07:00:59Z

Maybe just display a warning with some installation intructions (yarn add -D babel-eslint) and displaying a minimal .eslintrc.js config file?

I'd be fine with a warning like that if no .eslintrc* file is detected.
No need to display an .eslintrc.js example though, ESLint has a nice generator for it: ./node_modules/.bin/eslint --init

In fact, maybe we should do nothing:

don't try to fetch ESLint configuration

don't display errors nor warnings

And we let eslint-loader doing the job and that's all. 🤔

I'd also be fine with that, but implementing the warning would be a nice touch imo :)

Kocal · 2019-10-24T07:08:31Z

Mmmh yes it seems to be nice, displaying a warning if a .eslintrc* file and eslintConfig field in package.json is not found should be enough.

I've pinged Ryan on Slack about this issue, we should wait for it's opinion too.

weaverryan · 2019-10-24T10:04:57Z

Hi!

Is there any real use-case for having no configuration file? I like the warning idea, but I want to make sure there’s not a legitimate reason for this situation... and so a user might get annoyed (but that’s a minor concern).

Maybe the earring links to some docs showing a “nice” config file to start with? We’re all about guiding users who want the help.

Lyrkan · 2019-10-25T16:11:27Z

Hey @weaverryan,

Is there any real use-case for having no configuration file? I like the warning idea, but I want to make sure there’s not a legitimate reason for this situation... and so a user might get annoyed (but that’s a minor concern).

It could also be configured through comment (but I don't think that's widespread) or through the eslint-loader's options. To be honest I'm more worried about false positives caused by someone creating an .eslintrc.* file at an unexpected location.

But maybe that could be solved by either telling the user to create an .eslintrc.ignore file (that would be picked up by the ^\.eslintrc(\..+)?$ regexp) or adding an option to enableEslintLoader():

Encore.enableEslintLoader(
  () => {}, // We could probably also accept `null` here
  { disableEslintRcCheck: true }
);

Maybe the earring links to some docs showing a “nice” config file to start with? We’re all about guiding users who want the help.

I think telling the user to run ./node_modules/.bin/eslint --init would be a better idea since it already handles various presets and will always be in sync with the installed version of eslint.

Kocal · 2019-12-29T21:32:16Z

Hey everyone ✋

I don't think it's a good idea to have a file .eslintrc.ignore, because:

it's a file specific only to Encore, the file name is too generic
it can confuse the end user with the file .eslintignore which is used to configure ESLint to ignore files/paths

The best option is to add the disableEslintRcCheck: true option, which will toggle the message Run eslint --init [...].

This parameter should be used as a last resorts when no ESLint configuration are found.

Also to be 100% clear, as done for Babel configuration, the check of the ESLint configuration should be done by ESLint, not by Encore.

Kocal · 2020-01-11T06:03:36Z

Someone faced the babel-eslint issue while trying to setup ESLint to parse TypeScript files: #685

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

weaverryan · 2020-03-20T14:21:17Z

@Kocal This can be closed now, correct?

Kocal · 2020-03-20T14:26:37Z

Yes!

This PR was squashed before being merged into the master branch. Discussion ---------- Proposal to replace #504 (ESLint/Vue) This PR add a second argument to `Encore.enableEslintLoader()` that is used to let the ESLint loader lint `.vue` files: ```js Encore.enableEslintLoader(() => {}, { lintVue: true }); ``` Using `lintVue` won't add any ESLint configuration, that the job of the final user (see #504 (comment)). **EDIT:** While #657 is being discussed, you can use the following code to: - let ESLint process your `.vue` files - prevent the error `Use the latest vue-eslint-parser.`, see #656 ```js Encore.enableEslintLoader((options) => { delete options.parser; }, { lintVue: true }); ``` **EDIT 2:** PR #687 has been merged and issue #657 is now resolved. It means that you can use the following code to let eslint-loader handle `.vue` files: ```js Encore.enableEslintLoader(() => {}, { lintVue: true }); ``` Commits ------- 13b0750 chore: remove comment for vue files linting since #687 has been merged 2f1e85b chore: add comment for making .vue files lint working 12b3f77 eslint/vue: add 2nd parameter to Encore#enableEslintLoader eb85b24 eslint/vue: tweak config-generator aa782df eslint/vue: implement `getTest()` on ESlint loader helper bc444ff eslint/vue: tweak `WebpackConfig#enableEslintLoader()`

Kocal mentioned this issue Oct 19, 2019

Proposal to replace #504 (ESLint/Vue) #574

Merged

Kocal changed the title ~~Use .eslintrc.* files for ESLint configuration~~ RFC: Use .eslintrc.* files for ESLint configuration Oct 19, 2019

Kocal mentioned this issue Jan 11, 2020

Webpack encore requires "babel-eslint" even if not used #685

Closed

Kocal mentioned this issue Jan 12, 2020

Remove ESLint user-related config #687

Merged

Kocal closed this as completed Mar 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Use .eslintrc.* files for ESLint configuration #657

RFC: Use .eslintrc.* files for ESLint configuration #657

Kocal commented Oct 18, 2019 •

edited

Loading

Lyrkan commented Oct 21, 2019

Kocal commented Oct 22, 2019 •

edited

Loading

Lyrkan commented Oct 24, 2019

Kocal commented Oct 24, 2019

weaverryan commented Oct 24, 2019

Lyrkan commented Oct 25, 2019

Kocal commented Dec 29, 2019

Kocal commented Jan 11, 2020

weaverryan commented Mar 20, 2020

Kocal commented Mar 20, 2020

RFC: Use .eslintrc.* files for ESLint configuration #657

RFC: Use .eslintrc.* files for ESLint configuration #657

Comments

Kocal commented Oct 18, 2019 • edited Loading

The problem

A solution

Lyrkan commented Oct 21, 2019

Kocal commented Oct 22, 2019 • edited Loading

Lyrkan commented Oct 24, 2019

Kocal commented Oct 24, 2019

weaverryan commented Oct 24, 2019

Lyrkan commented Oct 25, 2019

Kocal commented Dec 29, 2019

Kocal commented Jan 11, 2020

weaverryan commented Mar 20, 2020

Kocal commented Mar 20, 2020

Kocal commented Oct 18, 2019 •

edited

Loading

Kocal commented Oct 22, 2019 •

edited

Loading