toss / es-toolkit

A modern JavaScript utility library that's 2-3 times faster and up to 97% smaller—a major upgrade to lodash.
https://es-toolkit.slash.page
Other
7.02k stars 322 forks source link

merge can mutate source #623

Closed Xiot closed 1 month ago

Xiot commented 1 month ago

I found some unexpected behaviour with the merge function.

It appears that when values are merged, there are cases where it just copies the reference to the source object.

const target = {}
const source = { a: { b: 1 } }
const m = merge(target, source)
console.log('merged', m)
console.log('target', target)
console.log('source', source)

m.a.c = 2
console.log('merged', m)
console.log('target', target)
console.log('source', source)

which produces

merged { a: { b: 1 } }
target { a: { b: 1 } }
source { a: { b: 1 } }
merged { a: { b: 1, c: 2 } }
target { a: { b: 1, c: 2 } }
source { a: { b: 1, c: 2 } }  // expected `source` to not have changed

This seems to be what is responsible https://github.com/toss/es-toolkit/blob/main/src/object/merge.ts#L98

The same code using the merge method from lodash produces

merged { a: { b: 1 } }
target { a: { b: 1 } }
source { a: { b: 1 } }
merged { a: { b: 1, c: 2 } }
target { a: { b: 1, c: 2 } }
source { a: { b: 1 } }

At least in my eyes, the one from lodash seems more correct, since the source is never mutated.
Curious to hear your thoughts on if this is intentional, and if so, what is the reasoning.

Xiot commented 1 month ago

Just noticed that there is a PR that addresses this https://github.com/toss/es-toolkit/pull/556

raon0211 commented 1 month ago

Hello, thanks for your report. We merged the pull request. https://github.com/toss/es-toolkit/blob/main/src/object/merge.ts#L98

Can you check if es-toolkit@1.22.0-dev.706 works for you?

Xiot commented 1 month ago

es-toolkit@1.22.0-dev.706 seems to fix the issue.

Thanks