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: replace lodash with es-toolkit #6502

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wojtekmaj
Copy link
Contributor

What's the problem this PR addresses?

es-toolkit is a modern lodash alternative that claims to be much faster and lighter, potentially improving Yarn overall performance: https://es-toolkit.slash.page/intro.html

How did you fix it?

The lazy way, frankly: using es-toolkit/compat to provide practically drop-in lodash replacement with minimal effort.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz
Copy link
Member

merceyz commented Sep 13, 2024

Some of our dependencies are using lodash so this would increase the install size and maybe the bundle size.

@wojtekmaj wojtekmaj force-pushed the es-toolkit branch 2 times, most recently from 1d28dc6 to 461da26 Compare September 13, 2024 23:05
@wojtekmaj wojtekmaj marked this pull request as draft September 13, 2024 23:05
@wojtekmaj wojtekmaj force-pushed the es-toolkit branch 3 times, most recently from f012bde to d6e415c Compare September 14, 2024 19:49
@wojtekmaj
Copy link
Contributor Author

Valid point, but it seems like not too many dependencies are in fact using lodash. It should be relatively easy to clean this whole tree up. In the meantime, this draft PR is in itself ready to go I believe. :)

@wojtekmaj wojtekmaj marked this pull request as ready for review September 14, 2024 20:57
@wojtekmaj
Copy link
Contributor Author

wojtekmaj commented Sep 14, 2024

Well, this PR DOES make Yarn smaller by a bit:

Before: 2.62 MiB
https://github.com/yarnpkg/berry/actions/runs/10862544508/job/30145695733#step:6:366

After: 2.6 MiB
image

I believe that's because while other dependencies like ink are indeed using lodash, they, just like Yarn, are importing modules selectively from it, leading to better tree shaking than one could initially assume.

1% improvement is not much but it certainly adds up 🤷

I also see a potential of shaving off further 2 KB, but I'd rather do it in small stages.

es-toolkit is a modern lodash alternative that claims to be much faster and lighter, potentially improving Yarn overall performance: https://es-toolkit.slash.page/intro.html
@merceyz
Copy link
Member

merceyz commented Sep 14, 2024

while other dependencies like ink are indeed using lodash, they, just like Yarn, are importing modules selectively from it

Because I patched it to do that, in the future we could change that patch to use es-toolkit instead.
https://github.com/yarnpkg/berry/blob/fb6d2d696f36040bb082ddd20adffab67eaa59dc/.yarn/patches/ink-npm-3.0.8-3a8005f59f.patch

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Please add an entry for lodash and @types/lodash here:

forbidDependency(ctx, `inquirer`, `Don't depend on inquirer - we use enquirer instead`);

@wojtekmaj
Copy link
Contributor Author

wojtekmaj commented Sep 14, 2024

I couldn't forbid @types/lodash because it appears to be deeply rooted inside pkg-tests-specs, but lodash alone was fine :)

@@ -209,6 +209,8 @@ module.exports = defineConfig({
enforceConsistentDependenciesAcrossTheProject(ctx);
enforceWorkspaceDependenciesWhenPossible(ctx);
forbidDependency(ctx, `inquirer`, `Don't depend on inquirer - we use enquirer instead`);
forbidDependency(ctx, `lodash`, `Don't depend on lodash - we use es-toolkit instead`);
forbidDependency(ctx, `inquirer`, `Don't depend on inquirer - we use enquirer instead`);
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
forbidDependency(ctx, `inquirer`, `Don't depend on inquirer - we use enquirer instead`);

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.

2 participants