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

enable eslinting of .vue files #504

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@elmariachi111
Copy link

commented Jan 29, 2019

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.

enable eslinting of .vue files
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

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

Aaaah so it was coming from this...

I didn't even think about it, thanks!

@elmariachi111

This comment has been minimized.

Copy link
Author

commented Jan 29, 2019

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

elmariachi111 added some commits Jan 29, 2019

@Lyrkan
Copy link
Collaborator

left a comment

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?

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

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

This comment has been minimized.

Copy link
@Lyrkan

Lyrkan Jan 29, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Kocal

Kocal Jan 30, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Kocal

Kocal Jan 30, 2019

Contributor

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;
    }
}

This comment has been minimized.

Copy link
@Lyrkan

Lyrkan Jan 30, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@Kocal

Kocal Jan 30, 2019

Contributor

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... 🤔

weaverryan added a commit that referenced this pull request Mar 25, 2019

feature #509 feat: Add method to configure loaders rules (Kocal, weav…
…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

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

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

@Lyrkan

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

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 used 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

This comment has been minimized.

Copy link

commented May 2, 2019

What do you think?

As a user I like this proposal.

@Lyrkan

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

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

This comment has been minimized.

Copy link

commented May 2, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

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

This comment has been minimized.

Copy link

commented May 2, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

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

This comment has been minimized.

Copy link

commented May 3, 2019

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.

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.