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
Chore: Remove lodash #14287
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 |
lib/source-code/source-code.js
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion committed in cba69f2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #14287 (comment) for project-wide benchmarking results. I believe the speed difference in this function will not be noticeable.
lib/source-code/token-store/utils.js
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless val
declaration removed in 414766e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Is 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.