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

fix(helpers): improve deepMergeObject handling case #62

Merged
merged 2 commits into from May 24, 2023
Merged

Conversation

Tahul
Copy link
Collaborator

@Tahul Tahul commented May 23, 2023

Hello;

Following https://github.com/nuxtlabs/studio-api/issues/708

This adds two tests:

  • recursively create objects

    • Creates a nested object by setting objects properties by hand recursively
    • This on passes properly ✅
  • recursively merge objects

    • Tries to merge two objects to get the same result as recursively create objects using deepMergeObject
    • This one does not pass ❌

Thank you for helping!

@Tahul Tahul changed the title chore(tests): write proving tests chore(tests): objects merging May 23, 2023
};

// Recursively merge existing object with `obj`
deepMergeObject(mod.exports.default, obj);
Copy link
Member

Choose a reason for hiding this comment

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

The test you provided was deepMergeObject(mod, obj) - which isn't the right target to merge. Does the same bug happen to your codebase?

Meanwhile it does help to identify a bug in the deep merge function tho. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure it was the way to use it, I landed on the same Cannot set property 'value' on type undefined error, so I thought this was an equivalent.

By using your modified version of deepMergeObject in my codebase, I get the expected result!

Thank you so much for digging in! 🙏

@antfu antfu changed the title chore(tests): objects merging fix(helpers): improve deepMergeObject handling case May 23, 2023
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #62 (6a27de9) into main (e916e98) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   92.05%   92.13%   +0.07%     
==========================================
  Files          24       24              
  Lines        1612     1615       +3     
  Branches      293      296       +3     
==========================================
+ Hits         1484     1488       +4     
+ Misses        128      127       -1     
Impacted Files Coverage Δ
src/helpers/deep-merge.ts 100.00% <100.00%> (+7.69%) ⬆️

@antfu antfu merged commit fb50fb1 into main May 24, 2023
3 checks passed
@antfu antfu deleted the test/objects-merge branch May 24, 2023 09:16
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.

None yet

2 participants