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 #3985

Merged
merged 40 commits into from
May 9, 2022
Merged

Conversation

VIKTORVAV99
Copy link
Member

Lodash is currently one of our biggest dependencies when it comes to size but we don't really need it anymore.
A lot of the Lodash functions we use now have native JS replacements and since we use babel to transpile and polyfill the code during build we can safely use newer features of JS without impacting browser compatibility.

@VIKTORVAV99 VIKTORVAV99 marked this pull request as ready for review April 20, 2022 18:56
@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Apr 20, 2022

I have replaced most of the lodash functions with native code where I could figure it out and used module specific imports for the other two (if someone want to take a crack at them please do).

This PR should be ready to merge but since a lot of core functions have changed ever so slightly you might want to spend some extra time testing this PR.

PS: Some of the helper functions I created are not fully compatible with lodash but works for us as far as I can tell, these can be replaced with modular imports if needed.

@VIKTORVAV99 VIKTORVAV99 marked this pull request as draft April 26, 2022 22:20
@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Apr 26, 2022

Something broke and until I figure out what this will keep this as a draft PR.

@VIKTORVAV99 VIKTORVAV99 marked this pull request as ready for review April 27, 2022 00:34
@VIKTORVAV99
Copy link
Member Author

@madsnedergaard This should be all ready to be reviewed merged now.

I removed almost all of the lodash functions and replaced them with native code but there where a few instances that where just too cumbersome and where it wouldn't really be worth it. I replaced these with modular imports so we only get just what we need.

I know there is a lot of files changed and most changes should be very straightforward but if there is any questions don't hesitate to ask.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Apr 29, 2022
Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

Nice, so great to use more native/built-in language utilities 🤩
I've attempted my best at going through the changes and tested things - so far everything seems great and works as intended, so let's merge this :)

Edit: There's a bunch of conflicts, but approving anyway - will test again after conflicts are resolved :)

@VIKTORVAV99
Copy link
Member Author

I guess I have some merge conflicts to resolve now... 😅

@VIKTORVAV99
Copy link
Member Author

@madsnedergaard merge conflicts should be resolved now!

Maybe I should have done these PRs after each other and not in parallell. 😅

@madsnedergaard
Copy link
Member

Awesome :D

@madsnedergaard madsnedergaard merged commit 346d621 into electricitymaps:master May 9, 2022
@VIKTORVAV99 VIKTORVAV99 deleted the remove-lodash branch May 9, 2022 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants