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

fix: don't include all of lodash #1847

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RobinClowers
Copy link

💥 Breaking Changes

  • None

🚀 Enhancements

  • Visx packages now only depend on specific lodash function packages, rather than all of lodash. This affects visx-responsive, visx-text, and visx-xychart.

📝 Documentation

  • None

🐛 Bug Fix

  • None

🏠 Internal

  • None

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

thanks for the improvement here @RobinClowers ! 🙌 had a quick comment on the non-root lock files.

packages/visx-shape/yarn.lock Outdated Show resolved Hide resolved
@RobinClowers RobinClowers force-pushed the fix-lodash-dependency branch from 8608590 to 95ac4ff Compare June 6, 2024 20:15
@RobinClowers
Copy link
Author

@williaster can we get this merged please? Let me know if there is anything else outstanding.

@VIKTORVAV99
Copy link

@RobinClowers have you considered replacing lodash entirely with https://es-toolkit.slash.page/?

It should be faster and smaller while being more or less a drop in replacement for lodash functions.

@RobinClowers
Copy link
Author

@VIKTORVAV99 I hadn't heard of es toolkit, looks like an awesome project!

Since many of these projects only depend on one or two functions, I'm not sure the savings would be significant (compared to all of lodash). Also, there might be subtle differences in the behavior between the libraries. That said, it sounds like a good improvement, but I'm not personally motivated to make the change. Especially since this low risk PR hasn't been merged, I worry that it would be wasted effort.

@RobinClowers
Copy link
Author

Oh, one other concern, I didn't know what the browser support policy is for visx, it might not be safe to depend on modern is for this project. For my project it would be, but again I haven't looked closely at the browser support guarantees visx provides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants