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

ui: remove unnecessary uses of lodash in database pages #78203

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Mar 21, 2022

Closes #68820

This commit removes the ue of functions from the lodash function
where regular JS can be used.

Release note: None

Closes cockroachdb#68820

This commit removes the ue of functions from the lodash function
where regular JS can be used.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz requested review from a team and maryliag and removed request for maryliag March 21, 2022 20:01
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

Copy link
Contributor

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Should we purge the lodash from our package.json as well ?

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

@xinhaoz
Copy link
Member Author

xinhaoz commented Mar 21, 2022

Unfortunately we're still using it in other places. Most of the use cases are from years ago and can be replaced with simple JS (e.g. isNil, array functions, noops), but there are some places where we don't have an appropriate utility function equivalent, like lodash's deep equals or merge functions. Not sure if we want to purge all of lodash from our package and create utils for those as well, or if it'd be worth it. Perhaps @jocrl might have more insight into whether or not it would be worthwhile to update those use cases and get rid of lodash entirely. From what I understand it is a pretty large package.

@jocrl
Copy link
Contributor

jocrl commented Mar 21, 2022

Agreed with Xin Hao, that there are still some places where lodash is useful. Lodash also exports the functions individually, so if we can define a specific set that we want to use we can just import those. I think it's a question of what our goals and priorities are.

I also think this might also involve a bigger picture of frontend decisions. For instance, managed-service still uses the whole loadash package. If anyone feels particularly motivated to try to get rid of it, I think a slack thread/RFC/file it for a CRUX ticket would be a good idea.

@xinhaoz
Copy link
Member Author

xinhaoz commented Mar 22, 2022

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Mar 22, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 22, 2022

Build succeeded:

@craig craig bot merged commit e0a4c51 into cockroachdb:master Mar 22, 2022
@xinhaoz xinhaoz deleted the db-pages-remove-lodash branch March 23, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: replace lodash with vanilla Array methods where possible in the databases pages
5 participants