enable eslinting of .vue files #504

elmariachi111 · 2019-01-29T21:03:45Z

addresses #473 : if the eslintloader is activated aside the vue loader it also must load .vue files. This proposal might not be complete: for that case we could also add dependency notes so users are hinted to install vue-eslint-parser.

addresses #473 : if the eslintloader is activated aside the vue loader it also must load .vue files. This proposal might not be complete: for that case we could also add dependency notes so users are hinted to install `vue-eslint-parser`.

Kocal · 2019-01-29T21:10:35Z

Aaaah so it was coming from this...

I didn't even think about it, thanks!

elmariachi111 · 2019-01-29T21:25:38Z

oooops. My PR CI fails because of.... linting errors 😂

Lyrkan

Hi @elmariachi111,

Thanks for working on that :)

Do you think you could also add a test to check if linting warning/errors are now working properly?

As for your proposal about vue-eslint-parser... that would probably be a good thing to have. How does Eslint behave with .vue files if that parser is missing? If it throws an error maybe you could add something there to detect it?

Lyrkan · 2019-01-29T21:43:15Z

lib/config-generator.js

@@ -318,8 +318,12 @@ class ConfigGenerator {
        }

        if (this.webpackConfig.useEslintLoader) {
+            const test = this.webpackConfig.useVueLoader


I'm wondering if that test is needed... would it be a bad thing if we always included .vue files? If the vue loader isn't enabled (and not added manually) the build would fail anyway right?

If we always include .vue files:

it makes our code a bit simpler

if for some reason someone chooses not to call enableVueLoader but set-up its own loader manually they can still use our default ESLint config

If the vue loader isn't enabled (and not added manually) the build would fail anyway right?

Yes, I think ESLint will throw an error like this if the user didn't have configurer eslint-plugin-vue:

Unexpected token <template> ....

A safer way is to let the user configure ESLint loader:

Encore .enableVueLoader() .enableEslintLoader() .configureEslintLoader(loader => { loader.test = /.(jsx?|vue)$/ })

It's a bit more verbose, but it will prevent some issues.

Or maybe a more generic method, if we don't want to add one method for each loader:

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

That will store configuration callbacks in a single property:

class WebpackConfig { constructor(runtimeConfig) { this.loaderConfigurationCallbacks = { 'eslint': () => {}, 'vue': () => {}, '...': () => {}, } } configureLoaderRule(loaderName, callback) { // add checks ... this.loaderConfigurationCallbacks[loaderName] = callback; } }

Yes, I think ESLint will throw an error like this if the user didn't have configurer eslint-plugin-vue

Hm, yes but that's only if you try to import a .vue file right?

What I meant is that if you don't enable the vue-loader the build will fail because Webpack won't know how to process those files anyway.

And if you don't use .vue files, well, that's not an issue because you'll never get that error.

This raises a point though... should we force people to use the Eslint vue plugin by default if they have .vue files?

Hm, yes but that's only if you try to import a .vue file right?

Yep

What I meant is that if you don't enable the vue-loader the build will fail because Webpack won't know how to process those files anyway.

And if you don't use .vue files, well, that's not an issue because you'll never get that error.

Hum you're right, I didn't think this way.

This raises a point though... should we force people to use the Eslint vue plugin by default if they have .vue files?

I don't know. I'm probably sure that some people will complain about that. 🤔

But if we do, we should not hardcode eslint-plugin-vue configuration inside Encore.
Maybe write a documentation or display some warnings in Encore about that... 🤔

…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

Kocal · 2019-03-26T10:42:06Z

I guess this one can be closed since #509 has been merged.

Lyrkan · 2019-03-27T13:15:41Z

@Kocal I'm not sure, due to how it works configureLoaderRule() should only be used as a last recourse in my opinion. Linting vue file seems like a really common use case to me and we could probably make it work...

Maybe not automatically but through an enableEslintLoader(...) option?

Kocal · 2019-04-01T13:21:47Z

Yes it's common to linting Vue files, but I don't think it's a good idea to force the user.

The good behavior for me is, if the user enabled ESLint loader and VueLoader, then:

if the dependency eslint-plugin-vue is installed, we enable linting on Vue files
if not, we display a warning like If you want to lint Vue files, then install the dependency "eslint-plugin-vue" [...]

Also we should probably tell the user he can configure ESLint+Vue with the following code:

Encore.configureEslintLoader({
  extends: [
    // add more generic rulesets here, such as:
    // 'eslint:recommended',
    'plugin:vue/recommended'
  ]
});

(In fact I'm not even sure that we can do this 😅)

What do you think?

masi · 2019-05-02T19:21:23Z

What do you think?

As a user I like this proposal.

Lyrkan · 2019-05-02T19:35:36Z

I don't really like the idea of automatically enabling features based on which packages are installed (especially since the notion of "installed" isn't as obvious as it seems because of dependency hoisting).

I think we could probably just tweak Encore.enableEslintLoader() (which currently only accepts 1 argument) to automatically enable the plugin + vue/recommended:

Encore.enableEslintLoader(options => {
    // This is applied on the final options object
    // options.extends = [ /* ... */ ];
}, {
    // Make the eslint-loader also process `.vue`
    // files and add `plugin:vue/recommended` to
    // extended rulesets.
    useVuePlugin: true,
});

masi · 2019-05-02T20:16:48Z

I think we could probably just tweak Encore.enableEslintLoader() (which currently only accepts 1 argument) to automatically enable the plugin + vue/recommended:

Ok. Though Encore is opinioned in other areas and enables features you have even to look up the sourcecode to know they are turned on (eg Terser or Define).

BTW, if I have enables TypeScript I want to linit it too. Now that ESLint has taken over from TSLint you want to probably have something like this as well:

Encore.enableEslintLoader(options => {
    // This is applied on the final options object
}, {
    // Make the eslint-loader also process `.ts` files
    // and add `plugin:@typescript-eslint/recommended` to
    // extended rulesets.
    useTypescriptPlugin: true,
});

Lyrkan · 2019-05-02T20:36:59Z

Now that ESLint has taken over from TSLint

Is that already the case? As far as I know TSLint will only be deprecated once ESLint checks are on the same level than the ones from TSLint... which is far from being achieved based on the following roadmap.

Also, since a TSLint => ESLint compat package is planned (cf. this post), it doesn't really make sense to start using ESLint for TypeScript files.

masi · 2019-05-02T20:52:29Z

Also, since a TSLint => ESLint compat package is planned (cf. this post), it doesn't really make sense to start using ESLint for TypeScript files.

As I read it the compat package is for ESLint, so former TSLint users can use their old TSLint rules with ESLint.

Besides the fact that ESLint isnt there at 100% Webpack has made to move: webpack/webpack-cli#834
But I digress this this is about .vue support though for my use I will have to get either TSLint or ESLint workin with .vue files. I'd prefer using ESLint as TSLint is now living corpse.

Lyrkan · 2019-05-02T22:06:02Z

As I read it the compat package is for ESLint, so former TSLint users can use their old TSLint rules with ESLint.

That's what I understood as well.

What I meant is that since a compatibility package will be available, using TSLint isn't a big issue. It still receives updates and has some useful checks that are not available in ESLint yet.

That being said I'm not against your proposal of also adding an useTypescriptPlugin option :)

Kocal · 2019-05-03T19:57:05Z

Hum, I'm thinking about something when reading this:

if the dependency eslint-plugin-vue is installed, we enable linting on Vue files

I think we could probably just tweak Encore.enableEslintLoader() (which currently only accepts 1 argument) to automatically enable the plugin + vue/recommended ...

In fact, how the user will be able to choose between every sets of rules? (base, essential, strongly-recommended, and recommended)? What if the user does not want to use any preset but just some rules? 🤔

To be honest, I really don't think we can't/should do more than enable eslint-loader to be ran on .vue files (test: /.(jsx?|vue)$/). 😕

masi · 2019-05-03T20:46:20Z

To be honest, I really don't think we can't/should do more than enable eslint-loader to be ran on .vue files (test: /.(jsx?|vue)$/). 😕

Does the Encore configure anything else for ESLint? AFAIC the only thing Encore does is to enable the eslint loader. So there isn't much to do than provide a nice interface to enable ESLint for other file types like .vue, .ts or whatever ESLint can handle. Though one could argue that adding the recommended rules for all supported files is the way to go. After all you can reconfigure it manually anyway.

Should be enough for .vue files. To lint typescript you need not only have the plugins @typescript-eslint but also the parser @typescript-eslint/parser enabled. Which makes it a bit more interesting to setup when you have both .js and .ts files in your sources. Unless @typescript-eslint/parser can handle plain JS without any issues. I have tried tried it out.

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 · 2020-05-06T15:51:04Z

This can be closed since #574 has been merged.

enable eslinting of .vue files

2e68ef3

addresses #473 : if the eslintloader is activated aside the vue loader it also must load .vue files. This proposal might not be complete: for that case we could also add dependency notes so users are hinted to install `vue-eslint-parser`.

elmariachi111 added 2 commits January 29, 2019 22:33

make mr lint happy :)

f3cac74

whitespace

3ccd3a7

Lyrkan reviewed Jan 29, 2019

View reviewed changes

Kocal mentioned this pull request Feb 2, 2019

feat: Add method to configure loaders rules #509

Merged

Kocal mentioned this pull request May 3, 2019

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

Merged

elmariachi111 closed this Jun 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable eslinting of .vue files #504

enable eslinting of .vue files #504

elmariachi111 commented Jan 29, 2019

Kocal commented Jan 29, 2019

elmariachi111 commented Jan 29, 2019

Lyrkan left a comment

Lyrkan Jan 29, 2019 •

edited

Kocal Jan 30, 2019

Kocal Jan 30, 2019 •

edited

Lyrkan Jan 30, 2019

Kocal Jan 30, 2019 •

edited

Kocal commented Mar 26, 2019

Lyrkan commented Mar 27, 2019

Kocal commented Apr 1, 2019 •

edited

masi commented May 2, 2019

Lyrkan commented May 2, 2019

masi commented May 2, 2019

Lyrkan commented May 2, 2019 •

edited

masi commented May 2, 2019 •

edited

Lyrkan commented May 2, 2019

Kocal commented May 3, 2019

masi commented May 3, 2019

Kocal commented May 6, 2020

enable eslinting of .vue files #504

enable eslinting of .vue files #504

Conversation

elmariachi111 commented Jan 29, 2019

Kocal commented Jan 29, 2019

elmariachi111 commented Jan 29, 2019

Lyrkan left a comment

Choose a reason for hiding this comment

Lyrkan Jan 29, 2019 • edited

Choose a reason for hiding this comment

Kocal Jan 30, 2019

Choose a reason for hiding this comment

Kocal Jan 30, 2019 • edited

Choose a reason for hiding this comment

Lyrkan Jan 30, 2019

Choose a reason for hiding this comment

Kocal Jan 30, 2019 • edited

Choose a reason for hiding this comment

Kocal commented Mar 26, 2019

Lyrkan commented Mar 27, 2019

Kocal commented Apr 1, 2019 • edited

masi commented May 2, 2019

Lyrkan commented May 2, 2019

masi commented May 2, 2019

Lyrkan commented May 2, 2019 • edited

masi commented May 2, 2019 • edited

Lyrkan commented May 2, 2019

Kocal commented May 3, 2019

masi commented May 3, 2019

Kocal commented May 6, 2020

Lyrkan Jan 29, 2019 •

edited

Kocal Jan 30, 2019 •

edited

Kocal Jan 30, 2019 •

edited

Kocal commented Apr 1, 2019 •

edited

Lyrkan commented May 2, 2019 •

edited

masi commented May 2, 2019 •

edited