Skip to content
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

Merged
merged 43 commits into from May 21, 2021
Merged

Chore: Remove lodash #14287

merged 43 commits into from May 21, 2021

Conversation

@stephenwade
Copy link
Contributor

@stephenwade stephenwade commented Apr 2, 2021

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_modules

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.

stephenwade added 27 commits Apr 2, 2021
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
@eslint-github-bot
Copy link

@eslint-github-bot eslint-github-bot bot commented Apr 2, 2021

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.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

Loading

@stephenwade stephenwade changed the title Remove lodash Chore: Remove lodash Apr 2, 2021
@stephenwade
Copy link
Contributor Author

@stephenwade stephenwade commented Apr 2, 2021

I'll take a look at the CI failure tomorrow. Looks like I missed a .flat call

Loading

package.json Outdated Show resolved Hide resolved
Loading
@stephenwade stephenwade requested a review from mdjermanovic May 8, 2021
const rangeIndex = [...this.lineStartIndices].reverse().findIndex(el => index >= el);
const lineNumber = rangeIndex === -1 ? 0 : this.lineStartIndices.length - rangeIndex;
Copy link
Member

@mdjermanovic mdjermanovic May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Loading

Copy link
Contributor Author

@stephenwade stephenwade May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion committed in cba69f2.

Loading

Copy link
Contributor Author

@stephenwade stephenwade May 20, 2021

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

Loading

Copy link
Contributor Author

@stephenwade stephenwade May 20, 2021

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.

Loading

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;
Copy link
Member

@mdjermanovic mdjermanovic May 13, 2021

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?

Loading

Copy link
Member

@mysticatea mysticatea May 13, 2021

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.

Loading

Copy link
Contributor Author

@stephenwade stephenwade May 20, 2021

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.

Loading

Copy link
Contributor Author

@stephenwade stephenwade May 20, 2021

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

Loading

Copy link
Contributor Author

@stephenwade stephenwade May 20, 2021

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.

Screenshot of benchmark results running the npm test command. The remove-lodash-3 branch runs in about 172 seconds and the master branch runs in about 174 seconds.

I believe that the speed difference in this function will not be noticeable and that we should stick with findIndex.

Loading

lib/cli-engine/formatters/html-template-message.js Outdated Show resolved Hide resolved
Loading
lib/cli-engine/formatters/html.js Outdated Show resolved Hide resolved
Loading
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks for the great work!

Loading

@mdjermanovic mdjermanovic merged commit c0f418e into eslint:master May 21, 2021
12 checks passed
Loading
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented May 21, 2021

Thanks for contributing!

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants