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

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

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Kocal
Copy link
Contributor

commented May 3, 2019

This PR add a second argument to Encore.enableEslintLoader() that is used to let the ESLint loader lint .vue files:

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

Using lintVue won't add any ESLint configuration, that the job of the final user (see #504 (comment)).

Kocal added some commits May 3, 2019

@Kocal Kocal force-pushed the Kocal:feat/eslint-vue branch from 29c40a3 to 6c35a2d May 4, 2019

@masi

This comment has been minimized.

Copy link

commented May 5, 2019

FYI: to make the Vue linter plugin work you have to use it's own parser:

    .enableEslintLoader(options => {
        options.parser = 'vue-eslint-parser';
        options.parserOptions.parser = 'babel-eslint';
    })

If you fail to do so, linting will fail:

https://eslint.vuejs.org/user-guide/#what-is-the-use-the-latest-vue-eslint-parser-error

Or is this is part of the configuration work left for the user to do? Otherwise the user may assume that the loader ensures not only "babel-eslint" but "also "vue-eslint-parser" which it currently does not. Actually the current code doesn't even verify if the Vue loader is enabled at all.

Sidenote: if you're using typescript then it is @typescript-eslint/parserfor parserOptions.parser.

@Kocal

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Or is this is part of the configuration work left for the user to do?

Normally yes, but since Encore already configure some ESLint options (loader) I may think to no.

I don't like to configure ESLint with .enableEslintLoader() - because linting issues are not displayed in IDE, and we can't run ESLint manually -, but maybe we can document something like this:

Encore.enableEslintLoader(options => {
  // remove Encore pre-defined value
  delete options.parser; // let eslint-vue-plugin handle the new parser value

  options.extends = (options.extends || []).concat('plugin:vue/recommended');
  options.parserOptions = Object.assign({}, options.parserOptions || {}, {
    parser: 'babel-eslint'
  }); 
}, { 
  lintVue: true
});

If it were up to me, I would only use .enableEslintLoader() to use my .eslintrc file to prevent issues:

Encore.enableEslintLoader(options => {
  options.configFile = __dirname + '/.eslintrc';
  delete options.parser; // I'm not sure I should do this, it's taken from #473 
}, {
  lintVue: true
});

In my opinion, Encore should only run and not configure ESLint. 😕

Actually the current code doesn't even verify if the Vue loader is enabled at all.

You don't need to enable Vue loader to lint Vue files with ESLint loader.

@Kocal

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

In fact I even don't find my PR useful, using .configureLoaderRule() should be enough: 🤔

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

This comment has been minimized.

Copy link

commented May 5, 2019

I don't like to configure ESLint with .enableEslintLoader() - because linting issues are not displayed in IDE, and we can't run ESLint manually

And I don't like the fact that the configuration is hidden away within Encore/Webpack.

Encore.enableEslintLoader(options => {
  options.configFile = __dirname + '/.eslintrc';
  delete config.parser; // I'm not sure I should do this, it's taken from #473 
}, {
  lintVue: true
});

I didn't delete config.parser, but set it to the same value as in my .eslintrc (which has been BTW been deprectated in favour of .eslintrc.yaml and .eslintrc.js).

Does Encore need to set babel-eslint as parser? If it didn't we hadn't do go into such troubles for Vue (and Typescript) which need a different configuration.

In my opinion, Encore should only run and not configure ESLint. 😕

Perhaps the new eslintOption lintVue could turn off the default configuration completely.

Personally I'm happy to use configureLoaderRule and kicking the default configuraton, but getting a warning about messing with the loader rules every times is annoying.

@Kocal

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Personally I'm happy to use configureLoaderRule and kicking the default configuraton, but getting a warning about messing with the loader rules every times is annoying.

We can improving the actual behavior by using a boolean like displayWarningForConfigureLoaderRule = true, display the warning when it's true, and set it at false after calling the method. 👍

@Lyrkan

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Sorry for not looking into this sooner.

I actually prefer this solution to #504 even if it only adds .vue files to the ones processed by the eslint-loader.

