lint: add eslint based on config-semistandard

[rubiagatra]

Closes #1063

PR approach

• Using https://github.com/standard/eslint-config-standard#usage for eslint installation

Todo

• Suggestion is eslint with semistandard
• Only apply to files that are changed
• run apply as part of adding, list steps to generate lint changes
• have second person do run to make sure we don't have unintended changes

[rubiagatra]

Hello, @mhdawson need your help for review this,

thank you for your guidance 😀

[mhdawson]

@rubiagatra I'm not sure that gets us semitstandard.

These two files might be a good example:

https://github.com/pkgjs/support/blob/main/.eslintrc.json
https://github.com/pkgjs/support/blob/main/package.json (the script target pretest call eslint) and the devDependencies as the list of what was require for that.

[mhdawson]

What you have in the PR in the following line is probably better than the example as I can see how it tries to run only on what is changed, but I think we'd want it to report issues versus just fixing them (which is what I assume the --fix option is for). Maybe we want 2 targets one to report issues which is run as part of building/testing and checking in, and a separate one which can be used to run the --fix targer.

"lint:js": "eslint --fix $(git diff --name-only HEAD '**/*.js' | xargs)",

Also to clarify as part of " run apply as part of adding, list steps to generate lint changes" we want to fix all files so the linter won't complain and the validate that in the following step so that after we finish this issue we have all code complying with the linter.

[rubiagatra]

thank you for your feedback! @mhdawson

recently I updated for some changes

1. Using semistandard
2. I applied 2 targets for pretest and lint (before commit)

the workflow

• after someone adds some commit on js files will automatically check on pre-commit
• if they want to fix it just call npm run lint:fix
• for build/ci/pretest I added without --fix argument

I already using pretest on my local it resulted

✖ 1460 problems (1446 errors, 14 warnings)
  1330 errors and 12 warnings potentially fixable with the `--fix` option.

• Do I need to change it based on the linter? there are some kinds of logic changes because of the use of var instead of const and some unused escape character
• Do I need to separate pretest with pretest:js? the test on ci is failing right now

[mhdawson]

@rubiagatra, I think the pre-test should use the same logic as the other targets so that it only runs on files that have changed.

In terms of running the linter and adding the changed files I'm thinking we should run the --fix target and let it fix what can be automatically fixed and add that to this PR as a separate commit.

For the const/let versus var then doing those in followup PRs would make sense as for this first step we were thinking we'd just include changes which were purely mechanical. Since linter will only run on changed files it should be ok to only fix a subset of the reported issues.

[rubiagatra]

I explain the latest changes @mhdawson

• on github action

with:
   fetch-depth: 0

so we can compare the remote main branch with the current PR commit, reference https://github.com/actions/checkout#fetch-all-history-for-all-tags-and-branches

also, add npm run pretest:js and npm run lint:js checking lint for only changed file compare to the main branch

• package json

"pretest": "node-gyp rebuild -C test",
"pretest:js": "eslint $(git diff --name-only refs/remotes/origin/main HEAD '**/*.js' | xargs)",

"lint:js": "eslint $(git diff --name-only refs/remotes/origin/main HEAD '**/*.js' | xargs)",
"lint:fix": "node tools/clang-format --fix && eslint --fix $(git diff --cached --name-only '**/*.js' | xargs)",
"pre-commit:lint": "npm run lint:fix"

as your request for pre-commit on the local machine, it will run lint:fix but for ci / build /testing it will compare to the main branch only the changed js file.

thanks!

[mhdawson]

@rubiagatra I pushed a commit with a few tweaks, I think to make it a bit more consistent with what we did on the c++ side. It runs the linter before running the tests and you have to run the fix target specifically to have it fix automatically for you.

I think that with that commit it likely no longer needs the changes to the ci related files (and probably explains why they fail). Could you take a look and see if you agree and if so remove the changes to the ci reils?

[mhdawson]

@rubiagatra I also noticed that for the C formatting, if the are upstaged files with lint issues it will warn:

[midawson@midawson node-addon-api]$ npm run lint:fix

> node-addon-api@4.1.0 lint:fix
> node tools/clang-format --fix && eslint --fix $(git diff --cached --name-only '**/*.js' | xargs)

Error running git-clang-format: The following files would be modified but have unstaged changes:
M	test/function_reference.cc
Please commit, stage, or stash them first.

I wonder if we can get the same behaviour for js files?

[rubiagatra]

I run several commits to understand how the ci works, I think these are the takeaways from it.

1. I am not changing the GitHub action as you request, I think your tweak is enough.

2. We are not run the Eslint on ci.yaml and ci-win.yaml actions because git diff --name-only refs/remotes/origin/main '**/*.js' | xargs is not working on PowerShell (xargs not supported), for checking out the lint we could only use lint.yml action for checking. I am moving back pretest only run node-gyp rebuild -C test

3. If Eslint wants some behavior like tools/clang-format.js we could create tools/eslint-format.js (Maybe on the follow-up issue / PR). For now, lint:fix will fix out the diff on unstaged/staged files eslint --fix $(git diff --cached --name-only '**/*.js' | xargs && git diff --name-only '**/*.js' | xargs)

I think that's the scope of this PR, what do you think @mhdawson ? (All the checks also passed right now)

[mhdawson]

@rubiagatra I think its now in a good shape.

I don't think it covers "run apply as part of adding, list steps to generate lint changes" so I unchecked that but I think we could do that in a separate PR. It would be running the fix and committing those changes but that might make good smaller PR's versus one bigger one anyway since in some cases it needs changes beyond what the fix can resolve.

As you say improving so that we get a better message from eslint could be in a follow-on PR.

[mhdawson]

LGTM