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

A better eslint rules config #16118

Open
wants to merge 43 commits into
base: develop
Choose a base branch
from
Open
Changes from 24 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
8e9b627
Update .eslintrc.yaml
finscn Aug 28, 2023
8f00748
Update .eslintrc.yaml
finscn Aug 30, 2023
19062a9
Update .eslintrc.yaml
finscn Aug 30, 2023
bb4f9a7
Update .eslintrc.yaml
finscn Aug 30, 2023
f3ace46
Update .eslintrc.yaml
finscn Aug 30, 2023
82990e7
Update .eslintrc.yaml
finscn Aug 30, 2023
733ffbe
Update .eslintrc.yaml
finscn Aug 31, 2023
98ccad5
Update .eslintrc.yaml
finscn Aug 31, 2023
8588453
Update .eslintrc.yaml
finscn Aug 31, 2023
8c9d6c2
Update .eslintrc.yaml
finscn Aug 31, 2023
4de06af
Update .eslintrc.yaml
finscn Aug 31, 2023
95d1189
Update .eslintrc.yaml
finscn Aug 31, 2023
98f8db7
Update .eslintrc.yaml
finscn Aug 31, 2023
ef7eec4
Update .eslintrc.yaml
finscn Aug 31, 2023
10a90ba
Update .eslintrc.yaml
finscn Aug 31, 2023
f1505bf
Update .eslintrc.yaml
finscn Sep 12, 2023
c0b808d
Update .eslintrc.yaml
finscn Sep 12, 2023
9d8771d
Update .eslintrc.yaml
finscn Sep 25, 2023
61499c6
Update .eslintrc.yaml
finscn Sep 25, 2023
dbf5ccd
Update .eslintrc.yaml
finscn Sep 25, 2023
eecf7d6
Update .eslintrc.yaml
finscn Oct 8, 2023
e5931f8
use `@typescript-eslint/indent`
finscn Oct 27, 2023
c5aafa7
Update .eslintrc.yaml
finscn Oct 27, 2023
e38fab6
停用 prefer-template
finscn Oct 29, 2023
236490a
更新 ignoredNodes
finscn Nov 6, 2023
2755afb
新增 member-delimiter-style
finscn Nov 6, 2023
2149fd4
Update .eslintrc.yaml
finscn Nov 13, 2023
8f679b8
Update .eslintrc.yaml
finscn Nov 13, 2023
e198953
使用 @stylistic
finscn Nov 24, 2023
7a08d6f
关闭 no-prototype-builtins
finscn Nov 24, 2023
fc77ff6
完善 comma-dangle 在ts里的设定
finscn Nov 26, 2023
2c1a46f
完善规则
finscn Nov 27, 2023
689a260
补充 no-case-declarations: error
finscn Nov 27, 2023
c514ea6
Update .eslintrc.yaml
finscn Nov 27, 2023
d4caf75
Update .eslintrc.yaml
finscn Nov 27, 2023
2caf27c
Update .eslintrc.yaml
finscn Nov 28, 2023
858fd37
Update .eslintrc.yaml
finscn Nov 28, 2023
2365ce0
Update .eslintrc.yaml
finscn Nov 28, 2023
f5f9fe5
新增 comma-spacing 规则
finscn Nov 28, 2023
50fcc3e
完善一些关于空格的规则
finscn Dec 1, 2023
366e8ea
Update .eslintrc.yaml
finscn Dec 1, 2023
ddaaa5e
新增 function-call-spacing
finscn Dec 1, 2023
31ce9b2
关闭 no-inner-declarations 规则
finscn Dec 20, 2023
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
120 changes: 107 additions & 13 deletions .eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ extends:
- plugin:@typescript-eslint/recommended
- plugin:@typescript-eslint/recommended-requiring-type-checking

plugins: ["@typescript-eslint"]
plugins: ['@typescript-eslint']

settings:
import/resolver:
Expand Down Expand Up @@ -48,9 +48,8 @@ rules:
##### RECOMMENDED RULE OVERRIDES #####

camelcase: off # underscores may come in handy in some cases
eqeqeq: warn # important check missing from recommended

keyword-spacing: warn # we require this explicitly
no-multi-spaces: off # useful for manually align some expression across lines
prefer-rest-params: off # we need ES5 to be fast too
prefer-spread: off # we need ES5 to be fast too
space-before-function-paren: [warn, always] # we require this explicitly
Expand All @@ -72,22 +71,23 @@ rules:
import/no-unresolved: off # TODO: fix internal modules
import/prefer-default-export: off # prefer named exports
indent: [error, 4, {
SwitchCase: 0
}]
'@typescript-eslint/indent': [error, 4, {
SwitchCase: 0,
ignoredNodes: [ # https://stackoverflow.com/a/72897089
"FunctionExpression > .params[decorators.length > 0]",
"FunctionExpression > .params > :matches(Decorator, :not(:first-child))",
"ClassBody.body > PropertyDefinition[decorators.length > 0] > .key",
'FunctionExpression > .params[decorators.length > 0]',
'FunctionExpression > .params > :matches(Decorator, :not(:first-child))',
'ClassBody.body > PropertyDefinition[decorators.length > 0] > .key',
]
}]

lines-between-class-members: off # be more lenient on member declarations
max-classes-per-file: off # helper classes are common
max-len: [warn, 150] # more lenient on max length per line
no-console: error # prefer the uniform logging methods

no-plusplus: off # allow increment/decrement operators
no-continue: off # allow unlabeled continues
no-mixed-operators: off # this is just cumbersome
no-multi-assign: off # it is handy sometimes

no-nested-ternary: off # it is handy sometimes
no-param-reassign: off # the output object is passed as parameters all the time
no-restricted-syntax: off # for-in is a efficient choice for plain objects
Expand All @@ -112,7 +112,6 @@ rules:
'ts-nocheck': true,
'ts-check': false,
}]
'@typescript-eslint/no-unused-expressions': warn

# TODO: this is just too much work
'@typescript-eslint/explicit-module-boundary-types': off
Expand Down Expand Up @@ -148,8 +147,6 @@ rules:
allowBoolean: true
}]

'@typescript-eslint/type-annotation-spacing': warn

# We choose to use style `... as foo` since it's more common.
# In the other hand, angle bracket style can be ambiguous with j/tsx syntax.
# For example, https://babeljs.io/docs/en/babel-plugin-transform-typescript#istsx
Expand All @@ -163,3 +160,100 @@ rules:
'@typescript-eslint/explicit-function-return-type': [error, {
allowIIFEs: true, # IIFEs are widely used, writing their signature twice is painful
}]


# constructor-super: error
# curly: [error, all]
# no-debugger: error
# no-duplicate-case: error
# no-eval: error
# no-sparse-arrays: error
# no-trailing-spaces: error
# no-var: error
# prefer-const: error
# prefer-numeric-literals: off

prefer-template: off
Copy link
Contributor

Choose a reason for hiding this comment

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

Why to disable prefer-template?

Copy link
Contributor Author

@finscn finscn Nov 6, 2023

Choose a reason for hiding this comment

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

字符串模板性能更低, 开启该功能会强制使用字符串模板, 即使简单的一次拼接(如 "Hello " + user.name) 也会被转换成 'Hello ${user.name}'. 得不偿失.

'@typescript-eslint/no-unsafe-argument': warn
Copy link
Contributor

Choose a reason for hiding this comment

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

This change the default error to warn. Any reasons for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@finscn finscn Nov 6, 2023

Choose a reason for hiding this comment

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

因为按照目前的cocos代码, 如果是 error, 那么会大量标红.如果cocos愿意大规模重构代码, error当然是更好的


max-len: [warn, 250] # more lenient on max length per line
no-console: warn # prefer the uniform logging methods
Copy link
Contributor

Choose a reason for hiding this comment

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

Code in engine should not use console.log/warn/error/info directly. error is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

理论上是这样, 但是 console.log 无法避免. 无论是开发者的代码 还是引擎代码. 总是会在某些时候需要在控制台打印一些信息. 即使生产环境也是如此.

no-multi-spaces: [warn, {
'ignoreEOLComments': true,
'exceptions': {
'Property': true,
'VariableDeclarator': false
}
}]
no-multiple-empty-lines: [warn, {max: 2, maxEOF: 1}]
no-multi-assign: [error, { 'ignoreNonDeclaration': true }]
max-statements-per-line: [error, { max: 1 }]

comma-dangle: [error, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you choose to disallow trailing commas? I think it'd better to allow. For example, if you had the following array:

const array = [
  a,
  b
];

In this time, you want to append a new item, the diff might be look like:

- b
+b,
+c

In compare with the allowing case, the diff would be:

  b,
+c,

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've changed the config. I think you are right.

'arrays': always-multiline,
'objects': always-multiline,
'imports': never,
'exports': never,
'functions': never
}]
quote-props: [error, consistent-as-needed]
Copy link
Contributor

@dumganhar dumganhar Oct 30, 2023

Choose a reason for hiding this comment

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

This config will make

const object1 = {
    foo: 'bar',
    baz: 42,
    'qux-lorem': true, // string with `-` could not be a key without quote.
};

to be changed to

const object1 = {
    'foo': 'bar', // quote ' was added for `foo` property
    'baz': 42,  // quote ' was added for `baz` property
    'qux-lorem': true,
};

So I think keep the default behavior is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

在实际代码中, 一致性也是很重要的. 在一个json/object的代码块中, key的形式应该尽量保证一致.
而且在现代编辑器中 有没有引号 会导致 key的颜色不同. 如下图:

image

如果实际代码中出现这种 key的颜色不一致的情况, 其实是对阅读代码产生干扰的.

function-paren-newline: off
Copy link
Contributor

@dumganhar dumganhar Oct 30, 2023

Choose a reason for hiding this comment

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

Its default value is multiline which i think it's better.

If we disable this config, the following code

function foo (
    bar, baz
) {}

const foo1 = function (
    bar, baz
) {};

const foo2 = (
    bar, baz
) => {};

will not be formated to

function foo (bar, baz) {}

const foo1 = function (bar, baz) {};

const foo2 = (bar, baz) => {};

Copy link
Contributor Author

@finscn finscn Nov 6, 2023

Choose a reason for hiding this comment

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

单行 function 就和 单行 if 单行 for 一样, 除了 "节省行数" 并没有任何其他好处. 反而破坏代码的一致性. 而且会造成一定的阅读困难.
要不要把函数写成一行 应该是开发者根据函数复杂度和代码长度自己决定.
打开这个选项, 会出现很多 "虽然变为了单行, 但是单行长度很长" 的情况. 并不便于代码的阅读.

我个人完全无法接受 单行函数, 单行if 单行for .

space-unary-ops: error
Copy link
Contributor

@dumganhar dumganhar Oct 30, 2023

Choose a reason for hiding this comment

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

This rule space-unary-ops: error is enabled by default. Why do we have to set it explicitly?

Copy link
Contributor Author

@finscn finscn Nov 6, 2023

Choose a reason for hiding this comment

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

实际测试下来 , space-unary-ops 并不是error, 可能是被继承的配置篡改了.

extends:
    - eslint:recommended
    - airbnb-base

其实我建议不要继承这些所谓的 "已有的最佳实践", 这样的话 eslint的配置就是黑盒的了.
可以去看看 eslint:recommended 和 airbnb-base里是怎么设置的 适合cocos的就copy过来. 不适合的就删除或者改掉.

Copy link
Contributor

@dumganhar dumganhar Nov 6, 2023

Choose a reason for hiding this comment

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

I agree with this.

key-spacing: [error, {
'singleLine': {
'beforeColon': false,
'afterColon': true
},
'multiLine': {
'beforeColon': false,
'afterColon': true,
'align': colon
}
}]
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule is not good for our code. If we add a longer property in object, all previous properties will change.
For instance, if we get

const obj = {
    hello: 'world',
    a    : 'bbb',
};

If we want to add a new property called foooooooo like:

const obj = {
    hello    : 'world',  // code changed
    a        : 'bbb',      // code changed
    foooooooo: 'ahaha',
};

We just added a new property, but the whole object definition code were changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

嗯 这个我可以理解.按你说的问题不大.


func-names: off
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value of func-names is warn. Why do we need to disable the warning?
Anonymous functions will hard for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

打开这个开关, 对于 arr.forEach( (item, index) => {}) 还有很多 callback/ hook 函数 也要求写名字 太讨厌了.

default-param-last: off
Copy link
Contributor

Choose a reason for hiding this comment

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

https://eslint.org/docs/latest/rules/default-param-last

It's better to put default argument to the last. Why should we disable this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

在实际开发中我经常有类似:
function   foo(a:number , b:number =1, c?:string) { ... } 这样的需求.
就是 可选参数在最后一个. 强制给可选参数赋默认值, 会破坏原有逻辑. 不传参传默认值 是完全不同的概念

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

one-var: [error, never]
eqeqeq: [error, always, {'null': ignore}]
arrow-body-style: [error, as-needed]
arrow-parens: [error, always]
brace-style: [error, 1tbs]
id-denylist: [error, any, unknown, Number, number, Boolean, boolean, String, Undefined]
id-match: off
no-cond-assign: off
no-constant-condition: off
nonblock-statement-body-position: [error, below]
padded-blocks: [error, {
'classes': always
}]

'@typescript-eslint/no-this-alias': [off]
'@typescript-eslint/no-extra-parens': [error, all, {
'conditionalAssign': false,
'returnAssign': false,
'nestedBinaryExpressions': false,
'enforceForArrowConditionals': false,
'enforceForFunctionPrototypeMethods': false,
'ignoreJSX': all
}]
'@typescript-eslint/no-unused-expressions': [error, {
'allowShortCircuit': true,
'allowTernary': true,
'allowTaggedTemplates': false
}]
'@typescript-eslint/no-duplicate-type-constituents': [error, {
'ignoreIntersections': false,
'ignoreUnions': true
}]
'@typescript-eslint/keyword-spacing': [error]
'@typescript-eslint/space-infix-ops': [error]
'@typescript-eslint/type-annotation-spacing': [error, {
'before': false,
'after': true,
'overrides' : {
'arrow': {
'before': true,
'after': true,
}
}
}]
Loading