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

refactor(karma-webpack): Remove lodash from dependencies #364

Merged
merged 4 commits into from
Nov 24, 2018

Conversation

Chalarangelo
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

In an attempt to remove dependencies that are barely used in popular npm packages, I have replaced lodash's clone method with an inline clone function with the same functionality. Based on that and provided that lodash is not used anywhere else in this package from what I can tell, the dependency on lodash has also been removed from package.json.

Breaking Changes

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Oct 1, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@ryanclark ryanclark 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 your PR, removing lodash is great. I only see one issue - can you change your clone function to work on nested objects/arrays? The webpack config is nested so if we're doing a clone it should be deep.

Alternatively, we might not even need to clone it - I'm not sure why it is. If this is so, we can just remove both the clone part and lodash.

Copy link
Collaborator

@matthieu-foucault matthieu-foucault left a comment

Choose a reason for hiding this comment

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

  • _.clone is a shallow clone, so no need to make the clone function recursive (if we need it at all).
  • Given that Object.assign({}, undefined); and Object.assign({}, null); both return {}, there's no need for those || {}

Copy link
Collaborator

@matthieu-foucault matthieu-foucault left a comment

Choose a reason for hiding this comment

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

Should the updated package-lock.json be committed as well?

@ryanclark
Copy link
Collaborator

@matthieu-foucault thanks - I don't personally use lodash. I agree with your second point then I'm happy with it - also the package-lock.json does need to be committed.

@Chalarangelo
Copy link
Contributor Author

  • package-lock.json updated.
  • Removed the || {}.

Anything else?

Copy link
Collaborator

@matthieu-foucault matthieu-foucault left a comment

Choose a reason for hiding this comment

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

Look good to me

@matthieu-foucault matthieu-foucault changed the title Remove lodash from dependencies refactor(karma-webpack): Remove lodash from dependencies Nov 24, 2018
@matthieu-foucault matthieu-foucault merged commit 432d139 into codymikol:master Nov 24, 2018
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.

4 participants