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 redundant dependencies #1830

Merged
merged 6 commits into from
Jan 16, 2023
Merged

Remove redundant dependencies #1830

merged 6 commits into from
Jan 16, 2023

Conversation

ajimae
Copy link
Contributor

@ajimae ajimae commented Jan 13, 2023

Summary

Remove redundant dependencies

Description

Remove lodash.defaultsdeep from dependency tree

Todo

  • Tests
    • Unit
    • Integration
    • Acceptance
  • Documentation
  • Type label for the PR

Thanks to @studentIvan for the suggestions and recommended performance optimization here

studentIvan and others added 3 commits January 24, 2021 12:29
defaultsDeep includes the lodash.defaultsdeep file what weight 51kb
we can use just Object.assign here
- remove loadah.defaultsdeep library
- remove object.assign and use spread instread
@changeset-bot
Copy link

changeset-bot bot commented Jan 13, 2023

🦋 Changeset detected

Latest commit: ac0ac58

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@commercetools/sdk-auth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Jan 15, 2023

Codecov Report

Merging #1830 (ac0ac58) into master (43b8c12) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1830      +/-   ##
==========================================
- Coverage   94.66%   94.64%   -0.02%     
==========================================
  Files         145      145              
  Lines        4908     4912       +4     
  Branches     1334     1338       +4     
==========================================
+ Hits         4646     4649       +3     
- Misses        259      260       +1     
  Partials        3        3              
Impacted Files Coverage Δ
packages/sdk-auth/src/auth.js 98.57% <100.00%> (-0.70%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

// _.defaultsDeep({ 'a': { 'b': 2 } }, { 'a': { 'b': 1, 'c': 3 } }) // { a: { b: 2, c: 3 } }
// Object.assign({ 'a': { 'b': 2 } }, { 'a': { 'b': 1, 'c': 3 } }) // { a: { b: 1, c: 3 } }

// handle scopes array - Object.assign would merge arrays together
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is actually not really good if a missing dependency would change the behavior here. I would take a look what's the size of the config and do the merge straightforward in a function without the need of any dependency. Or directly use Object.assign

@@ -36,7 +36,7 @@
},
"dependencies": {
"@commercetools/sdk-middleware-http": "^7.0.0",
"lodash.defaultsdeep": "^4.6.0",
"lodash.defaultsdeep": "^4.6.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this dependency still needed after the change to Object.assign?

Copy link
Contributor Author

@ajimae ajimae Jan 16, 2023

Choose a reason for hiding this comment

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

Just removed it 🙂

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.

3 participants