Support Eslint 7 & add rules #15

moroine · 2020-05-19T07:25:17Z

This PR ~~is WIP~~, plugins not supporting yet eslint 7:

eslint-plugin-ramda is not really active, maybe we will need to remove it
~~eslint-plugin-jsx-a11y: PR opened~~
~~eslint-plugin-import: Waiting release~~

I've also added lot of rules, idk if they're all needed or if they will lead to too much changes! I haven't tried them yet, let's review them together here.

Related PR:

https://github.com/nugit/nugit-visualization/pull/2388

limtingzhi · 2020-06-02T07:55:21Z

rules/flow.js

    'flowtype/no-mixed': 'off',
    'flowtype/no-dupe-keys': 'error',
+    // TODO: enable?
+    'flowtype/no-existential-type': 'off',


I think can be enabled since * is deprecated. But I guess we would have to change * to any.

limtingzhi · 2020-06-02T08:01:23Z

rules/flow.js

+    'flowtype/no-flow-fix-me-comments': 'off',
+    'flowtype/no-mixed': 'off',
+    // TODO: enable?
+    'flowtype/no-mutable-array': 'off',


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

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

limtingzhi · 2020-06-02T08:04:54Z

rules/flow.js

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


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

limtingzhi · 2020-06-02T08:19:13Z

rules/flow.js

    'flowtype/require-return-type': 'off',
+    'flowtype/require-types-at-top': 'error',


This might lead to quite a number of changes, need to test it out. Some of our files have this kind of pattern, which we could either move to different files else the types would be difficult to see.

Type A Function A Type B Function B

https://github.com/gajus/eslint-plugin-flowtype/blob/master/.README/rules/require-types-at-top.md

Yeah, would lead to lot of changes but this has autofix I think

Yeah, I think so too. But some files might need to reorganise, should be a few I guess.

limtingzhi · 2020-06-02T08:23:25Z

rules/flow.js

    'flowtype/space-after-type-colon': ['error', 'always'],
    'flowtype/space-before-generic-bracket': ['error', 'never'],
    'flowtype/space-before-type-colon': ['error', 'never'],
+    // TODO: enable?
+    'flowtype/spread-exact-type': 'off',


Not sure about this 🤔

https://github.com/gajus/eslint-plugin-flowtype/blob/master/tests/rules/assertions/spreadExactType.js

I think it's because spreading non-exact type is not safe and not properly supported by flow

Might be possible to enable 🤔

limtingzhi · 2020-06-02T08:26:10Z

rules/flow.js

    'flowtype/type-id-match': 'off',
+    // Use identifier as declaration imports are not well supported by vscode
+    'flowtype/type-import-style': ['error', 'identifier'],


Prefer this to be off. As at times, there is not a need to have identifier style when importing types from a type file. Less verbose too.

https://github.com/gajus/eslint-plugin-flowtype/blob/master/.README/rules/type-import-style.md

What about forcing declaration style? It might help separating type from non type 🤔

Sure, we can force declaration style. Would prefer that to identifier style if you want to enforce one of them.

limtingzhi · 2020-06-02T08:35:58Z

rules/imports.js

+        'alphabetize': {
+          'order': 'asc',
+          'caseInsensitive': true,
+        }


❤️ 😂 🙏 . Though I'm not sure how it will work out with aliasing 🤔

Let's see 😄

limtingzhi · 2020-06-02T08:41:01Z

rules/imports.js

@@ -195,33 +204,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',


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

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

limtingzhi · 2020-06-02T08:53:56Z

rules/imports.js

+    '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',


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

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

limtingzhi · 2020-06-02T14:01:15Z

rules/react.js

+    // 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-declaration',
+    }],


I'm not sure about this. I prefer to write this way inside a function component, and this would probably prevent it 🤔

var Component = (props) => { return <div />; };

Also, unnamedComponents should be function-expression instead. There is no function-declaration for it.

This was mainly related to https://twitter.com/dan_abramov/status/1255229440860262400?s=09

Hmmms, true 🤔 But means inside class, we would also have to not use arrows then. We can try it out and see how many changes etc.

No it's just on functions, class arrow functions will be good

limtingzhi · 2020-06-02T14:44:49Z

rules/react.js

@@ -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',


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

I think I'll enable them one-by-one to have separated commits

limtingzhi · 2020-06-02T14:49:42Z

@moroine I guess we probably need to test them out to further understand what will be the changes needed. I commented on the ones that I think might have issues with.

If eslint-plugin-ramda is not really actively supported, we can drop it for now. But it seems like it is working with ESLint 7: ramda/eslint-plugin-ramda#28

moroine · 2020-06-12T07:43:23Z

@moroine I guess we probably need to test them out to further understand what will be the changes needed. I commented on the ones that I think might have issues with.

@limtingzhi I agree, I'll tests theses changes one-by-one in order to test them out

If eslint-plugin-ramda is not really actively supported, we can drop it for now. But it seems like it is working with ESLint 7: ramda/eslint-plugin-ramda#28

Yeah I think we can remove them

limtingzhi · 2020-10-06T02:16:56Z

CHANGELOG.md

@@ -3,32 +3,34 @@
 ## Changes



Missing breaking changes eslint to ^7.10.0.

limtingzhi · 2020-10-06T02:17:24Z

CHANGELOG.md

+- **[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.0`


eslint-plugin-import is suppose to be ^2.22.1 and not ^2.22.0.

limtingzhi · 2020-10-06T02:19:46Z

CHANGELOG.md

+- upgrade `eslint-plugin-import` to `^2.22.0`
+- upgrade `eslint-plugin-jsx-a11y` to `^6.3.1`
+- upgrade `eslint-plugin-react` to `^7.21.2`
+- upgrade `eslint-plugin-react-hooks` to `^4.1.2`


eslint-plugin-react-hooks is written above as breaking changes. Could you remove this one and update the breaking changes to ^4.1.2?

limtingzhi · 2020-10-06T02:43:56Z

rules/imports.js

@@ -146,8 +145,18 @@ module.exports = {
    // TODO: enforce a stricter convention in module import order?


Do we need to remove this TODO?

moroine added 10 commits May 19, 2020 07:13

upgrade eslint-plugin-react-hooks

abd5b3d

Fix dependency list in README

e0e33d4

upgrade eslint-plugin-flowtype

d44cec5

upgrade eslint-plugin-react

0ada918

set require-atomic-updates: error

90568b4

Add flowtype rules

2f3fa09

add import rules

3d4e63b

enable no-undefined

81e002a

add style rules

eb56be7

Add react rules

5ba8acb

moroine requested a review from limtingzhi May 19, 2020 07:25

moroine changed the base branch from master to develop May 19, 2020 07:25

limtingzhi reviewed Jun 2, 2020

View reviewed changes

moroine added 14 commits September 29, 2020 09:11

remove eslint-plugin-ramda

de32f93

upgrade dependencies

cc03c02

Set minimum NodeJs version

1ed29e2

disable sort-keys

108abf0

fix lint

cd4f9af

Enable 'flowtype/no-existential-type'

12689b6

Enable flowtype/no-mutable-array

3f4c2c1

enable flowtype/spread-exact-type

d353749

Update gitignore

3c7c575

v1.0.0-alpha.1

a46b862

fix react/function-component-definition rule

2206a9b

v1.0.0-alpha.3

405fcae

disable rules without autofix

a223833

disable flowtype/require-types-at-top

2424c1c

moroine added 3 commits September 30, 2020 17:15

v1.0.0-alpha.4

fb718af

remove flowtype/require-compound-type-alias

2427366

v1.0.0-alpha.5

833c392

moroine changed the title ~~[WIP] Support Eslint 7 & add rules~~ Support Eslint 7 & add rules Oct 1, 2020

moroine requested a review from limtingzhi October 1, 2020 08:14

limtingzhi reviewed Oct 6, 2020

View reviewed changes

moroine added 6 commits October 6, 2020 09:28

feat: consider import starting with $ to be internal

cd6f942

v1.0.0-alpha.6

cc49768

chore: update changelog

eeb2040

feat: react/jsx-indent check logicExpressions & attributes

ef3bf96

v1.0.0-alpha.7

1e102c1

chore: update Changelog

70bdcdd

moroine requested a review from limtingzhi October 7, 2020 07:40

moroine added 2 commits October 7, 2020 11:06

fix: internal path pattern

00b2789

v1.0.0-alpha.8

8f4b52c

limtingzhi approved these changes Oct 8, 2020

View reviewed changes

moroine merged commit 351a8ef into develop Oct 8, 2020

moroine deleted the improvment/eslint-7 branch October 8, 2020 05:41

limtingzhi mentioned this pull request Nov 24, 2020

v1.0.0 Release #25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Eslint 7 & add rules #15

Support Eslint 7 & add rules #15

moroine commented May 19, 2020 •

edited

limtingzhi Jun 2, 2020

limtingzhi Jun 2, 2020

moroine Jun 12, 2020

limtingzhi Jun 2, 2020

limtingzhi Jun 2, 2020

moroine Jun 12, 2020

limtingzhi Jun 12, 2020

limtingzhi Jun 2, 2020

moroine Jun 12, 2020

limtingzhi Jun 12, 2020

limtingzhi Jun 2, 2020

moroine Sep 29, 2020

limtingzhi Sep 29, 2020

limtingzhi Jun 2, 2020

moroine Jun 12, 2020

limtingzhi Jun 2, 2020

moroine Jun 12, 2020

limtingzhi Jun 2, 2020

moroine Jun 12, 2020

limtingzhi Jun 2, 2020 •

edited

moroine Jun 12, 2020

limtingzhi Jun 12, 2020

moroine Jun 12, 2020

limtingzhi Jun 2, 2020

moroine Jun 12, 2020

limtingzhi commented Jun 2, 2020 •

edited

moroine commented Jun 12, 2020

limtingzhi Oct 6, 2020

limtingzhi Oct 6, 2020

limtingzhi Oct 6, 2020

limtingzhi Oct 6, 2020

		'flowtype/require-return-type': 'off',
		'flowtype/require-types-at-top': 'error',

		@@ -146,8 +145,18 @@ module.exports = {
		// TODO: enforce a stricter convention in module import order?

Support Eslint 7 & add rules #15

Support Eslint 7 & add rules #15

Conversation

moroine commented May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limtingzhi Jun 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limtingzhi commented Jun 2, 2020 • edited

moroine commented Jun 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moroine commented May 19, 2020 •

edited

limtingzhi Jun 2, 2020 •

edited

limtingzhi commented Jun 2, 2020 •

edited