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

Support Eslint 7 & add rules #15

Merged
merged 35 commits into from
Oct 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
abd5b3d
upgrade eslint-plugin-react-hooks
moroine May 19, 2020
e0e33d4
Fix dependency list in README
moroine May 19, 2020
d44cec5
upgrade eslint-plugin-flowtype
moroine May 19, 2020
0ada918
upgrade eslint-plugin-react
moroine May 19, 2020
90568b4
set `require-atomic-updates`: `error`
moroine May 19, 2020
2f3fa09
Add flowtype rules
moroine May 19, 2020
3d4e63b
add import rules
moroine May 19, 2020
81e002a
enable no-undefined
moroine May 19, 2020
eb56be7
add style rules
moroine May 19, 2020
5ba8acb
Add react rules
moroine May 19, 2020
de32f93
remove eslint-plugin-ramda
moroine Sep 29, 2020
cc03c02
upgrade dependencies
moroine Sep 29, 2020
1ed29e2
Set minimum NodeJs version
moroine Sep 29, 2020
108abf0
disable sort-keys
moroine Sep 29, 2020
cd4f9af
fix lint
moroine Sep 29, 2020
12689b6
Enable 'flowtype/no-existential-type'
moroine Sep 29, 2020
3f4c2c1
Enable flowtype/no-mutable-array
moroine Sep 29, 2020
d353749
enable flowtype/spread-exact-type
moroine Sep 29, 2020
3c7c575
Update gitignore
moroine Sep 29, 2020
a46b862
v1.0.0-alpha.1
moroine Sep 29, 2020
2206a9b
fix react/function-component-definition rule
moroine Sep 29, 2020
405fcae
v1.0.0-alpha.3
moroine Sep 29, 2020
a223833
disable rules without autofix
moroine Sep 30, 2020
2424c1c
disable flowtype/require-types-at-top
moroine Sep 30, 2020
fb718af
v1.0.0-alpha.4
moroine Sep 30, 2020
2427366
remove flowtype/require-compound-type-alias
moroine Oct 1, 2020
833c392
v1.0.0-alpha.5
moroine Oct 1, 2020
cd6f942
feat: consider import starting with $ to be internal
moroine Oct 6, 2020
cc49768
v1.0.0-alpha.6
moroine Oct 6, 2020
eeb2040
chore: update changelog
moroine Oct 6, 2020
ef3bf96
feat: react/jsx-indent check logicExpressions & attributes
moroine Oct 6, 2020
1e102c1
v1.0.0-alpha.7
moroine Oct 6, 2020
70bdcdd
chore: update Changelog
moroine Oct 7, 2020
00b2789
fix: internal path pattern
moroine Oct 7, 2020
8f4b52c
v1.0.0-alpha.8
moroine Oct 7, 2020
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 .eslintrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"extends": "./base.js"
"extends": "./script.js"
}
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
node_modules/

yarn.lock
.npmrc
40 changes: 40 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,43 @@
# Unreleased

## Changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing breaking changes eslint to ^7.10.0.

- **[Breaking]**: upgrade `eslint` to `^7.10.0`
- **[Breaking]**: upgrade `eslint-plugin-react-hooks` to `^4.1.2`
- **[Breaking]**: upgrade `eslint-plugin-flowtype` to `^5.2.0`
- **[Breaking]**: remove `eslint-plugin-ramda` to `^5.1.0`
- **[Breaking]**: minimum supported NodeJs version 12.18
- upgrade `babel-eslint` to `^10.1.0`
- upgrade `eslint-plugin-eslint-comments` to `^3.2.0`
- upgrade `eslint-plugin-import` to `^2.22.1`
- upgrade `eslint-plugin-jsx-a11y` to `^6.3.1`
- upgrade `eslint-plugin-react` to `^7.21.2`
- Fix dependency list in README
- Consider import starting with $ to be internal

## Likely to cause new errors

- `require-atomic-updates`
- `array-bracket-newline`
- `func-names`: is now reported as error instead of warning
- `func-style`
- `lines-around-directive`: removed & replaced by `padding-line-between-statements`
- `no-multiple-empty-lines`: `{ max: 1, maxBOF: 0, maxEOF: 0 }`
- `no-restricted-syntax`: allow `for..of` usage
- `flowtype/require-readonly-react-props`
- `flowtype/no-existential-type`
- `import/no-namespace`
- `import/order`: add `alphabetize: { order: 'asc' 'caseInsensitive': true }` & change `groups` to the following - `order: `builtin`, `external`, `internal`, `parent`, `sibling`
- `import/no-unassigned-import`
- `import/no-anonymous-default-export`: for functions & classes
- `import/exports-last`
- `import/group-exports`
- `react/function-component-definition`
- `react/jsx-key`
- `react/no-direct-mutation-state`
- `react/state-in-constructor`
- `react/prefer-read-only-props`

# v0.3.1

## Changes
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@ see `eslint-config-nugit/base`.

Our default export contains all of our ESLint rules, including ECMAScript 6+ and FlowJS.

It requires `eslint` and `eslint-plugin-import`.
It requires `eslint`, `eslint-plugin-import` and `eslint-plugin-flowtype`.

- Add `"extends": "nugit/base"` to your .eslintrc
6 changes: 5 additions & 1 deletion base.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ module.exports = {
'./rules/es6',
'./rules/imports',
'./rules/strict',
'./rules/ramda.js',
'./rules/flow.js',
'./rules/eslint-comments',
].map(require.resolve),
Expand All @@ -33,6 +32,11 @@ module.exports = {
'jest': true,
'shared-node-browser': true,
},
rules: {
// Reports modules without any exports, or with unused exports
// https://github.com/benmosher/eslint-plugin-import/blob/f63dd261809de6883b13b6b5b960e6d7f42a7813/docs/rules/no-unused-modules.md
'import/no-unused-modules': 'off',
},
},
],
};
40 changes: 19 additions & 21 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "eslint-config-nugit",
"version": "0.3.1",
"version": "1.0.0-alpha.8",
"description": "Nugit's JS ESLint config, following our styleguide",
"main": "index.js",
"scripts": {
Expand Down Expand Up @@ -28,31 +28,29 @@
},
"homepage": "https://github.com/nugit/eslint-config-nugit",
"devDependencies": {
"babel-eslint": "^10.0.3",
"eslint": "^6.8.0",
"eslint-plugin-eslint-comments": "^3.1.2",
"eslint-plugin-flowtype": "^4.5.3",
"eslint-plugin-import": "^2.19.1",
"eslint-plugin-jsx-a11y": "^6.2.3",
"eslint-plugin-ramda": "^2.5.1",
"eslint-plugin-react": "^7.17.0",
"eslint-plugin-react-hooks": "^2.3.0"
"babel-eslint": "^10.1.0",
"eslint": "^7.10.0",
"eslint-plugin-eslint-comments": "^3.2.0",
"eslint-plugin-flowtype": "^5.2.0",
"eslint-plugin-import": "^2.22.1",
"eslint-plugin-jsx-a11y": "^6.3.1",
"eslint-plugin-react": "^7.21.2",
"eslint-plugin-react-hooks": "^4.1.2"
},
"peerDependencies": {
"babel-eslint": "^10.0.3",
"eslint": "^6.8.0",
"eslint-plugin-eslint-comments": "^3.1.2",
"eslint-plugin-flowtype": "^4.5.3",
"eslint-plugin-import": "^2.19.1",
"eslint-plugin-jsx-a11y": "^6.2.3",
"eslint-plugin-ramda": "^2.5.1",
"eslint-plugin-react": "^7.17.0",
"eslint-plugin-react-hooks": "^2.3.0"
"babel-eslint": "^10.1.0",
"eslint": "^7.10.0",
"eslint-plugin-eslint-comments": "^3.2.0",
"eslint-plugin-flowtype": "^5.2.0",
"eslint-plugin-import": "^2.22.1",
"eslint-plugin-jsx-a11y": "^6.3.1",
"eslint-plugin-react": "^7.21.2",
"eslint-plugin-react-hooks": "^4.1.2"
},
"engines": {
"node": ">= 6"
"node": ">= 12"
},
"dependencies": {
"confusing-browser-globals": "^1.0.7"
"confusing-browser-globals": "^1.0.9"
}
}
3 changes: 1 addition & 2 deletions rules/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ module.exports = {

// Disallow assignments that can lead to race conditions due to usage of await or yield
// https://eslint.org/docs/rules/require-atomic-updates
// TODO: enable, semver-major
'require-atomic-updates': 'off',
'require-atomic-updates': 'error',

// disallow comparisons with the value NaN
'use-isnan': 'error',
Expand Down
20 changes: 16 additions & 4 deletions rules/flow.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,35 @@ module.exports = {
'flowtype/delimiter-dangle': ['error', 'always-multiline', 'always-multiline', 'always-multiline'],
'flowtype/generic-spacing': ['error', 'never'],
'flowtype/newline-after-flow-annotation': ['error', 'always'],
'flowtype/object-type-delimiter': ['error', 'comma'],
'flowtype/no-existential-type': 'error',
'flowtype/no-flow-fix-me-comments': 'off',
'flowtype/no-mixed': 'off',
'flowtype/no-dupe-keys': 'error',
'flowtype/no-mutable-array': 'off',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it forces us to only use $ReadOnlyArray and totally disallow [] or Array though. Because in our codebase, I remember we do mutate some stuff for performance purposes.

https://github.com/gajus/eslint-plugin-flowtype/blob/master/.README/rules/no-mutable-array.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have this

Note that initialization of a variable with an empty array is considered valid (e.g., const values: Array = [];). This behavior resembles the behavior of Flow's unsealed objects, as it is assumed that empty array is intended to be mutated.

And also we will still be able to do perf optimisations using // eslint-disable-next-line it will emphasis the alert on the mutation

'flowtype/no-primitive-constructor-types': 'error',
'flowtype/no-types-missing-file-annotation': 'error',
'flowtype/no-unused-expressions': 'error',
// TODO: disable any?
Copy link
Collaborator

Choose a reason for hiding this comment

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

flowtype/no-weak-types: I think we can keep any for now, else they might be too many changes.

'flowtype/no-weak-types': ['error', { any: false }],
'flowtype/object-type-delimiter': ['error', 'comma'],
'flowtype/require-compound-type-alias': ['off', 'always'],
'flowtype/require-exact-type': 'off',
'flowtype/require-indexer-name': 'off',
'flowtype/require-inexact-type': 'off',
'flowtype/require-parameter-type': 'off',
'flowtype/require-readonly-react-props': 'off',
'flowtype/require-readonly-react-props': 'error',
'flowtype/require-return-type': 'off',
'flowtype/require-types-at-top': 'off',
'flowtype/require-valid-file-annotation': 'off',
'flowtype/sort-keys': ['error', 'asc', { caseSensitive: false, natural: true }],
'flowtype/require-variable-type': 'off',
'flowtype/semi': ['error', 'always'],
'flowtype/sort-keys': ['error', 'asc'],
'flowtype/space-after-type-colon': ['error', 'always'],
'flowtype/space-before-generic-bracket': ['error', 'never'],
'flowtype/space-before-type-colon': ['error', 'never'],
'flowtype/spread-exact-type': 'off',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 it's because spreading non-exact type is not safe and not properly supported by flow

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be possible to enable 🤔

'flowtype/type-id-match': 'off',
// Use identifier as declaration imports are not well supported by vscode
'flowtype/type-import-style': ['off', 'declaration'],
'flowtype/union-intersection-spacing': ['error', 'always'],
'flowtype/use-flow-type': 'warn',
},
Expand Down
45 changes: 30 additions & 15 deletions rules/imports.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,8 @@ module.exports = {
'import/no-duplicates': 'error',

// disallow namespace imports
// TODO: enable?
// https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-namespace.md
'import/no-namespace': 'off',
'import/no-namespace': 'error',

// Ensure consistent use of file extension within the import path
// https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/extensions.md
Expand All @@ -143,11 +142,30 @@ module.exports = {

// ensure absolute imports are above relative imports and that unassigned imports are ignored
// https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md
// TODO: enforce a stricter convention in module import order?
'import/order': [
'error', {
'groups': [['builtin', 'external', 'internal']],
'groups': [
['builtin'],
['external'],
['internal'],
['parent'],
['sibling'],
],
'newlines-between': 'never',
'alphabetize': {
order: 'asc',
caseInsensitive: true,
},
'pathGroups': [
{
pattern: '$*/**',
group: 'internal',
},
{
pattern: '$*',
group: 'internal',
},
],
},
],

Expand Down Expand Up @@ -195,33 +213,32 @@ module.exports = {
// Prevent unassigned imports
// https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-unassigned-import.md
// importing for side effects is perfectly acceptable, if you need side effects.
'import/no-unassigned-import': 'off',
'import/no-unassigned-import': 'error',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure will it affect index files that just imports and do nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will have to use eslint-disable but these files are bad because using side-effects. Best is to remove them as much as we can


// Prevent importing the default as if it were named
// https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-named-default.md
'import/no-named-default': 'error',

// Reports if a module's default export is unnamed
// https://github.com/benmosher/eslint-plugin-import/blob/d9b712ac7fd1fddc391f7b234827925c160d956f/docs/rules/no-anonymous-default-export.md
'import/no-anonymous-default-export': ['off', {
allowArray: false,
'import/no-anonymous-default-export': ['error', {
allowArray: true,
allowArrowFunction: false,
allowAnonymousClass: false,
allowAnonymousFunction: false,
allowLiteral: false,
allowObject: false,
allowLiteral: true,
allowObject: true,
}],

// This rule enforces that all exports are declared at the bottom of the file.
// https://github.com/benmosher/eslint-plugin-import/blob/98acd6afd04dcb6920b81330114e146dc8532ea4/docs/rules/exports-last.md
// TODO: enable?
'import/exports-last': 'off',
'import/exports-last': 'error',

// Reports when named exports are not grouped together in a single export declaration
// or when multiple assignments to CommonJS module.exports or exports object are present
// in a single file.
// https://github.com/benmosher/eslint-plugin-import/blob/44a038c06487964394b1e15b64f3bd34e5d40cde/docs/rules/group-exports.md
'import/group-exports': 'off',
'import/group-exports': 'error',
Comment on lines +235 to +241
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it when these two rules import/exports-last and import/group-exports are enabled together, we wouldn't have that much changes in terms of lines movement in our files?

Cause I'm not sure about import/exports-last as we do have this kind of patterns which will be flagged. And I guess is okay to have exports in the middle as sometimes we only export what we need to.

const bool = true
export default bool
const str = 'foo'

https://github.com/benmosher/eslint-plugin-import/blob/98acd6afd04dcb6920b81330114e146dc8532ea4/docs/rules/exports-last.md#this-will-be-reported

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 but it's easier when navigating. If you need to add export a function then it will be grouped in last statement like

const a = ...
 function b() {}

export { a, b };


// forbid default exports. this is a terrible rule, do not use it.
// https://github.com/benmosher/eslint-plugin-import/blob/44a038c06487964394b1e15b64f3bd34e5d40cde/docs/rules/no-default-export.md
Expand All @@ -237,7 +254,7 @@ module.exports = {

// Forbid cyclical dependencies between modules
// https://github.com/benmosher/eslint-plugin-import/blob/d81f48a2506182738409805f5272eff4d77c9348/docs/rules/no-cycle.md
'import/no-cycle': ['error', { maxDepth: Infinity }],
'import/no-cycle': ['error', { ignoreExternal: true, maxDepth: Infinity }],

// Ensures that there are no useless path segments
// https://github.com/benmosher/eslint-plugin-import/blob/ebafcbf59ec9f653b2ac2a0156ca3bcba0a7cf57/docs/rules/no-useless-path-segments.md
Expand All @@ -256,9 +273,7 @@ module.exports = {

// Reports modules without any exports, or with unused exports
// https://github.com/benmosher/eslint-plugin-import/blob/f63dd261809de6883b13b6b5b960e6d7f42a7813/docs/rules/no-unused-modules.md
// TODO: enable, semver-major
'import/no-unused-modules': ['off', {
ignoreExports: [],
missingExports: true,
unusedExports: true,
}],
Expand Down
38 changes: 0 additions & 38 deletions rules/ramda.js

This file was deleted.

19 changes: 12 additions & 7 deletions rules/react.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ module.exports = {
// https://github.com/yannickcr/eslint-plugin-react/blob/843d71a432baf0f01f598d7cf1eea75ad6896e4b/docs/rules/forbid-dom-props.md
'react/forbid-dom-props': ['off', { forbid: [] }],

// Enforce a specific function type for function components.
// https://github.com/yannickcr/eslint-plugin-react/blob/v7.20.0/docs/rules/function-component-definition.md
'react/function-component-definition': ['error', {
namedComponents: 'function-declaration',
unnamedComponents: 'function-expression',
}],

// Enforce boolean attributes notation in JSX
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-boolean-value.md
'react/jsx-boolean-value': ['error', 'never', { always: [] }],
Expand Down Expand Up @@ -82,7 +89,7 @@ module.exports = {

// Validate JSX has key prop when in array or iterator
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-key.md
'react/jsx-key': 'off',
'react/jsx-key': ['error', { checkFragmentShorthand: true }],

// Limit maximum of props on a single line in JSX
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-max-props-per-line.md
Expand Down Expand Up @@ -177,7 +184,7 @@ module.exports = {

// Prevent direct mutation of this.state
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-direct-mutation-state.md
'react/no-direct-mutation-state': 'off',
'react/no-direct-mutation-state': 'error',

// Prevent usage of isMounted
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-is-mounted.md
Expand Down Expand Up @@ -300,7 +307,7 @@ module.exports = {

// Enforce JSX indentation
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-indent.md
'react/jsx-indent': ['error', 2],
'react/jsx-indent': ['error', 2, { indentLogicalExpressions: true, checkAttributes: true }],

// Disallow target="_blank" on links
// https://github.com/yannickcr/eslint-plugin-react/blob/ac102885765be5ff37847a871f239c6703e1c7cc/docs/rules/jsx-no-target-blank.md
Expand Down Expand Up @@ -476,12 +483,10 @@ module.exports = {

// Enforce state initialization style
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/state-in-constructor.md
// TODO: set to "never" once babel-preset-airbnb supports public class fields
'react/state-in-constructor': ['off', 'never'],
'react/state-in-constructor': ['error', 'never'],

// Enforces where React component static properties should be positioned
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/static-property-placement.md
// TODO: set to "static public field" once babel-preset-airbnb supports public class fields
'react/static-property-placement': ['error', 'static public field'],

// Disallow JSX props spreading
Expand All @@ -494,7 +499,7 @@ module.exports = {

// Enforce that props are read-only
// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prefer-read-only-props.md
'react/prefer-read-only-props': 'off',
'react/prefer-read-only-props': 'error',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might lead to a lot of changes but I think is most likely simple fixes. Not sure if it would be too verbose though. 🤔

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 I'll enable them one-by-one to have separated commits

},

settings: {
Expand Down
Loading