Reduce disk churn for ESLint upgrades

[TimvdLippe]

The version of ESLint you are using.
7.19.0 and earlier upgrades to, for example, 7.13.0

The problem you want to solve.
ESLint upgrades cause significant file changes in node_modules. We have hit this problem multiple times, some statistics:

• 7.14.0 -> 7.19.0 (5 minor versions)

3796 files changed, 162528 insertions(+), 5876 deletions(-)

https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2692306

• 6.8.0 -> 7.13.0 (1 major version, 13 minor versions)

8913 files changed, 125261 insertions (+), 241831 deletions (-)

https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2534873

• 6.0.1 -> 6.8.0 (7 minor versions)

634 files changed, 47325 insertions (+), 9376 deletions (-)

https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2137384

As an effect of these ESLint upgrades, checking out repositories becomes a lot slower and disk churn increases significantly for all contributors and bots.

Please note that we are required to check in our node_modules for security and stability reasons. We are not allowed to make calls to external repositories in our testing infrastructure, which means we have to include the files in our repository. That said, even if you wouldn't check in your files, the effects on files changed and disk operations remain. E.g. if you gitignore your node_modules, ESLint upgrades would still cause significant disk churn.

Your take on the correct solution to problem.
Reduce the disk churn either by dropping dependencies that cause excessive disk usage and drop dependencies that are commonly duplicated in node_modules trees. For example, in nearly all of our upgrades, we are observing multiple copies of lodash in among others @eslint/eslintrc or AST-like packages like es-abstract.

Are you willing to submit a pull request to implement this change?
I don't think this is something I can do as an external contributor, but feel free to led me/the community know if there are dependencies that could be pruned and usages can be migrated to reduce overall disk usage.

[nzakas]

Thanks for the heads up. I’m not sure there’s much we can do at this point — ESLint is pretty complicated and does require a bunch of dependencies. If you have specific ideas about how dependencies that could be replaced, we will happily take a look, but it’s unlikely the core team will have time to do a full investigation of each dependency to try to find alternatives.

[mdjermanovic]

drop dependencies that are commonly duplicated in node_modules trees. For example, in nearly all of our upgrades, we are observing multiple copies of lodash in among others @eslint/eslintrc or AST-like packages like es-abstract.

We can't assume that lodash will be in every user's node_modules, so it has to be declared as ESLint's dependency. The dependency requirement in ESLint's package.json has been updated to ^4.17.20, as advised by https://snyk.io/vuln/SNYK-JS-LODASH-590103.

As for the lodash duplicates in eslint/node_modules, @eslint/eslintrc/node_modules, and under other installed packages, the current version of lodash is 4.17.20, which is most likely in-range for all lodash-dependent packages you use. Installing v4.17.20 as the top-level node_modules/lodash would probably avoid the duplication you have in the node_modules tree, but packages can't really control that, It's up to the package manager tools and the upgrade process to avoid duplicates when it's possible (maybe running npm dedupe after the dependency updates can help).

[TimvdLippe]

@mdjermanovic Thank you for your response. I have ran npm dedupe on our repository and it did indeed remove numerous duplicate packages: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2703961 Sadly, most of these were @babel duplications and it was not able to remove (for example) the duplicate lodash dependency.

I took a look at @eslint/eslintrc and discovered that there was only a single usage of 1 function of lodash. Therefore, I was able to replace the usage of lodash.memoize with a simple Set in eslint/eslintrc#29

@nzakas Based on the above PR, I am assuming that there are other similar opportunities that could reduce the amount of dependencies ESLint uses. Would you be interested in such PRs, to eventually reduce the amount of files present in a node_modules as a result of the ESLint dependencies?

[nzakas]

@TimvdLippe absolutely. If you find any opportunities to reduce dependencies, please feel free to send a PR and reference this issue.

[nzakas]

Oops, fat-fingered the close button.

[mdjermanovic]

I have ran npm dedupe on our repository and it did indeed remove numerous duplicate packages: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2703961 Sadly, most of these were @babel duplications and it was not able to remove (for example) the duplicate lodash dependency.

In your package-lock.json, requirements for lodash are ^4.17.13, ^4.17.15, ^4.17.19, or ^4.17.20, so the version 4.17.20 satisfies all.

You currently have 5 instances installed: 4.17.19 at the top level, and four 4.17.20 below.

