search

lukeed / klona

A tiny (240B to 501B) and fast utility to "deep clone" Objects, Arrays, Dates, RegExps, and more!
MIT License
1.62k stars 43 forks source link

Fixed class Object assign. #31

Closed tripodsgames closed 3 years ago

tripodsgames commented 3 years ago

When we clone a class that uses Object.assign the cloning result is different. This PR fixes it by removing the hasOwnProperty check. This PR fixes it by checking if the property exists in the original object.

See https://github.com/lukeed/klona/issues/30#issuecomment-908270498 and https://github.com/lukeed/klona/issues/30#issuecomment-919723976

codecov-commenter commented 3 years ago

Codecov Report

Merging #31 (fd9cab1) into master (28565c3) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #31   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          115       115           
=========================================
  Hits           115       115
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/lite.js 100.00% <100.00%> (ø)

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 28565c3...fd9cab1. Read the comment docs.

lukeed commented 3 years ago

Hi, please remove all code style changes. I can look at the diff after irrelevant changes have been removed.

tripodsgames commented 3 years ago

Hi, please remove all code style changes. I can look at the diff after irrelevant changes have been removed.

Done it.

tripodsgames commented 3 years ago

@lukeed, if you want I can do the tests using Object.keys

lukeed commented 3 years ago

No thank you.

I'll review this over the weekend, sorry for the delay. Waiting because I'm pretty sure there was a reason tmp was used, but clearly I forgot to document that reason in the tests 🙈

tripodsgames commented 3 years ago

No thank you.

I'll review this over the weekend, sorry for the delay. Waiting because I'm pretty sure there was a reason tmp was used, but clearly I forgot to document that reason in the tests see_no_evil

Ok, hope you can find a solution for this one, using the x instead of tmp fix the problem for me at least... by the way, i created a new test for Object.assign in this one. Use it in your tests to fix the Object.assign please, i use it a lot.

tripodsgames commented 3 years ago

Any update?

danieljackins commented 3 years ago

Would also love for this PR to get merged 🙏