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

Change lodash imports #15

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

Conversation

Gangezilla
Copy link

Hi there,

We've started using your package (which is great, by the way, thank you!) recently, and I noticed that it started bloating the JS file that it gets included into. Digging into it a bit further, I saw that this package was importing lodash components as import {foo, bar} from 'lodash', which imports the entire lodash package. I've changed the imports to be

import foo from 'lodash/foo';
import bar from 'lodash/bar';

which imports only the specific modules required. Doing this should reduce the file size footprint of this package :).

  • I also started work on replacing some of the lodash functions with native JS implementations, such as in the utils.js file. In this particular instance I replaced the lodash.keys method with Object.keys and lodash.map with the Array.map function. There are a few other instances this could be done, such as in the usage of filter, reduce, and a few other methods, but I thought I'd get this merged in first and return to these later.

Thanks, and let me know if I missed anything or you have any questions!

@@ -5,6 +5,7 @@
"main": "index.js",
"scripts": {
"test": "mocha",
"test-watch": "mocha --watch",
Copy link
Author

Choose a reason for hiding this comment

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

I added this script in here as well to make it easier to make fixes and test at the same time.

@Gangezilla
Copy link
Author

I've updated the lockfile, and the version of node in circle in order to get CI to pass. There were issues with deprecated packages, and the version of Node we were using, 8.4.0 was erroring. I've updated this to 8.9.0, as this was recommended as being safe in the CI error.

@dimitropoulos
Copy link
Contributor

forgive the ignorance if this isn't possible but is there a way to leverage https://github.com/lodash/babel-plugin-lodash here in this case? I've had good experience with it in the past.

@SergiuB
Copy link

SergiuB commented Mar 15, 2021

This PR is very useful and safe, I wonder why it wasn't merged?

@thibmeu
Copy link
Contributor

thibmeu commented Aug 24, 2022

lodash got removed in #55.
the updates of the dependencies and helper script could still be useful though.

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.

4 participants