ember-cli / ember-cli-eslint

Ember CLI addon for linting Ember projects with ESLint
MIT License
116 stars 47 forks source link

Leverage eslint-plugin-compat with `this.project.targets` #158

Open cibernox opened 7 years ago

cibernox commented 7 years ago

Some context: https://github.com/amilajack/eslint-plugin-compat

This eslint plugin is gaining popularity. The idea is to customize the rules based on the target browsers, warning when the user attempts to use a feature that is not present across the board for the entire support matrix, unless it has been polyfilled.

Examples of those include:

This issue intends to be a placeholder to discuss wether we want to bake this into this plugin or develop it as a separate addon, and track its implementation if needed.

rwjblue commented 7 years ago

I'm generally in favor, but I think the extent of things here would be to update the default .eslintrc.js file in the blueprint.

We should experiment with the config required, but I suspect something like this should suffice:

const targets = require('config/targets');

module.exports = {
  root: true,
  parserOptions: {
    ecmaVersion: 6,
    sourceType: 'module'
  },
  extends: 'eslint:recommended',
  env: {
    browser: true
  },
  plugins: ['compat'],
  rules: {
    'compat/compat': [2, targets.browsers]
  }
};

Can someone test / confirm this?

cibernox commented 7 years ago

I'll try to teste it once the first ember-cli 2.13-beta with support for targets goes out

rwjblue commented 7 years ago

The beauty of this is that all you have to have is a config/targets.js file for the above to work, no need to wait 😺

rwjblue commented 7 years ago

We should confirm the above works properly now...

SlyDen commented 7 years ago

in my case I got such error:

Configuration for rule "compat/compat" is invalid:
    Value "ie 11,last 2 Edge versions,last 2 Chrome versions,last 2 Firefox versions,last 1 Safari versions" has more items than allowed.

though if I omitted targets.browsers, e.g.: 'compat/compat': 2 it was ok ...

cafreeman commented 7 years ago

Ditto on the above. Getting this error with latest ember-cli, eslint, etc.

cafreeman commented 7 years ago

Actually I guess it's slightly different:

Configuration for rule "compat/compat" is invalid:
    Value "last 2 Edge versions,last 2 Chrome versions,last 2 Firefox versions,last 2 Safari versions,last 2 iOS versions,last 2 ChromeAndroid versions" should NOT have more than 0 items.
sandydoo commented 7 years ago

Couldn't get eslint-plugin-compat to accept the browser targets via the rule and I'm not sure it currently supports that option. However, it does support specifying the targets in eslintrc using settings! Also had to provide a relative path for the require too.

Working config:

const targets = require('./config/targets');

module.exports = {
  root: true,
  parserOptions: {
    ecmaVersion: 6,
    sourceType: 'module'
  },
  extends: 'eslint:recommended',
  env: {
    browser: true
  },
  settings: {
    targets: targets.browsers
  },
  plugins: ['compat'],
  rules: {
    'compat/compat': 2
  }
};

Tested with:

ember-cli-eslint@4.2.0
eslint-plugin-compat@1.0.4
raido commented 6 years ago

Passing targets through settings: {..} also worked for me.

raido commented 6 years ago

Actually eslint-plugin-compat does not currently support everything, for example Object.assign will pass through without a notice. So use with caution.