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 lodash deps #13057

Merged
merged 10 commits into from Mar 27, 2021
Merged

Remove lodash deps #13057

merged 10 commits into from Mar 27, 2021

Conversation

hzoo
Copy link
Member

@hzoo hzoo commented Mar 26, 2021

Inlining the function in most cases

Q                       A
Fixed Issues? Closes #11726, ref #13021
Any Dependency Changes? Lodash
License MIT

sourceType: "script",
babelrc: false,
inputSourceMap: task.inputSourceMap || undefined,
...opts,
Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't seem like we'd need to deep merge any of the options above so should be equivalent (or are all primitives)

@@ -23,7 +22,9 @@ export default function builder<T extends t.Node>(

let arg;
if (i < countArgs) arg = args[i];
if (arg === undefined) arg = loClone(field.default);
if (arg === undefined) {
arg = Array.isArray(field.default) ? [] : field.default;
Copy link
Member Author

Choose a reason for hiding this comment

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

in my search, all the default options are primitives except for a default of empty array

types

Copy link
Member

Choose a reason for hiding this comment

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

We should throw if we see a non-empty array or an object, so that we don't accidentally break this assumption in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah right, https://lodash.com/docs/#clone seems complicated, maybe we need to check what we do allow instead then given it could be anything?

so string/boolean/null/[]

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can just throw if Array.isArray(def) ? def.length > 0 : def && typeof def === "object"

Copy link
Member Author

Choose a reason for hiding this comment

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

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 26, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 26, 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 c34d89c:

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

@@ -33,6 +32,10 @@ export default valueToNode as {
(value: unknown): t.Expression;
};

function isRegExp(value): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function isRegExp(value): boolean {
function isRegExp(value): value is RegExp {

Gulpfile.mjs Outdated
@@ -162,7 +162,9 @@ function generateStandalone() {
let allList = "";

for (const plugin of pluginConfig) {
const camelPlugin = camelCase(plugin);
const camelPlugin = plugin.replace(/-([a-z])/g, c =>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const camelPlugin = plugin.replace(/-([a-z])/g, c =>
const camelPlugin = plugin.replace(/-[a-z]/g, c =>

I don't think we need the capturing group here

@@ -20,7 +20,6 @@
"@babel/helper-fixtures": "workspace:^7.13.10",
"babel-check-duplicated-nodes": "^1.0.0",
"escape-string-regexp": "condition:BABEL_8_BREAKING ? ^4.0.0 : ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"escape-string-regexp": "condition:BABEL_8_BREAKING ? ^4.0.0 : ",

Can remove this as well now since you removed the file that uses it

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch thanks!!

"use strict";

module.exports = process.env.BABEL_8_BREAKING
? require("escape-string-regexp")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Mar 26, 2021
Gulpfile.mjs Outdated Show resolved Hide resolved
@@ -244,7 +246,7 @@ fs.readdirSync(fixtureLoc).forEach(function (binName) {

delete taskOpts.os;
}
merge(opts, taskOpts);
opts = { args: [], ...taskOpts };
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this merge too (the only thing replaced is args, which is always set in our tests given it's babel-cli)

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks!

I love how "inline some deps" results in "more deleted lines than added lines" 😂

@nicolo-ribaudo nicolo-ribaudo merged commit 6b39baf into babel:main Mar 27, 2021
@julienw
Copy link
Contributor

julienw commented Apr 12, 2021

hey @hzoo @nicolo-ribaudo, is it expected that last version of @babel/cli still depends on lodash ? See https://github.com/babel/babel/blob/main/packages/babel-cli/package.json#L31 :-)
The CHANGELOG file mentions that it's removed from @babel/cli too, hence my question.

Thanks!

@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 Jul 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2021
@hzoo hzoo deleted the remove-deps branch March 3, 2022 04:09
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@babel/core - Get rid of (only 4 instances) lodash to fix security issue
6 participants