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

Possible incompatibility with enableEslintLoader() and eslint-vue-plugin #473

Closed
Kocal opened this issue Dec 20, 2018 · 3 comments · Fixed by #509
Closed

Possible incompatibility with enableEslintLoader() and eslint-vue-plugin #473

Kocal opened this issue Dec 20, 2018 · 3 comments · Fixed by #509
Labels

Comments

@Kocal
Copy link
Contributor

Kocal commented Dec 20, 2018

Hi, I am the frontend dev that was blaming webpack-encore one year ago 👀

First, let me congratulate you @weaverryan and the community, webpack-encore seems has become very mature over time. It's nice to have webpack 4, Babel 7 and ESLint working out of the box.

Even if I still prefer using vue-cli for a SPA (with Symfony as API) or a raw simple webpack config for smaller project, webpack-encore is really a must have for more oriented Symfony projects.


So I'm facing an issue with using the ESLint loader with eslint-plugin-vue for linting .vue files.

This is how I configure the eslint-loader:

Encore.enableEslintLoader(config => {
  config.configFile = __dirname + '/.eslintrc';
});

This is my .eslintrc:

{
  "extends": [
    "airbnb-base",
    "plugin:vue/recommended",
    "@yproximite/base"
  ],
  "rules": {
      ...
   }
}

And this is my ESLint dependencies:

❱Development❰ app@my-app.vm:/srv/app|⇒  yarn list --pattern eslint  
yarn list v1.12.3
├─ @yproximite/eslint-config-base@0.2.0
│  ├─ babel-eslint@10.0.1
│  └─ eslint-scope@3.7.1
├─ babel-eslint@8.2.6
│  └─ eslint-scope@3.7.1
├─ eslint-config-airbnb-base@13.1.0
├─ eslint-config-prettier@3.3.0
├─ eslint-import-resolver-node@0.3.2
├─ eslint-loader@1.9.0
├─ eslint-module-utils@2.2.0
├─ eslint-plugin-import@2.14.0
├─ eslint-plugin-prettier@3.0.0
├─ eslint-plugin-vue@5.0.0
├─ eslint-restricted-globals@0.1.1
├─ eslint-scope@4.0.0
├─ eslint-utils@1.3.1
├─ eslint-visitor-keys@1.0.0
├─ eslint@5.9.0
└─ vue-eslint-parser@4.0.3

And this is what happens when running webpack-encore:

selection_007

This is a webpack-encore issue because everything is fine when running ESLint manually:

selection_008

I followed the link What is the "Use the latest vue-eslint-parser" error?, and it's a problem of ESLint parser configuration.

We should do this to fix the problem:

- "parser": "babel-eslint",
  "parserOptions": {
+     "parser": "babel-eslint",
      "ecmaVersion": 2017,
      "sourceType": "module"
  }

But the parser options is hardcoded in loaders/eslint.js:
selection_009

So I updated my code and tried this:

Encore.enableEslintLoader(config => {
  config.configFile = __dirname + '/.eslintrc';
  delete config.parser;
});
Screenshot

selection_010

or even this:

Encore.enableEslintLoader(config => {
  config.configFile = __dirname + '/.eslintrc';
  config.parserOptions = Object.assign({}, config.parserOptions || {}, {
    parser: 'babel-eslint'
  });
  delete config.parser;
});
Screenshot

selection_011

I have no more eslint-loader errors, but eslint-loader seems to not be working anymore 🤷‍♂️

Does someone already faced this issue?
I will do more investigation and see if I can help more.

Thanks!

@Kocal
Copy link
Contributor Author

Kocal commented Dec 20, 2018

That's really strange.
This is the final configuration that ESLint use (console.log(config) here) and this is what I got:
selection_012

Options parser and parserOptions.parser are correct, but webpack-encore still not displays ESLint warnings/errors 😞

This is what I have when running ESLint manually, and everything works fine:
selection_013

@elmariachi111
Copy link

The problem is imho not in the configuration but rather in the loader rule setup: the eslint loader that encore configures never adds .vue files to its rules. Added it in my PR when eslint & vue loader are both active. With that patch you can

.eslintrc.json

module.exports = {
    root: true,
    parserOptions: {
      parser: "babel-eslint",
      ecmaVersion: 2017,
      sourceType: 'module',
    },
    env: {
      browser: true,
    },
    extends: ["standard", "plugin:vue/recommended"],
    plugins: ['vue'],
  };

webpack.config.js

const eslintrc = require('./.eslintrc.json')
//...
.enableVueLoader()
  .enableEslintLoader(options => {
    return eslintrc
  })
//...

and I think these are all the dependencies:
yarn add -D eslint eslint-config-standard eslint-loader eslint-plugin-import eslint-plugin-promise eslint-plugin-standard vue-eslint-parser

plays well together with vscode's js language server when this is set:

{
    "eslint.validate": [
      "javascript",
      "javascriptreact",
      { "language": "vue", "autoFix": true }
    ],
    "vetur.validation.template": false,
    "eslint.alwaysShowStatus": true,
    "eslint.autoFixOnSave": true
  }

good luck!

@Kocal
Copy link
Contributor Author

Kocal commented Mar 2, 2019

If someone has the same issue, since #509 you will be able to write this:

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

weaverryan added a commit that referenced this issue Mar 25, 2019
…erryan)

This PR was merged into the master branch.

Discussion
----------

feat: Add method to configure loaders rules

This PR is a proposal to resolve #473 and #504 in a clean way.

`Encore.configureLoaderRule()` is a low-level function, and has for goal to let the user having full access to Webpack loaders rules (what we push into `module.rules`) and configure them.

This is the implementation of the idea I had in #504 (comment).

For resolving Vue files linting issue, the next example would be the ideal solution  I think:
```js
Encore
  .configureLoaderRule('eslint', loader => {
    loader.test = /\.(jsx?|vue)$/
  });

// actually, the equivalent code is something like this:
const webpackConfig = Encore.getWebpackConfig();
const eslintLoader = webpackConfig.module.rules.find(rule => rule.loader === 'eslint-loader');

eslintLoader.test = /\.(jsx?|vue)$/;

return webpackConfig;
```

For now, only ESLint loader is supported, but we can easily add other loaders.

Let me know what you think of this solution, thanks!

Commits
-------

94da0c2 language tweak
9dd6ca4 chore(tests): correct some tests due to last feature for CSSModules
4dbe443 chore(cr): add real aliases support
729a696 feat: add warning
5cf3d02 feat: add missing loaders, add more tests
86dc788 refactor(test): `configureLoaderRule()` will be easier to test
39cb9bd chore(cr): move tests into
bc0e9bf chore(cr): add shortcut function applyRuleConfigurationCallback
bc1d025 chore(cr): more user-friendly error for valid configurable loaders
01880cf Add method on "Encore"
49a4258 add tests
9892516 feat: implement "configureLoaderRule" method
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants