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

[TS MVP] Update ESLint configuration #11719

Closed
arkflpc opened this issue May 6, 2022 · 8 comments
Closed

[TS MVP] Update ESLint configuration #11719

arkflpc opened this issue May 6, 2022 · 8 comments
Assignees
Labels
domain:ts squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@arkflpc
Copy link
Contributor

arkflpc commented May 6, 2022

We need ESLint helping us with TypeScript code as well as with JavaScript.

Enable ESLint for *.ts files and verify that:

  •  yarn run lint checks them,
  • git commit checks them,
  • there is any support in IDE (like VS Code).

We should keep all existing rules for JavaScript that have been working for us, but we need to decide about TypeScript-specific ones.

Part of #11708.

@arkflpc arkflpc added squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos". domain:ts and removed type:task This issue reports a chore (non-production change) and other types of "todos". labels May 6, 2022
@Reinmar
Copy link
Member

Reinmar commented May 9, 2022

Potentially, we'll need to release a new packages.

We have https://www.npmjs.com/package/eslint-config-ckeditor5 and https://www.npmjs.com/package/eslint-plugin-ckeditor5-rules already. E.g. the latter package contains our custom rules – we may need to port or extend these rules so they can be applied to TS code.

It's also unclear where we should define the TS-specific rules. Can it be done in eslint-config-ckeditor5? Or should we introduce a new package?

@Reinmar
Copy link
Member

Reinmar commented May 9, 2022

So, there seems to be three steps:

  • Agree (team-wide) on TS-specific rules to be enabled.
  • Write them down in the configs and ensure that our setup (yarn lint, precommit hooks, etc) execute them.
  • Update the existing TS code (let's assume this is only ckeditor5-utils at this stage) to adhere to these new rules.

@Reinmar
Copy link
Member

Reinmar commented May 9, 2022

Potential risk: We defined our ESLint rules ~5 years ago. It may happen that we use some deprecated rules that will not work with TS. Thus, we may need to review more of our rules, than just the ones related to TS.

@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label May 18, 2022
@arkflpc arkflpc self-assigned this May 19, 2022
@arkflpc
Copy link
Contributor Author

arkflpc commented May 25, 2022

I'm starting the discussion here, and this is my proposal.

Let's start with all the recommended rules, but:

Rule My recommendation Comment
ban-types Don't ban Function I used the Function type in a few places in #11755. Although, I don't insist on keeping it there.
no-explicit-any Off It's not going to work in our code-base (good for projects written from scratch in TypeScript).
no-inferrable-types {  "ignoreParameters": true } It looks odd when all the parameters of some function have types, but not the default ones).
no-non-null-assertion Off We have plenty of them. I don't think it's feasible to add guards for undefined everywhere. That can add not-testable branches that lower code coverage.
array-type "array" or "array-simple" Let's be consistent weather we want number[] or Array<number>. I know we use the latter in JsDocs, but two non-trivial array arguments in single function can easily make the signature longer than maximum line length.
consistent-indexed-object-style Enable There was a discussion about Record<string, T> vs { [ x: string ]: T }. Let's pick something (I vote for Record) and be consistent.
consistent-type-assertions Enable with { assertionStyle: 'as', objectLiteralTypeAssertions: 'allow-as-parameter' } Just for consistency.
explicit-module-boundary-types Enable with { allowArgumentsExplicitlyTypedAsAny: true } Let's require typing for all exported stuff. I'd allow any in parameters. Otherwise, we'd be forced to use unknown type in functions. It would be the same from the outside, but would require unnecessary casting inside a function if we want dynamic code inside (we do).
member-delimiter-style Enable Just for consistency.
no-confusing-non-null-assertion Enable Prevents very confusing syntax with ! operator.
type-annotation-spacing Enable Just for consistency with every code found on the Internet.

I would omit rules that require type information, because it would require building the whole project. And I feel it's going to require some very magical configuration to make it working. Apart from checks being slower, of course.

@arkflpc
Copy link
Contributor Author

arkflpc commented May 27, 2022

@arkflpc
Copy link
Contributor Author

arkflpc commented May 27, 2022

"lines-around-comment" does not respect TypeScript-specific things like interfaces. We're going to need an empty line after opening an interface:

interface X {

	/** A member */
	doIt(): void;
}

See typescript-eslint/typescript-eslint#1150.

@CatStrategist
Copy link
Contributor

CatStrategist commented May 30, 2022

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/explicit-member-accessibility.md

I would like to propose above rule with 'explicit' option. It removes implicit knowledge that properties not marked with access modifier are public and may avoid us issues with unnecessary exposing fields.

Also I think it looks tidier :)

class Animal {
    public name: string;
    public age: number;

    protected herd: Herd;
    protected familyID: string;

    private city: string;
    private origin: Origin;
    private loveInterest: Animal;
}
class Animal {
    name: string;
    age: number;

    protected herd: Herd;
    protected familyID: string;

    private city: string;
    private origin: Origin;
    private loveInterest: Animal;
}

@arkflpc
Copy link
Contributor Author

arkflpc commented Jun 7, 2022

Decisions so far from internal disputes:

  • array-type - off
  • consistent-type-imports - on
  • explicit-member-accessibility - on, except for constructors
  • no-empty-interface - off
  • no-inferrable-types - off
  • no-non-null-asserted-nullish-coalescing - on
  • no-non-null-assertion - off
  • parameter-properties - on
  • unified-signatures - on
  • no-empty-function - off

@arkflpc arkflpc closed this as completed Jun 8, 2022
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jun 8, 2022
@CKEditorBot CKEditorBot added this to the upcoming milestone Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ts squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

4 participants