Migrate to eslint

[Jiannan-dev]

@kezhenxu94 hey,this part is already done, I only added a few rules to the eslint configuration (don't worry, it contains important file header rules) because I just started contributing and I'm not sure which rules you need

[Jiannan-dev]

Will CI still execute this line? Trying to fix this would result in a lot of file changes, as many of the files now don't conform to the eslint configuration

[Jiannan-dev]

When I run `npm run lint` I get 273 errors, directly using `npm run lint:fix` fixes these directly, but review is a pain in the ass I'm afraid

[kezhenxu94]

When I run npm run lint I get 273 errors, directly using npm run lint:fix fixes these directly, but review is a pain in the ass I'm afraid

Can you try and see how many lines changed, for this kind of changes we don't usually review.

If some of the errors are because of default rules, we can add some rules according to let our current codes to override the default ones.

Anyway I'll look into this and evaluate the preferable methods. Thanks for doing this. 🙇

[kezhenxu94]

When I run npm run lint I get 273 errors, directly using npm run lint:fix fixes these directly, but review is a pain in the ass I'm afraid

Can you try and see how many lines changed, for this kind of changes we don't usually review.

If some of the errors are because of default rules, we can add some rules according to let our current codes to override the default ones.

Anyway I'll look into this and evaluate the preferable methods. Thanks for doing this. 🙇

[Jiannan-dev]

@kezhenxu94 I turned off individual rules,git diff shows that 366 lines were changed

[Jiannan-dev]

But there is another one that needs to be handled manually because of the duplicate declaration of SkyWalkingEventEmitter

[kezhenxu94]

@kezhenxu94 I turned off individual rules,git diff shows that 366 lines were changed

What do you mean "turned off individual rules", after that how many rules and what are enabled?

[Jiannan-dev]

Two rules are turned off, one for method parameters not being used, and the other for regular no-useless-escape,the only manually enabled rule is file-header, the rest are eslint:recommend enabled rules

This is the modified rule config

module.exports = {
  extends: ["eslint:recommended", "plugin:prettier/recommended"],
  plugins: ["header"],
  parser: "@typescript-eslint/parser",
  parserOptions: {
    sourceType: "module"
  },
  rules: {
    "header/header": ["error", ".file-headerrc"],
    "no-useless-escape": "off",
    "no-unused-vars": [
      "error",
      // we are only using this rule to check for unused arguments since TS
      // catches unused variables but not args.
      { varsIgnorePattern: ".*", args: "none" }
    ]
  },
  env: {
    node: true
  }
};

[kezhenxu94]

But there is another one that needs to be handled manually because of the duplicate declaration of SkyWalkingEventEmitter

Let's rename class name SkyWalkingEventEmitter to SkyWalkingEventEmitterImpl.

[kezhenxu94]

@kezhenxu94 I turned off individual rules,git diff shows that 366 lines were changed

This is acceptable to me

[kezhenxu94]

Can you adjust the rules to minimize the changes, if we can change 366 lines to pass the check, we can do that first

[Jiannan-dev]

CI failure seems to be due to the node version,strange, the CI error only occurs on node@10, on node>12 this does not cause problems

[kezhenxu94]

CI failure seems to be due to the node version,strange, the CI error only occurs on node@10, on node>12 this does not cause problems

Is it because the linter dropped support of node 10?

[Jiannan-dev]

Looks like it's because eslint@8 No longer supported node@10 , there are two options

1. Remove node@10 tests from the github CI script
2. Downgrade to eslint@7 to support node@10 testing

Also, Happy Chinese New Year!

[kezhenxu94]

Looks like it's because eslint@8 No longer supported node@10 , there are two options

1. Remove node@10 tests from the github CI script

2. Downgrade to eslint@7 to support node@10 testing

Also, Happy Chinese New Year!

Thanks for looking into this, we can remove the lint command in CI for node 10, we can even only run lint in node 14 as the lint result doesn't rely on node version.

Can you try to add an if condition in that CI file? You can refer to https://github.com/apache/skywalking/blob/c548b00517a5f305d3a98dbdfdc81525aa82f0f7/.github/workflows/ci-it.yaml#L82 to see the if condition syntax in GitHub workflow file.

Happy Chinese New Year!! @ruleeeer 🎉

[kezhenxu94]

Thank you @ruleeeer very much!