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

Use a custom ESLint rule list #27

Merged
merged 3 commits into from
Jul 19, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ Currently it is a thin layer on top of many amazing community projects, such as:
* [webpack](https://webpack.github.io/) with [webpack-dev-server](https://github.com/webpack/webpack-dev-server), [html-webpack-plugin](https://github.com/ampedandwired/html-webpack-plugin) and [style-loader](https://github.com/webpack/style-loader)
* [Babel](http://babeljs.io/) with [preset-es2015](https://www.npmjs.com/package/babel-preset-es2015), [preset-es2016](https://www.npmjs.com/package/babel-preset-es2016), [preset-react](https://www.npmjs.com/package/babel-preset-react) and [transform-rest-spread](https://babeljs.io/docs/plugins/transform-object-rest-spread/)
* [Autoprefixer](https://github.com/postcss/autoprefixer)
* [ESLint](http://eslint.org/) with [eslint-config-airbnb](https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb)
* [ESLint](http://eslint.org/)
* and more.

All of them are transient dependencies of the provided npm package.
Expand Down
3 changes: 0 additions & 3 deletions config/.eslintrc

This file was deleted.

14 changes: 14 additions & 0 deletions config/babel.dev.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

module.exports = {
cacheDirectory: true,
presets: ['es2015', 'es2016', 'react'],
plugins: ['transform-object-rest-spread']
};
16 changes: 16 additions & 0 deletions config/babel.prod.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

module.exports = {
presets: ['es2015', 'es2016', 'react'],
plugins: [
'transform-object-rest-spread',
'transform-react-constant-elements'
]
};
280 changes: 280 additions & 0 deletions config/eslint.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,280 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

// Inspired by https://github.com/airbnb/javascript but less opinionated.

var OFF = 0; // rules that split the community (e.g. semicolons)
var WARNING = 1; // style rules accepted by the majority of popular styleguides
var ERROR = 2; // rules that prevent common mistakes

module.exports = {
root: true,

plugins: ['react', 'import'],

env: {
es6: true,
commonjs: true,
browser: true
},

parserOptions: {
ecmaVersion: 6,
sourceType: 'module',
ecmaFeatures: {
jsx: true,
generators: true,
experimentalObjectRestSpread: true
}
},

settings: {
'import/resolver': {
node: {
extensions: ['.js', '.json']
}
},
'import/extensions': ['.js'],
'import/ignore': [
'node_modules',
'\\.(json|css|jpg|png|gif|svg|eot|svg|ttf|woff|woff2|mp4|webm)$',
]
},

rules: {

// http://eslint.org/docs/rules/

'array-callback-return': ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about sorting these? Just kind of annoying to find things otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, totally. I was mostly looking at airbnb's configs, and they are split into multiple files, so I ended up with several sorted “regions”. I wouldn’t spend time on this right now because there are more pressing issues, but this would be nice.

'block-scoped-var': WARNING,
curly: [WARNING, 'multi-line'],
'default-case': [ERROR, { commentPattern: '^no default$' }],
'dot-notation': [WARNING, { allowKeywords: true }],
'dot-location': [ERROR, 'property'],
eqeqeq: [ERROR, 'allow-null'],
'guard-for-in': ERROR,
'no-caller': ERROR,
'no-case-declarations': WARNING,
'no-empty-pattern': ERROR,
'no-eval': ERROR,
'no-extend-native': ERROR,
'no-extra-bind': ERROR,
'no-extra-label': ERROR,
'no-fallthrough': ERROR,
'no-implied-eval': ERROR,
'no-invalid-this': WARNING,
'no-iterator': ERROR,
'no-labels': [ERROR, { allowLoop: false, allowSwitch: false }],
'no-lone-blocks': ERROR,
'no-loop-func': ERROR,
'no-multi-spaces': WARNING,
'no-multi-str': ERROR,
'no-native-reassign': ERROR,
'no-new': ERROR,
'no-new-func': ERROR,
'no-new-wrappers': ERROR,
'no-octal': ERROR,
'no-octal-escape': ERROR,
'no-redeclare': ERROR,
'no-return-assign': ERROR,
'no-script-url': ERROR,
'no-self-assign': ERROR,
'no-self-compare': ERROR,
'no-sequences': ERROR,
'no-throw-literal': ERROR,
'no-unused-expressions': ERROR,
'no-unused-labels': ERROR,
'no-useless-concat': ERROR,
'no-useless-escape': ERROR,
'no-with': ERROR,
radix: ERROR,
yoda: WARNING,
'no-cond-assign': [ERROR, 'always'],
'no-console': OFF, // TODO: enable for production?
'no-constant-condition': WARNING,
'no-control-regex': ERROR,
'no-debugger': WARNING, // TODO: enable for production?
'no-dupe-args': ERROR,
'no-dupe-keys': ERROR,
'no-duplicate-case': ERROR,
'no-empty': [WARNING, {
allowEmptyCatch: true
}],
'no-empty-character-class': ERROR,
'no-ex-assign': ERROR,
'no-extra-boolean-cast': WARNING,
'no-extra-semi': WARNING,
'no-func-assign': ERROR,
'no-invalid-regexp': ERROR,
'no-irregular-whitespace': WARNING,
'no-negated-in-lhs': ERROR,
'no-obj-calls': ERROR,
'no-prototype-builtins': WARNING,
'no-regex-spaces': ERROR,
'no-sparse-arrays': ERROR,
'no-unexpected-multiline': ERROR,
'no-unreachable': ERROR,
'no-unsafe-finally': WARNING,
'use-isnan': ERROR,
'valid-typeof': ERROR,
'block-spacing': [WARNING, 'always'],
'brace-style': [WARNING, '1tbs', { allowSingleLine: true }],
camelcase: [WARNING, { properties: 'never' }],
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like it will be controversial!

'comma-spacing': [WARNING, { before: false, after: true }],
'comma-style': [WARNING, 'last'],
'computed-property-spacing': [WARNING, 'never'],
'eol-last': WARNING,
indent: [WARNING, 2],
Copy link
Contributor

Choose a reason for hiding this comment

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

"2 spaces always" seems controversial

'key-spacing': [WARNING, { beforeColon: false, afterColon: true }],
'keyword-spacing': [WARNING, {
before: true,
after: true,
overrides: {
return: { after: true },
throw: { after: true },
case: { after: true }
}
}],
'linebreak-style': [WARNING, 'unix'],
'new-cap': [ERROR, { newIsCap: true }],
'new-parens': ERROR,
'newline-per-chained-call': [WARNING, { ignoreChainWithDepth: 4 }],
'no-array-constructor': ERROR,
'no-mixed-operators': [ERROR, {
groups: [
['+', '-', '*', '/', '%', '**'],
['&', '|', '^', '~', '<<', '>>', '>>>'],
['==', '!=', '===', '!==', '>', '>=', '<', '<='],
['&&', '||'],
['in', 'instanceof']
],
allowSamePrecedence: false
}],
'no-mixed-spaces-and-tabs': WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

does this conflict with some editors? like if you use intelliJ I think this may be a common setting. not sure though

'no-multiple-empty-lines': [WARNING, { max: 2, maxEOF: 1 }],
'no-nested-ternary': WARNING,
'no-new-object': ERROR,
'no-restricted-syntax': [
ERROR,
'LabeledStatement',
'WithStatement',
],
'no-spaced-func': WARNING,
'no-trailing-spaces': WARNING,
'no-unneeded-ternary': [WARNING, { defaultAssignment: false }],
'no-whitespace-before-property': ERROR,
'object-property-newline': [WARNING, {
allowMultiplePropertiesPerLine: true,
}],
'one-var': [WARNING, 'never'],
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it should be ok for people to do multiple declarations in a line? this config and one-var-declaration-per-line are relevant

'one-var-declaration-per-line': [WARNING, 'always'],
'operator-assignment': [ERROR, 'always'],
'semi-spacing': [WARNING, { before: false, after: true }],
'space-before-blocks': WARNING,
'space-before-function-paren': [WARNING, { anonymous: 'always', named: 'never' }],
'space-in-parens': [WARNING, 'never'],
'space-infix-ops': WARNING,
'space-unary-ops': [WARNING, {
words: true,
nonwords: false,
overrides: {},
}],
'spaced-comment': [WARNING, 'always', {
exceptions: ['-', '+'],
markers: ['=', '!']
}],
'unicode-bom': [ERROR, 'never'],
'no-delete-var': ERROR,
'no-label-var': ERROR,
'no-shadow': WARNING,
'no-shadow-restricted-names': ERROR,
'no-undef': ERROR,
'no-unused-vars': [ERROR, { vars: 'local', args: 'after-used' }],
'no-use-before-define': [ERROR, 'nofunc'],
'arrow-spacing': [WARNING, { before: true, after: true }],
'generator-star-spacing': [WARNING, { before: false, after: true }],
'no-confusing-arrow': [WARNING, {
allowParens: true,
}],
'no-const-assign': ERROR,
'no-dupe-class-members': ERROR,
'no-duplicate-imports': WARNING,
'no-new-symbol': ERROR,
'no-this-before-super': ERROR,
'no-useless-computed-key': ERROR,
'no-useless-constructor': ERROR,
'no-useless-rename': [ERROR, {
ignoreDestructuring: false,
ignoreImport: false,
ignoreExport: false,
}],
'no-var': WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

This one seems like it will be divisive

'prefer-arrow-callback': [WARNING, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems divisive too

allowNamedFunctions: false,
allowUnboundThis: true,
}],
'prefer-rest-params': ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems bothersome because it forces people to learn what rest params are, if they already are happy using arguments

'require-yield': ERROR,
'rest-spread-spacing': [ERROR, 'never'],
'template-curly-spacing': WARNING,
'yield-star-spacing': [WARNING, 'after'],
strict: [ERROR, 'never'],

// https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/

'import/no-unresolved': [ERROR, { commonjs: true }],
'import/named': ERROR,
'import/default': ERROR,
'import/namespace': ERROR,
'import/export': ERROR,
'import/no-named-as-default': ERROR,
'import/no-named-as-default-member': ERROR,
'import/no-extraneous-dependencies': ERROR,
'import/no-mutable-exports': WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is so bad about exporting something you initially defined with "let"? In general these import rules seem quite sticklery

'import/no-commonjs': WARNING,
'import/no-amd': ERROR,
'import/imports-first': WARNING,
'import/no-duplicates': ERROR,
'import/prefer-default-export': WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

I have not seen this import/prefer-default-export rule but it seems like a pain. If you have a few different named exports, and you remove all but one, now you need to refactor all your callers?


// https://github.com/yannickcr/eslint-plugin-react/tree/master/docs/rules
'react/jsx-handler-names': [WARNING, {
eventHandlerPrefix: 'handle',
eventHandlerPropPrefix: 'on',
}],
'react/jsx-key': WARNING,
'react/jsx-no-duplicate-props': [ERROR, { ignoreCase: true }],
'react/jsx-no-undef': ERROR,
'react/jsx-pascal-case': [ERROR, {
allowAllCaps: true,
ignore: [],
}],
'react/jsx-uses-react': [ERROR, { pragma: 'React' }],
'react/jsx-uses-vars': ERROR,
'react/no-deprecated': ERROR,
'react/no-did-mount-set-state': [WARNING, 'allow-in-func'],
'react/no-did-update-set-state': [WARNING, 'allow-in-func'],
'react/no-direct-mutation-state': ERROR,
'react/no-is-mounted': ERROR,
'react/no-multi-comp': [WARNING, { ignoreStateless: true }],
'react/no-string-refs': WARNING,
'react/prefer-es6-class': OFF, // TODO: revisit after updating docs
'react/prefer-stateless-function': OFF, // TODO: revisit after updating docs
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this OFF - a lot of people do not understand the stateless function way of doing react, and they don't really need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is partly because the docs don’t introduce it well.
It’s worth revisiting in a year or so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just find it annoying to refactor between a function and a class when I change whether something has state or not....

'react/react-in-jsx-scope': ERROR,
'react/require-render-return': ERROR,
'react/jsx-space-before-closing': WARNING,
'react/wrap-multilines': [WARNING, {
declaration: true,
assignment: true,
return: true
}],
'react/jsx-equals-spacing': [ERROR, 'never'],
'react/jsx-indent': [WARNING, 2]
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems likely to be a controversial one, because different people will use different numbers of spaces

}
};
15 changes: 6 additions & 9 deletions config/webpack.config.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ var relative = isInNodeModules ? '../../..' : '..';
module.exports = {
devtool: 'eval',
entry: [
'./src/index.js',
'webpack-dev-server/client?http://localhost:3000'
'webpack-dev-server/client?http://localhost:3000',
'./src/index.js'
],
output: {
// Next line is not used in dev but WebpackDevServer crashes without it:
Expand All @@ -42,11 +42,7 @@ module.exports = {
test: /\.js$/,
include: path.resolve(__dirname, relative, 'src'),
loader: 'babel',
query: {
cacheDirectory: true,
presets: ['es2015', 'es2016', 'react'],
plugins: ['transform-object-rest-spread']
}
query: require('./babel.dev')
},
{
test: /\.css$/,
Expand All @@ -58,7 +54,7 @@ module.exports = {
loader: 'json'
},
{
test: /\.(jpg|png|gif|eot|svg|ttf|woff|woff2)$/,
test: /\.(jpg|png|gif|svg|eot|svg|ttf|woff|woff2)$/,
loader: 'file',
},
{
Expand All @@ -68,7 +64,8 @@ module.exports = {
]
},
eslint: {
configFile: path.join(__dirname, '.eslintrc')
configFile: path.join(__dirname, 'eslint.js'),
useEslintrc: false
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! this is what we needed to fix it

},
postcss: function() {
return [autoprefixer];
Expand Down
Loading