Chore: Remove lodash#14287
Conversation
lodash.last(array) -> array[array.length - 1]
v = lodash.get(a, "b.c") -> if (a && a.b && a.b.c) v = a.b.c
lodash.noop -> () => {}
lodash.findLast(array) -> [...array].reverse().find(_ =>_)
lodash.isString(str) -> typeof str === "string";
Add the escape-string-regexp package
Add the fast-deep-equal package
Add the deep-extend package
Add the clone package
Add the omit package
Add the upper-case-first package
Add the fast-memoize package
Add the map-values package
|
Hi @stephenwade!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
Read more about contributing to ESLint here |
|
I'll take a look at the CI failure tomorrow. Looks like I missed a .flat call |
| const rangeIndex = [...this.lineStartIndices].reverse().findIndex(el => index >= el); | ||
| const lineNumber = rangeIndex === -1 ? 0 : this.lineStartIndices.length - rangeIndex; |
There was a problem hiding this comment.
| const rangeIndex = [...this.lineStartIndices].reverse().findIndex(el => index >= el); | |
| const lineNumber = rangeIndex === -1 ? 0 : this.lineStartIndices.length - rangeIndex; | |
| const lineNumber = index >= this.lineStartIndices[this.lineStartIndices.length - 1] | |
| ? this.lineStartIndices.length | |
| : this.lineStartIndices.findIndex(el => index < el); |
This way, we can avoid an intermediary array and reverse().
Also, lodash.sortedLastIndex was a binary search? If so, then replacing it with findIndex would affect performance, though I believe getLocFromIndex is mostly used for calculating error locations, in which case the performance might not be that important.
There was a problem hiding this comment.
findIndex is slower, but not exponentially so, and both operations can run millions of times per second.
$ node benchmark-getLocFromIndex.js
sortedLastIndex x 10,168,021 ops/sec ±1.34% (90 runs sampled)
Array#findIndex x 2,795,631 ops/sec ±1.41% (89 runs sampled)
Benchmark code: https://github.com/stephenwade/my-eslint-benchmark
There was a problem hiding this comment.
See #14287 (comment) for project-wide benchmarking results. I believe the speed difference in this function will not be noticeable.
| return lodash.sortedIndexBy( | ||
| tokens, | ||
| { range: [location] }, | ||
| getStartLocation | ||
| ); | ||
| const val = getStartLocation({ range: [location] }); | ||
| const index = tokens.findIndex(el => val <= getStartLocation(el)); | ||
|
|
||
| return index === -1 ? tokens.length : index; |
There was a problem hiding this comment.
Similar to the previous comment, lodash.sortedIndexBy most likely uses a binary search (and jsdoc description for this function search says "Binary-searches..."), while findIndex performs a linear search.
I think utils.search is used only on arrays of comments, which typically shouldn't be large arrays, so the difference might be small.
@eslint/eslint-tsc thoughts about this, should we look for a binary search library?
There was a problem hiding this comment.
I also think the difference might be small.
By the way, getStartLocation({ range: [location] }) is { range: [location] }.range[0], so it does nothing.
There was a problem hiding this comment.
Useless val declaration removed in 414766e.
There was a problem hiding this comment.
findIndex is slower, but not exponentially so, and both operations can run millions of times per second.
$ node benchmark-search.js
sortedIndexBy x 7,188,306 ops/sec ±0.84% (89 runs sampled)
Array#findIndex x 2,764,162 ops/sec ±1.65% (84 runs sampled)
Benchmark code: https://github.com/stephenwade/my-eslint-benchmark
There was a problem hiding this comment.
I used hyperfine to benchmark the test suite on this branch and the master branch. This branch ran slightly faster on my computer, although the results were very close.
I believe that the speed difference in this function will not be noticeable and that we should stick with findIndex.
00ac183 to
414766e
Compare
e4d94ce to
bf74251
Compare
mdjermanovic
left a comment
There was a problem hiding this comment.
LGTM, thanks for the great work!
|
Thanks for contributing! |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Reduce dependencies
What changes did you make? (Give an overview)
Addresses part of #14098. Removes lodash entirely from the project, replacing all uses with native code, custom code, and a handful of other tiny modules.
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_modulesIs there anything you'd like reviewers to focus on?
There is one fairly major change in here. I replaced lodash.template by creating a JS file in place of each template. Let me know if I should take a different approach with that or if I should split it out into a separate PR.