Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Kocal opened this issue Oct 18, 2019 · 10 comments
Closed

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

Kocal opened this issue Oct 18, 2019 · 10 comments

Comments

@Kocal
Copy link
Contributor

@Kocal Kocal commented Oct 18, 2019

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.

@Kocal Kocal changed the title Use .eslintrc.* files for ESLint configuration RFC: Use .eslintrc.* files for ESLint configuration Oct 19, 2019
@Lyrkan
Copy link
Collaborator

@Lyrkan Lyrkan commented Oct 21, 2019

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
Copy link
Contributor Author

@Kocal Kocal commented Oct 22, 2019

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:

Sélection_999(346)

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
Copy link
Collaborator

@Lyrkan Lyrkan commented Oct 24, 2019

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
Copy link
Contributor Author

@Kocal Kocal commented Oct 24, 2019

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
Copy link
Member

@weaverryan weaverryan commented Oct 24, 2019

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
Copy link
Collaborator

@Lyrkan Lyrkan commented Oct 25, 2019

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
Copy link
Contributor Author

@Kocal Kocal commented Dec 29, 2019

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
Copy link
Contributor Author

@Kocal Kocal commented Jan 11, 2020

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

weaverryan added a commit that referenced this issue Mar 20, 2020
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
Copy link
Member

@weaverryan weaverryan commented Mar 20, 2020

@Kocal This can be closed now, correct?

@Kocal
Copy link
Contributor Author

@Kocal Kocal commented Mar 20, 2020

Yes!

@Kocal Kocal closed this Mar 20, 2020
weaverryan added a commit that referenced this issue Apr 17, 2020
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()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.