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

Remove ESLint user-related config #687

Merged
merged 5 commits into from
Mar 20, 2020

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Jan 12, 2020

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
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 or @typescript-eslint/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.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 13, 2020

Thanks @Kocal, I think that we really need this :)

I couldn't use FriendlyErrorsPlugin for this because error is not coming from Webpack. If someone has a better idea... 😕

Not a big issue imo, the message looks fine (edit: ahah, no wonder it looks fine and really similar to the ones we already have after pushing that missing commit 😄).

EDIT: tests failures are unrelated I think.

Yeah, the ones on AppVeyor have been there for a while now (and I still have no clue what causes them...), and the ones on Travis are related to TypeScript (a bit weird that they only happen for that job though).

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WONDERFUL.

Sorry for the slow review. I tweaked the message slightly, but that's all. I'm ok with the breaking change, because it will be an explosion: if the user does not have an external config file, it will explode and tell them (not fail silently).

@weaverryan
Copy link
Member

Thank you @Kocal!

@weaverryan weaverryan merged commit 829d84a into symfony:master Mar 20, 2020
@Kocal Kocal deleted the remove-eslint-user-config branch March 20, 2020 14:27
Kocal added a commit to Kocal/webpack-encore that referenced this pull request Mar 20, 2020
weaverryan added a commit that referenced this pull request 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants