Skip to content

fix: merge objects with Module type#121

Merged
pi0 merged 7 commits intomainfrom
fix/merge-non-plain-obj
Jan 5, 2024
Merged

fix: merge objects with Module type#121
pi0 merged 7 commits intomainfrom
fix/merge-non-plain-obj

Conversation

@TheAlexLichter
Copy link
Copy Markdown
Member

🔗 Linked issue

Resolves #119

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7c7a9a4) 44.74% compared to head (3174b1b) 45.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   44.74%   45.73%   +0.99%     
==========================================
  Files           4        4              
  Lines         219      223       +4     
  Branches       33       35       +2     
==========================================
+ Hits           98      102       +4     
  Misses        119      119              
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/defu.ts Outdated
Object.getPrototypeOf(prototype) === null) &&
!(Symbol.toStringTag in value) &&
!(Symbol.iterator in value)
(!(Symbol.toStringTag in value) || !(Symbol.iterator in value))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm wondering if it is purely for Date merging fix, using instance of Date would be faster

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it isn't. See the linked issue. It also applies to Module (import via *)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I see! With this PR want to merge Module instances!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Checking, this logic currently basically bypasses entire L59 as iterator symbol does not exist in any of tested values (removing the line also all the tests pass). Wondering how we can cover it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, I see! With this PR want to merge Module instances!

yes indeed 👍🏻

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Checking, this logic currently basically bypasses entire L59 as iterator symbol does not exist in any of tested values (removing the line also all the tests pass). Wondering how we can cover it

Oh, I see 🤔 That's a bit tricky. I also wonder how to cover it then...

But - do we need to? Dates are correctly replaced (not merged) like that too

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have ported test suite of is-plain-object (see commits to main) and also merged in with this PR. Intrestingly we will be breaking two more tests at least (Math and native arguments will be wrongly detected as pure object)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Updated with a logic that precisely works for Module to reduce (more!) regression chances

@pi0 pi0 changed the title fix: merge some non-plain objects properly fix: merge Module objects Jan 5, 2024
@pi0 pi0 changed the title fix: merge Module objects fix: merge objects with Module type Jan 5, 2024
@pi0 pi0 merged commit 1b9fcab into main Jan 5, 2024
@pi0 pi0 deleted the fix/merge-non-plain-obj branch January 5, 2024 18:42
@ferferga ferferga mentioned this pull request Jan 10, 2024
8 tasks
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.

[Bug] Objects exported with '* as' export aren't merged recursively

2 participants