Add support for eslint

[pinoniq]

Webpack encore adds built in support for a coupld of "wide spread" front end libraries. Being ReactJs, jQuery and SASS.

There is however one that is even more widespread: eslint. Having an .enableEslint() (or even enabling it by default in non production mode) would be a plus.

However, if we go along the "default should be good enough" path. This means taking a side in the many different code-styles that exist in the JavaScript world.

A small list:

• https://www.drupal.org/docs/develop/standards/javascript/javascript-coding-standards
• https://google.github.io/styleguide/jsguide.html
• https://github.com/airbnb/javascript
• https://github.com/rwaldron/idiomatic.js/
• https://github.com/standard/standard

Instead of choosing a side, symfony could use the default eslint, with the possibility to change the "extends" property.

.enableEslint({
  extends: "airbnb"
})

Combining this with .autoProvidejQuery() and .enableReactPreset() would basically resort to something like

.addLoader({
  test: /\.(js|jsx)$/,
  loader: 'eslint-loader',
  exclude: [/node_modules/],
  enforce: "pre",
  options: {
    "extends": "airbnb",
    "parser": "babel-eslint",
    "settings": {
      "import/resolver": {
        "webpack": {
          "config": "webpack.config.js"
        }
      }
    },
    "globals": {
      "$": false
    }
    emitWarning: true
  }
})

Is this something webpack encore is willing to add? If so, what path do we take?

[weaverryan]

Hi there!

Interesting! So basically, this would output build warnings if eslint fails, right? I think it’s interesting; after all, it would be opt-in. But mostly, I like it including this in your Webpack build is considered a fairly common “best practice” (versus something that’s “possible”, but few people do it). Do you have an impression about that?

[pinoniq]

At the company I work every webpack/gulp/grunt build task includes eslint. This to avoid having the developer to miss linting errors and only see it on post-commit instead of during his work.

Especially given the fact it adds very little overhead and allows you to see eslint errors "live" in the console running the encore.

How far this is a "best practice" in the world I don't really know. The eslint-loader we use has about 40k downloads a day: https://www.npmjs.com/package/eslint-loader . Most blogs even include it in the "default webpack/react setup" and I'm used to it. Haven't spoken about it to other people on conferences however.

And yes, it should be opt-in. Especially given the Symfony flex philosophy.

[pinoniq]

The stack we use is:

yarn add --dev eslint babel-eslint eslint-loader eslint-config-airbnb eslint-plugin-import eslint-plugin-jsx-a11y eslint-plugin-react

the webpack.config.js has the following added line:

.addLoader({
        test: /\.(js|jsx)$/,
        loader: 'eslint-loader',
        exclude: [/node_modules/],
        enforce: "pre",
        options: {
            configFile: './.eslintrc',
            emitWarning: true
        }
    })

with the .eslintrc set to:

{
  "extends": "airbnb",
  "parser": "babel-eslint",
  "rules": {
    "linebreak-style": "off"
  },
  "settings": {
    "import/resolver": {
      "webpack": {
        "config": "webpack.config.js"
      }
    }
  }
}

This genreates the following output when configured with eslint: (I added some errors as example)

What I propose is to have the following "interface":

.enableEslint("airbnb")

It will then function in the same way as babel does. e.g. the moment you add a .eslintrc file, it uses that one instead of the "built-in" one.

wdyt?

[Lyrkan]

Hi,

That'd be a great addition, we'll also have to think about doing the same for tslint-loader.

About the enableEslint method I like the idea of keeping it really simple and easy to use but maybe we should consider using a callback like we already do for some other methods of the API, for instance:

Encore.enableEslint(options => {
    options.extends = 'airbnb';
});

It makes it a bit harder to use it but allows you to change a single options (other than extends) in case you need it.

Another possibility would be to mix both approaches:

// Use default preset
Encore.enableEslint();

// Only change `extends`
Encore.enableEslint('airbnb');

// Change the `extends` and other options
Encore.enableEslint('airbnb', options => {
    options.emitWarning = false;
});

[pinoniq]

I don't like the very last example. It makes the 'extends' property a little bit too special.

I like the idea of the callback however, But what about the approach of supporting whatever the user gives us (in true jQuery style)?

// Use default preset
Encore.enableEslint();

// Only change `extends`
Encore.enableEslint('airbnb');

// other options using a simple object
// Uses Object.assign({}, encoreDefaultOptions, options); internally
Encore.enableEslint({
    extends: 'airbnb',
    emitWarning: false;
});

// other options using callback that returns the options to use.
// this because of https://eslint.org/docs/rules/no-param-reassign
Encore.enableEslint(options => ({
    ...options,
    emitWarning: false;
}));

[weaverryan]

Great info! I like it! And I agree with the “interface”, though we probably will also need an optional callback arg of some sort. Would you be willing to open a PR for this?

[Lyrkan]

@weaverryan The callback is supported by @pinoniq's latest proposal, the idea being to have a different behavior based on the type of the first (and only) parameter.

The only thing that worries me a bit there is the options => ({ ...options, ... }) callback. While I definitely agree that it is better to respect the no-param-reassign rule we don't do that for the existing methods (probably because some callbacks allow to modify multiple parameters), and that may be a bit confusing for users:

Encore
    .enableVueLoader(options => ({
        // Won't work
        ...options,
        preLoaders: { ... }
    }))
    .enableEslint(options => ({
        // Will work
        ...options,
        emitWarning: false
    }))
;

[vkryukov76]

Don't forget about loading settings from the .eslintrc file.

[pinoniq]

@weaverryan I'll look into opening my first PR to the symfony community somewhere tomorrow or friday.

@Lyrkan True about the confusing part. Maybe have a follow up ticket to work on a "single interface to rule them all" ? And thus for now, I go for the same as for Vue?

@vkryukov76 that can be done using:

Encore
    .enableEslint({
        configFile: './.eslintrc',
    })
;

[pinoniq]

I apologize for the delay. I finally managed to open a pr for this. Later than expected

[wujekbogdan]

@pinoniq

Instead of choosing a side, symfony could use the default eslint, with the possibility to change the "extends" property.

I like the idea of .enableEslint() but IMO this method shouldn't take a config as an argument like proposed:

.enableEslint({
  extends: "airbnb"
})

Some code editors are able to import eslint settings from .eslintrc and .eslintrc.js files and apply formatting rules based on these settings. Extends defined via Encore's config won't be recognized by code editors.

[Lyrkan]

@wujekbogdan In my opinion not allowing to set the eslint-loader options would be a mistake:

• That doesn't prevent using an .eslintrc file (I even think that's the default behavior if you don't specify otherwise)
• Some of these options are not available when using only an .eslintrc file, for instance if you wish to use a different formatter (see https://github.com/webpack-contrib/eslint-loader#options)

[wujekbogdan]

In my opinion not allowing to set the eslint-loader options would be a mistake:

• That doesn't prevent using an .eslintrc file (I even think that's the default behavior if you don't specify otherwise)

You're right. If we still could use the config file then it's fine. I thought that the idea is to configure eslint only through the webpack config.

Some of these options are not available when using only an .eslintrc file, for instance if you wish to use a different formatter

With .eslintrc it's not possible but with .eslintrc.js it should be a problem.

[Lyrkan]

With .eslintrc it's not possible but with .eslintrc.js it should be a problem.

I don't think .eslintrc.js files allow more options than .eslintrc files, and setting formatters are definitely not part of the available ones (that's handled directly by the eslint-loader).

Some other options are also specific to the CLIEngine: https://eslint.org/docs/developer-guide/nodejs-api#cliengine

[tristanbes]

What's the status on this PR ? eslint is a must have :D

[tristanbes]

Can this issue be closed since 2a1a18a was merged ?