but maybe we can document something like this:

Encore.enableEslintLoader(options => {
  // remove Encore pre-defined value
  delete options.parser; // let eslint-vue-plugin handle the new parser value

  options.extends = (options.extends || []).concat('plugin:vue/recommended');
  options.parserOptions = Object.assign({}, options.parserOptions || {}, {
    parser: 'babel-eslint'
  }); 
}, { 
  lintVue: true
});

I'm against that as it is way to complicated and won't be understood by someone who isn't used to JavaScript (many Symfony devs aren't).

If it were up to me, I would only use .enableEslintLoader() to use my .eslintrc file to prevent issues:

Encore.enableEslintLoader(options => {
  options.configFile = __dirname + '/.eslintrc';
  delete config.parser; // I'm not sure I should do this, it's taken from #473 
}, {
  lintVue: true
});

Do you actually need all of that? we are not changing the default value for configFile, so it should already look for an .eslintrc file by default, no?

As for parser you'd also have to set it in your .eslintrc file anyway, so our default value would probably be overriden.

If we remove both of those lines we get something way simpler:

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

In fact I even don't find my PR useful, using .configureLoaderRule() should be enough: 🤔

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

I'd say that for now it should be enough, but I don't like recommending it since it relies on our internal implementation, which prevents us from guaranteeing BC.

In my opinion, Encore should only run and not configure ESLint.

When I started to respond I was disagreeing with you about that point, but the more I think about it the more I feel that you may be right...

We should probably only be configuring things that can't be set from within an .eslintrc file (loader options or CLIEngine-only options).

@Kocal

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

I'm against that as it is way to complicated and won't be understood by someone who isn't used to JavaScript (many Symfony devs aren't).

Nah just forgot about this, I don't know what happens in my head when I wrote this... 😅

Do you actually need all of that? we are not changing the default value for configFile, so it should already look for an .eslintrc file by default, no?

No we actually don't need to configure configFile, actually only parser is to configure.

As for parser you'd also have to set it in your .eslintrc file anyway, so our default value would probably be overriden.

That what I thought but nope, I have to delete options.parser to make it works. Otherwise I have the following error:
Capture d’écran de 2019-05-17 07-03-04

It's a well known error (see FAQ), it seems that eslint-loader override .eslint* config 🤔

By using the following code, eslint-loader is able to lint .vue files properly:

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

Capture d’écran de 2019-05-17 07-23-46

(Note that the issue with ./DataTable.scss?module, I don't have this when running ESLint manually. I will probably open a new issue soon 😉 )

When I started to respond I was disagreeing with you about that point, but the more I think about it the more I feel that you may be right...

In fact, the main problem with Encore configuring ESLint is that Encore pre-configure ESLint and the final user is not aware about that (unless he take a look to the source code).

And also the fact that IDEs won't be able to provide ESLint integration.

We should probably only be configuring things that can't be set from within an .eslintrc file (loader options or CLIEngine-only options).

Definitly yes! 😄

@Lyrkan

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2019

That what I thought but nope, I have to delete options.parser to make it works. Otherwise I have the following error:

Yep... it actually makes sense since options from the loader are passed to the CLIEngine and that ESLint uses the following order of precedence (https://eslint.org/docs/user-guide/configuring#configuration-cascading-and-hierarchy):

The complete configuration hierarchy, from highest precedence to lowest precedence, is as follows:

  1. Inline configuration
    1.1. /eslint-disable/ and /eslint-enable/
    1.2. /global/
    1.3. /eslint/
    1.4. /eslint-env/
  2. Command line options (or CLIEngine equivalents):
    2.1. --global
    2.2.--rule
    2.3. --env
    2.4. -c, --config
  3. Project-level configuration:
    3.1. .eslintrc.* or package.json file in same directory as linted file
    3.2. Continue searching for .eslintrc and package.json files in ancestor directories (parent has highest precedence, then grandparent, etc.), up to and including the root directory or until a config with "root": true is found.
  4. In the absence of any configuration from (1) through (3), fall back to a personal default configuration in ~/.eslintrc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.