-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
build: don't store eslint locally #53974
Conversation
Review requested:
|
5735f34
to
48182db
Compare
Note that all linters passed in this PR, meaning that eslint is still working the exact same :-) ref: https://github.com/nodejs/node/actions/runs/10022363992/job/27702051106?pr=53974 |
48182db
to
7ed6cf0
Compare
@nodejs/build is this ok for you? |
iOS app and Safari crashes whenever I look into pull-requests like this. Please separate the change into multiple commits and add
Where do we see this change? |
7ed6cf0
to
5a655e1
Compare
5a655e1
to
d5bb0a2
Compare
I've split the PR into
@dependabot will, every month, check |
0c010bb
to
cbd78e2
Compare
cbd78e2
to
d62e15b
Compare
as long as this doesn't break build, huge +1 to getting this out of core |
It doesn't AFAICT. Additionally, CI passing backs this up. |
Closing in favor of #54231 |
This PR removes
eslint
as a local dependency and switches to downloading it fromnpm
. It will still be updated regularly via @dependabot.Why?
eslint
is the only linter stored locally (besidescpplint
, which has patches, unlike ESLint).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 isn't a huge much, but it's a decent amount.