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

Use native or/and lodash modules instead of the whole lodash lib #291

Merged
merged 22 commits into from
Sep 1, 2021
Merged

Use native or/and lodash modules instead of the whole lodash lib #291

merged 22 commits into from
Sep 1, 2021

Conversation

kud
Copy link
Contributor

@kud kud commented Aug 20, 2021

I wanted to reduce the impact of this lib on my application, as it uses lodash in global and it is a bit huge.

For that I use either what was said here https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore or some lodash modules to be sure to have the same result.

This is a first shot. Maybe we can go deeper one day by removing all lodash modules but this is already a first step.

@danpaz
Copy link
Owner

danpaz commented Aug 20, 2021

Thanks for the PR! I'll need to fix the travis builds first (#292) before we can merge this, please give me a bit of time to get that sorted and I'll come back to review this.

@kud
Copy link
Contributor Author

kud commented Aug 20, 2021

Sure! Thank you. :)

@danpaz
Copy link
Owner

danpaz commented Aug 20, 2021

@kud I think it's fixed now. Can you push a new empty commit to this branch to kick off a build?

Copy link
Owner

@danpaz danpaz left a comment

Choose a reason for hiding this comment

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

As part of this PR would you be able to share bundle sizes you observe before and after these changes? I guess I was under the impression when we wrote this library that babel would automatically treeshake unused parts of lodash out of the bundle. I wonder if I was wrong about that or if something's changed with tooling over the years.

.prettierignore Outdated
@@ -0,0 +1 @@
*
Copy link
Owner

Choose a reason for hiding this comment

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

I assume this is to prevent your editor from autoformatting? It would be nice to introduce prettier to this project in a future PR, but for now can you remove this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. Okay :)

@kud
Copy link
Contributor Author

kud commented Aug 23, 2021

As part of this PR would you be able to share bundle sizes you observe before and after these changes? I guess I was under the impression when we wrote this library that babel would automatically treeshake unused parts of lodash out of the bundle. I wonder if I was wrong about that or if something's changed with tooling over the years.

Hello @danpaz . I understand your questionning/thoughts. The thing is it's great from you to have added the lodash babel plugin to reduce the size of the build via a tree shaking. However, it doesn't work for the final user of your lib if they don't add this optimisation. It works for your final built but not if the user imports your lib via commonjs/esm, which is my case.

That's why I'd like via this PR to suggest using only what it matters.

For the result, I saw that webpack-bundle-analyzer doesn't display anymore lodash in my final built. Which is great. :)

Copy link
Owner

@danpaz danpaz left a comment

Choose a reason for hiding this comment

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

LGTM!

@danpaz danpaz merged commit 34c47d2 into danpaz:master Sep 1, 2021
@danpaz danpaz mentioned this pull request Sep 1, 2021
@kud kud deleted the feat/reduce-lib branch September 1, 2021 08:05
@kud
Copy link
Contributor Author

kud commented Sep 1, 2021

Thanks 😊

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