build: don't store eslint locally

[avivkeller]

This PR removes eslint as a local dependency and switches to downloading it from npm. It will still be updated regularly via @dependabot.

Why?

1. eslint is the only linter stored locally (besides cpplint, which has patches, unlike ESLint).
2. There are multiple issues related to file paths being too long and tarballs failing to lint properly (see: Missing ./tools/node_modules/eslint/eslint.js on make test? #39709, make -s test-doc fails with Error: Cannot find module '/node-v14.4.0/tools/doc/../node_modules/eslint/node_modules/js-yaml' #34005, Erbium: test regression in tarball #32765, and others).

Why was eslint set up like this initially?

When eslint was first added to the repository in #1539 around 10 years, it replaced a (not as large, but still large) library, closure-lint. Since then, the library has almost 4x its size. At the time, there was no discussion (AFAICT) about keeping or removing the directory—it was simply included as it was.

Size-wise, this'll save ~40MB, which is a decent amount.

---

(I'll add more fixes as I find more issues)
Fixes#39709
Fixes#54231

[nodejs-github-bot]

Review requested:

• @nodejs/actions
• @nodejs/security-wg
• @nodejs/tsc

[avivkeller]

CC @nodejs/build @nodejs/linting

[avivkeller]

Reviewers, please review the commit '(review changes)'. The first commit removes the node_modules directory and may be hard to review in the github UI.

[avivkeller]

Everything is passing! The failure is because of #54229. The original PR didn't see any objections, is this good to land in the coming days?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (1bcdba2) to head (6b95bab).
Report is 289 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #54231 +/- ##
==========================================
+ Coverage 87.10% 87.33% +0.23% 
==========================================
Files 647 648 +1 Lines 181693 182321 +628 Branches 34869 34972 +103 ==========================================
+ Hits 158256 159225 +969 + Misses 16733 16371 -362 - Partials 6704 6725 +21 

see 90 files with indirect coverage changes

[bnb]

I'm +1 to this approach, but feel like there's probably folks who have more domain knowledge about this who should actually be the ones to approve it.

[silverwind]

I don't think there was any specific reason why those linters were checked into the repo besides maybe offline functionality, but I think that's likely no longer needed.

[avivkeller]

Great! I'm glad y'all are on board! Seeing as the linting is passing (except for the unrelated issue), all looks good in terms of land-ability.

Although, I assume this needs a CI to land.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60949/

[avivkeller]

CI is passing. 🎉
Linting is passing except for an unrelated failured that was patched in 7327e44.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60955/

[aduh95]

Everytime I run e.g. make lint-js on this branch, it runs npm. Can you make it sure npm is run only when there were changes in the package-lock.json?

[silverwind]

Everytime I run e.g. make lint-js on this branch, it runs npm. Can you make it sure npm is run only when there were changes in the package-lock.json?

FWIW, it's possible to achieve on-demand node_modules installation with Make by tracking the folder timestamp, but it's not a 100% thing as Make timestamp tracking works best with files. Here is how I do it in my projects:

node_modules: package-lock.json
npm install --no-save
@touch node_modules

[avivkeller]

FWIW, it's possible to achieve on-demand node_modules installation with Make by tracking the folder timestamp, but it's not a 100% thing as Make timestamp tracking works best with files. Here is how I do it in my projects:

node_modules: package-lock.json
npm install --no-save
@touch node_modules

I have it setup like:

tools/.jslintstamp:
cd tools/eslint &&$(call available-node,$(run-npm-ci))
@touch $@

If the user wants to re-build the lint, they have to run make lint-clean.

[avivkeller]

Hey, I'd love to get some reviews if there aren't any more objections. I don't want to pressure anyone tho.

[avivkeller]

Any more changes needed?

[avivkeller]

Thanks for approving! Does anyone else have any more feedback? It's been a while since any reviews.

[avivkeller]

No objections in a week, is this author ready?

[aduh95]

node/doc/contributing/collaborator-guide.md

Lines 900 to 908 in 27f1306

	*`author-ready` - A pull request is _author ready_ when:
	* There is a CI run in progress or completed.
	* There is at least one collaborator approval (or two TSC approvals for
	semver-major pull requests).
	* There are no outstanding review comments.
	Please always add the `author ready` label to pull requests that qualify.
	Please always remove it again as soon as the conditions are not met anymore,
	such as if the CI run fails or a new outstanding review comment is posted.

so this is not author ready because there are no recent Jenkins CI run. If you start one, it would be.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61796/

[avivkeller]

Per @aduh95's comment:

• There is a CI run in progress or completed. 🟢
• There is at least one collaborator approval.
• There are no outstanding review comments.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61882/

[avivkeller]

The MacOS CI appears to have failed to start, could someone restart it?

As for the linked linux OpenSSL one, see #54737

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61895/

[avivkeller]

FWIW CI is 🟢 (Yay!)

[nodejs-github-bot]

Landed in 437e168