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

Merge configs using lodash merge #101

Closed
wants to merge 1 commit into from

Conversation

innovate-invent
Copy link
Contributor

Fixes #97

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #101 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #101   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines          96     99    +3     
  Branches       16     16           
=====================================
+ Hits           96     99    +3
Impacted Files Coverage Δ
src/api/Request.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c6f511...f73186e. Read the comment docs.

@kiaking
Copy link
Member

kiaking commented Feb 26, 2020

Thank you so much for the PR! Sorry but could we not use lodash? It adds to the bundle size quite a bit...

@innovate-invent
Copy link
Contributor Author

It imports the lodash submodule for that function only, is that still large?

@kiaking
Copy link
Member

kiaking commented Feb 26, 2020

Unfortunately yes... we had same problem at Vuex ORM Core. Lodash uses many common functions, so even if you import only specific features, it still loads unnecessary codes.

I really hate this too, but it's how JS libraries have to do things I guess...

@innovate-invent
Copy link
Contributor Author

I guess the code will have to be ripped out of the lodash library..

@kiaking
Copy link
Member

kiaking commented Feb 26, 2020

Yeah that's what I did when implementing orderBy feature at Vuex ORM Core. You could check this one out though.
https://github.com/vuex-orm/vuex-orm/blob/3c642728b829006a93ac1d1f9b33144d109ad606/src/support/Utils.ts#L214

@cuebit
Copy link
Member

cuebit commented Feb 26, 2020

Yea, lodash is a simple fix but adds considerable weight and a significant performance impact.

The clone example that @kiaking is referring to may point you in the right direction – it's quite verbose because performance is crucial for Vuex ORM – so it could easily be half the code.

Note, the signature for type-safe merging is considerably more complex than cloning. If you wish to preserve types. Sometimes I abhor TS.

@innovate-invent innovate-invent marked this pull request as draft April 9, 2020 16:54
@cuebit
Copy link
Member

cuebit commented Feb 1, 2021

Closing due to inactivity. I don't believe there's been much steer on this topic to warrant this PR progressing.

@cuebit cuebit closed this Feb 1, 2021
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.

Merge configs
3 participants