fix(cloneDeep): maintain prototype when cloning class instances #794
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #794 +/- ##
==========================================
- Coverage 99.11% 99.08% -0.04%
==========================================
Files 305 305
Lines 2727 2727
Branches 800 800
==========================================
- Hits 2703 2702 -1
- Misses 23 24 +1
Partials 1 1 |
src/object/cloneDeep.ts
Outdated
| @@ -184,7 +185,9 @@ function cloneDeepImpl<T>(obj: T, stack = new Map<any, any>()): T { | |||
| } | |||
|
|
|||
| if (typeof obj === 'object' && obj !== null) { | |||
| const result = {}; | |||
| const result = | |||
| typeof obj.constructor === 'function' && !isPrototype(obj) ? Object.create(Object.getPrototypeOf(obj)) : {}; | |||
There was a problem hiding this comment.
I’m curious why we check if obj.constructor is a function and if obj is not a prototype here. Wouldn’t just Object.create(Object.getPrototypeOf(obj)) be sufficient?
There was a problem hiding this comment.
I assumed that calling Object.create() and Object.getPrototypeOf() on every object would be expensive and hurt performance, so I decided to add a check to ensure that we only call it on classes. But it turned out to be the opposite! Using just the Object.create(Object.getPrototypeOf(obj)) improved performance by about 15%. I updated the PR.
| @@ -238,4 +238,28 @@ describe('clone', () => { | |||
| expect(clonedError.message).toBe(error.message); | |||
| expect(clonedError.name).toBe(error.name); | |||
| }); | |||
|
|
|||
| it('should clone class instance', () => { | |||
There was a problem hiding this comment.
I think this test code is added in clone, not cloneDeep. Let me add a test case for cloneDeep, too.
There was a problem hiding this comment.
There already is a should clone instance test in cloneDeep.spec.ts, so that's why I only added a single test covering this case.
Addresses #777 and #801.
Benchmark results