I think that a deduplication tool should be able to move 4.17.20 to the top (i.e. to replace 4.17.19 there) and then remove duplicates. Which npm version did you use to run dedupe? Maybe a newer version of npm or even running the same version again could be able to fix this.

[TimvdLippe]

I have run npm dedupe again and yes, it was able to remove some more @babel packages, but sadly still not lodash: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2705383 This run was performed with NPM version 6.14.10.

Then I updated to NPM 7.4.5 and I can confirm that reduces a couple more packages: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2705386/ Thanks for the guidance on that!

I still think it is worthwhile to audit the current dependencies and see if there are any quick wins. For those users who are not aware of npm dedupe or end up with a suboptimal node_modules layout, removing a large package with 2 lines of code would still be valuable imo.

[TimvdLippe]

I took a look at the top 5 packages in a node_modules after running npm i eslint:

$ du -sh node_modules/* | sort -h:

1.0M	node_modules/esquery
1.1M	node_modules/ajv
1.2M	node_modules/acorn
2.5M	node_modules/table
3.8M	node_modules/eslint
4.8M	node_modules/lodash

lodash has been removed from eslintrc, but is still extensively used by eslint itself. I am not sure what would be possible here, other than removing every single usage of lodash and replacing it with ES6+ idiomatic code. That seems like a longterm project and will only provide benefits if all usages are removed. As long as at least 1 usage is present, you will have to keep the dependency.

eslint is of course this package, so that is somewhat expected.

table is interesting, as it appears to be used solely in the table formatter:

eslint/lib/cli-engine/formatters/table.js

Line 12 in b8aea99

table = require("table").table;

If you were to remove this dependency, the size of node_modules would reduce from 21080 KB to 18348 KB (e.g. ~13% reduction in size). I was able to find 299 references on GitHub for this formatter: https://github.com/search?l=JSON&q=%22--format+table%22&type=Code In contrast, --format checkstyle has 877 references (https://github.com/search?q=%22--format+checkstyle%22+eslint&type=Code) and --format codeframe has 3796 references (https://github.com/search?q=%22--format+codeframe%22+eslint&type=Code). And the default formatter is of course stylish.

One option would be to deprecate --format table in favor of disk savings, but I will leave that up to the maintainers to make that judgement call. I can understand if you would prefer to keep the formatter, but I think there would also be a case for removing the formatter, given the significant (10+%) impact on the overall disk usage of ESLint when installing. Another option would be to ship the formatter as a separate package that users would be able to install, if they specifically want this formatter.

The other packages (acorn, ajv and esquery) all seem to be part of core functionality of ESLint and I fear little can be done about that.

[btmills]

Another way of looking at this is seeing which dependencies are least-used and might be easiest to remove:

• 29 lodash
  • bin/eslint.js
  • lib/cli-engine/file-enumerator.js
  • lib/cli-engine/formatters/html.js
  • lib/init/autoconfig.js
  • lib/linter/apply-disable-directives.js
  • lib/linter/linter.js
  • lib/linter/node-event-generator.js
  • lib/rule-tester/rule-tester.js
  • lib/rules/comma-dangle.js
  • lib/rules/complexity.js
  • lib/rules/consistent-return.js
  • lib/rules/eol-last.js
  • lib/rules/indent.js
  • lib/rules/lines-around-comment.js
  • lib/rules/max-lines-per-function.js
  • lib/rules/max-lines.js
  • lib/rules/max-params.js
  • lib/rules/max-statements.js
  • lib/rules/no-fallthrough.js
  • lib/rules/no-useless-backreference.js
  • lib/rules/no-useless-computed-key.js
  • lib/rules/no-warning-comments.js
  • lib/rules/object-curly-newline.js
  • lib/rules/spaced-comment.js
  • lib/rules/utils/ast-utils.js
  • lib/shared/deprecation-warnings.js
  • lib/shared/runtime-info.js
  • lib/source-code/source-code.js
  • lib/source-code/token-store/utils.js
• 17 eslint-utils
  • lib/rules/no-extra-boolean-cast.js
  • lib/rules/no-extra-parens.js
  • lib/rules/no-implied-eval.js
  • lib/rules/no-import-assign.js
  • lib/rules/no-misleading-character-class.js
  • lib/rules/no-obj-calls.js
  • lib/rules/no-promise-executor-return.js
  • lib/rules/no-setter-return.js
  • lib/rules/no-useless-backreference.js
  • lib/rules/prefer-exponentiation-operator.js
  • lib/rules/prefer-named-capture-group.js
  • lib/rules/prefer-object-spread.js
  • lib/rules/prefer-regex-literals.js
  • lib/rules/require-unicode-regexp.js
  • lib/rules/wrap-iife.js
  • lib/source-code/source-code.js
  • lib/source-code/token-store/index.js
• 14 debug
  • bin/eslint.js
  • lib/cli-engine/cli-engine.js
  • lib/cli-engine/file-enumerator.js
  • lib/cli.js
  • lib/init/autoconfig.js
  • lib/init/config-file.js
  • lib/init/config-initializer.js
  • lib/init/source-code-utils.js
  • lib/linter/code-path-analysis/debug-helpers.js
  • lib/linter/config-comment-parser.js
  • lib/linter/linter.js
  • lib/linter/source-code-fixer.js
  • lib/rules/utils/lazy-loading-rule-map.js
  • lib/shared/traverser.js
• 8 @eslint/eslintrc
  • lib/cli-engine/cli-engine.js
  • lib/cli-engine/file-enumerator.js
  • lib/eslint/eslint.js
  • lib/init/autoconfig.js
  • lib/init/config-initializer.js
  • lib/linter/config-comment-parser.js
  • lib/linter/linter.js
  • lib/shared/config-validator.js
• 6 regexpp
  • lib/rules/no-control-regex.js
  • lib/rules/no-invalid-regexp.js
  • lib/rules/no-misleading-character-class.js
  • lib/rules/no-regex-spaces.js
  • lib/rules/no-useless-backreference.js
  • lib/rules/prefer-named-capture-group.js
• 4 espree
  • lib/init/config-initializer.js
  • lib/linter/linter.js
  • lib/rules/quote-props.js
  • lib/rules/utils/ast-utils.js
• 3 chalk
  • lib/cli-engine/formatters/codeframe.js
  • lib/cli-engine/formatters/stylish.js
  • lib/cli-engine/formatters/table.js
• 2 js-yaml
  • lib/cli-engine/formatters/tap.js
  • lib/init/config-file.js
• 2 eslint-visitor-keys
  • lib/linter/linter.js
  • lib/shared/traverser.js
• 2 cross-spawn
  • lib/init/npm-utils.js
  • lib/shared/runtime-info.js
• 2 json-stable-stringify-without-jsonify
  • lib/cli-engine/lint-result-cache.js
  • lib/init/config-file.js
• 2 esutils
  • lib/rules/func-name-matching.js
  • lib/rules/utils/ast-utils.js
• 2 ignore
  • lib/rules/no-restricted-imports.js
  • lib/rules/no-restricted-modules.js
• 1 strip-ansi
  • lib/cli-engine/formatters/stylish.js
• 1 file-entry-cache
  • lib/cli-engine/lint-result-cache.js
• 1 semver
  • lib/init/config-initializer.js
• 1 progress
  • lib/init/config-initializer.js
• 1 v8-compile-cache
  • bin/eslint.js
• 1 glob-parent
  • lib/cli-engine/file-enumerator.js
• 1 is-glob
  • lib/cli-engine/file-enumerator.js
• 1 minimatch
  • lib/cli-engine/file-enumerator.js
• 1 @babel/code-frame
  • lib/cli-engine/formatters/codeframe.js
• 1 text-table
  • lib/cli-engine/formatters/stylish.js
• 1 table
  • lib/cli-engine/formatters/table.js
• 1 imurmurhash
  • lib/cli-engine/hash.js
• 1 enquirer
  • lib/init/config-initializer.js
• 1 levn
  • lib/linter/config-comment-parser.js
• 1 eslint-scope
  • lib/linter/linter.js
• 1 esquery
  • lib/linter/node-event-generator.js
• 1 optionator
  • lib/options.js
• 1 functional-red-black-tree
  • lib/rules/indent.js
• 1 globals
  • lib/rules/no-extend-native.js
• 1 natural-compare
  • lib/rules/sort-keys.js
• 1 doctrine
  • lib/rules/valid-jsdoc.js
• 1 ajv
  • lib/shared/ajv.js

[mdjermanovic]

table has the new ajv version 7 in node_modules/table/node_modules/ajv, I think it takes most of the node_modules/table size.

Once we update ESLint to use ajv version 7 (#13888), it should replace node_modules/ajv (which is currently v6, as required by ESLint), so the copy under table becomes unnecessary and dedupe should be able to remove it.

[TimvdLippe]

@mdjermanovic Good catch! Yes that would be good to upgrade ajv. It seems like there is a WIP PR up at #13911 to do so and is pending a decision on the backwards compatibility question? (Based on my initial reading it is backwards compatible for ESLint concerned, but probably good to verify)

[mdjermanovic]

It's most likely we'll upgrade ajv, but that change needs RFC due to backward compatibility concerns for ESLint plugins.

[stephenwade]

I am not sure what would be possible here, other than removing every single usage of lodash and replacing it with ES6+ idiomatic code.

@mdjermanovic @nzakas Would you be open to a PR (or probably multiple PRs) to remove uses of lodash? I'm interested in trying to accomplish this.

[nzakas]

Issue suggesting we remove a couple of formatters: #14277

It's worth noting that we have discussed (eslint/rfcs#58) moving the --init command into a separate package, which would allow us to remove a bunch of dependencies as well. If anyone wants to take up that RFC, that would be another way to continue this effort.

[fregante]

If anyone wants to take up that RFC

What does that entail? A whole new repo and npm package? I might be able to extract that, but I can't directly maintain them in the future (like for the formatters), so after the extraction and publishing I'd assign repo and npm name back to you.

[TimvdLippe]

@nzakas Thanks for the update! Especially after removing the built-in formatters and potentially the init command, we will soon be reaching marginal incremental returns. At that point, I think the usage of lodash and ajv would be the last points to work on, but it might be good to call this done after that. I think it is best if we avoid a situation of "perpetual improvement" for the sake of reducing disk usage, while the gains are limited. My expectation is that with these last couple of changes, ESLint should be in a good enough spot to no longer be among the top of biggest size contributors in node_modules, which is the ideal end result.

[stephenwade]

table just released an update to remove lodash, so I'll be coming back to this soon with another PR to remove other uses of lodash.

[nzakas]

@fregante that’s correct. We need to finalize the design in an RFC, but after that, we would setup a separate repo and npm package. We could create the repo under the eslint org, you could fork it, then do a PR with the initial commit. That way, we could get all our required licensing and files and not worry about ownership transfer.

@TimvdLippe I agree. I think we are well on our way with lodash. I’m not sure there’s much we can do about ajv, as that is how our configs are validated. Definitely open to ideas, though.

@stephenwade nice! Thanks for keeping an eye on that.

[stephenwade]

I've submitted #14287 to update table and remove lodash.

node_modules size comparison:

~/git/eslint-test-master$ echo '{ "dependencies": { "eslint": "eslint/eslint" } }' > package.json
~/git/eslint-test-master$ npm install >/dev/null
~/git/eslint-test-master$ du -sh node_modules
22M	node_modules

~/git/eslint-test-stephenwade$ echo '{ "dependencies": { "eslint": "stephenwade/eslint#remove-lodash-3" } }' > package.json
~/git/eslint-test-stephenwade$ npm install >/dev/null
~/git/eslint-test-stephenwade$ du -sh node_modules
17M	node_modules

~/git$ diff -U0 <(cd eslint-test-master; du -hd1 node_modules) <(cd eslint-test-stephenwade; du -hd1 node_modules) | grep "^[-+]\d"
-4.9M	node_modules/lodash
-16K	node_modules/escape-string-regexp
+20K	node_modules/escape-string-regexp
+28K	node_modules/deep-extend
-240K	node_modules/@babel
+256K	node_modules/@babel
-22M	node_modules
+17M	node_modules

[fregante]

@stephenwade table will be removed by #14316

[stephenwade]

@fregante That's great! This PR could still be merged into a minor release though, so I'm going to leave the table update in.

[fregante]

Updating to a patch release doesn't change anything for the end user and just causes my PR to have conflicts 😃

[stephenwade]

Updating to a patch release is important for my PR because it updates table to a version that does not depend on lodash. That's why I'm leaving it in.

[TimvdLippe]

Good news! Removing Lodash removed 44k lines in NodeJS: nodejs/node#38764

[nzakas]

Nice. Great work, everyone.

[github-actions]

Oops! It looks like we lost track of this issue. @eslint/eslint-tsc what do we want to do here? This issue will auto-close in 7 days without an update.

[nzakas]

Looks like we have completed everything for this issue now, so closing. Thanks everyone for making ESLint a bit slimmer.