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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

enable eslinting of .vue files #504

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e68ef3
enable eslinting of .vue files
elmariachi111 Jan 29, 2019
f3cac74
make mr lint happy :)
elmariachi111 Jan 29, 2019
3ccd3a7
whitespace
elmariachi111 Jan 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/config-generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,12 @@ class ConfigGenerator {
}

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

@Lyrkan Lyrkan Jan 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Kocal Kocal Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member

@Kocal Kocal Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

? /\.(jsx?|vue)$/
: /\.jsx?$/;

rules.push({
test: /\.jsx?$/,
test,
loader: 'eslint-loader',
exclude: /node_modules/,
enforce: 'pre',
Expand Down