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

Remove remaining lodash dependencies #13139

Merged
merged 6 commits into from Apr 13, 2021

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues? Fixes #13134
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

We were only using a very small number of functions from lodash, so we can avoid depending on it.

This PR still uses an external clone-deep implementation, but it has 10M weekly downloads (so it's battle-tested) and it's about 60% smaller than lodash/cloneDeep.

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Apr 12, 2021
@@ -1,4 +1,4 @@
import last from "lodash/last"
import last from "lo-dash/last"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just an input/output fixture, but it was annoying when grepping for lodash 😂

Copy link
Member

Choose a reason for hiding this comment

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

lol we want to ctrl+f and find no results

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 12, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit dc0b4bd:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 12, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45155/

// to ignore the log message rather than making @babel/cli crash.
return;
}
const logSuccess = util.debounce(function () {
Copy link
Member

@hzoo hzoo Apr 12, 2021

Choose a reason for hiding this comment

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

oh great! I wasn't sure about replacing in the earlier PR because of the trailing: true and also logSuccess.flush(); on line 173, but looks like it covers it!

@@ -42,7 +42,7 @@ function compile(code, filename) {
// sourceRoot can be overwritten
{
sourceRoot: path.dirname(filename) + path.sep,
...deepClone(transformOpts),
...cloneDeep(transformOpts),
Copy link
Member

Choose a reason for hiding this comment

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

And the reason we probably want to keep this is because transformOpts might have like things like functions vs. just objects given how people configure @babel/register with js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, also people can pass any JS value as plugin options.

@@ -40,7 +40,7 @@
"eslint-rule-composer": "^0.3.0"
},
"devDependencies": {
"eslint": "^7.5.0",
"lodash": "^4.17.20"
"clone-deep": "^4.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

clone-deep is quite bloated package. Maybe try klona?

Copy link
Member Author

Choose a reason for hiding this comment

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

My strategy for choosing the clone-deep package was "Ctrl+F for clone in our yarn.lock file", so thanks for suggesting an alternative 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh it looks like klona requires Node.js >= 8, so until the Babel 8 release we cannot use it.

@nicolo-ribaudo nicolo-ribaudo merged commit d24bd7c into babel:main Apr 13, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the remove-lodash branch April 13, 2021 20:41
@jayaddison
Copy link
Contributor

@nicolo-ribaudo Nice :) Does this mean that lodash can be removed from the base directory's package.json, or is that kept in place for stability/other reasons?

@nicolo-ribaudo
Copy link
Member Author

The base package.json is used for internal scripts (it's not published), probably we still use lodash in them.

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 19, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories PR: New Dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trim down install size by 5 mb by dropping lodash
6 participants