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

feat(deepMerge): bring up #141

Merged
merged 7 commits into from Dec 3, 2019
Merged

feat(deepMerge): bring up #141

merged 7 commits into from Dec 3, 2019

Conversation

@SukkaW
Copy link
Member

SukkaW commented Nov 22, 2019

Continuation of hexojs/hexo#3880

@SukkaW SukkaW requested a review from curbengh Nov 22, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 22, 2019

Coverage Status

Coverage increased (+0.1%) to 95.387% when pulling 1bb1053 on SukkaW:deepMerge into a5fcfa5 on hexojs:master.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 22, 2019

I'm curious about the performance difference (if any) between native and lodash.merge.

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Nov 22, 2019

@curbengh From benchmark of hexojs/hexo#3880 it seems that it is more or less the same as the master branch.

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Nov 22, 2019

Node.js 8 Node.js 10 Node.js 12 Node.js 13
Current master branch of Hexo 38.0s 39.8s 81.7s 85.3s
hexojs/hexo#3880 40.0s 35.0s 87.9s 83.5s
lib/deep_merge.js Outdated Show resolved Hide resolved
lib/deep_merge.js Outdated Show resolved Hide resolved
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 24, 2019

const obj1 = {a: {b: 1, c: 1, d: {e: 1, f: 1}}};
const obj2 = {a: {b: 2, d: {f: 'f'} }};

deepMerge(obj1,obj2)

console.log(deepMerge(obj1,obj2))
// { a: { b: 2, d: { f: 'f', e: 1 }, c: 1 } }

console.log(deepMerge(obj1))
// { a: { b: 1, c: 1, d: { e: 1, f: 1 } } }

console.log(deepMerge(obj2))
// { a: { b: 2, d: { f: 'f', e: 1 }, c: 1 } }

Is it intended that obj1 is not modified? In that case, the example in the docs needs to be updated to,

const obj1 = {a: {b: 1, c: 1, d: {e: 1, f: 1}}};
const obj2 = {a: {b: 2, d: {f: 'f'} }};
const obj3 = deepMerge(obj1, obj2);

console.log(obj3); // {a: {b: 2, c: 1, d: {e: 1, f: 'f'} }}
console.log(obj1); // {a: {b: 1, c: 1, d: {e: 1, f: 1}}}, target object does not change.

Besides, it may not be desirable that source is modified, or at least currently deepMerge() works in reverse to Object.assign().

var o1 = { a: 1 };
var o2 = { b: 2 };
var o3 = { c: 3 };

var obj = Object.assign(o1, o2, o3);
console.log(obj); // { a: 1, b: 2, c: 3 }
console.log(o1);  // { a: 1, b: 2, c: 3 }, target object itself is changed.
console.log(o2);  // { b: 2 }, source object does not change
console.log(o3);  // { c: 3 }
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 24, 2019

The deepmerge package works well, it doesn't modify source and target objects. We can derive from it with some simplifications (i.e. remove custom options).

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Nov 24, 2019

@curbengh

https://runkit.com/sukkaw/5ddaa73ff19d1a001c7b51e6

The benchmark shows the deepmerge package is faster..

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Nov 25, 2019

@curbengh I believe the default option of deepmerge will work. I might update the test (hexojs/hexo#3880) to see if the default options works.


Update:

hexojs/hexo#3880 shows the default options of deepmerge works nearly as a perfect lodash.merge replacement, the only difference is lodash.merge will change target object while deepmerge doesn't.


2141f3b

After this commit, deepMerge() will change target while source is not touched, just act as Object.assign() or lodash.merge().

@SukkaW SukkaW force-pushed the SukkaW:deepMerge branch from 2dfa5e9 to 2141f3b Nov 25, 2019
@SukkaW SukkaW requested a review from curbengh Nov 25, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Nov 30, 2019

Using the example in the readme, somehow source is also modified.

const obj1 = {a: {b: 1, c: 1, d: {e: 1, f: 1}}};
const obj2 = {a: {b: 2, d: {f: 'f'} }};
deepMerge(obj1, obj2);

console.log(obj1) // { a: { b: 2, d: { f: 'f', e: 1 }, c: 1 } }
console.log(obj2) // { a: { b: 2, d: { f: 'f', e: 1 }, c: 1 } }

But the example used in the unit test is fine,

const obj1 = {a: 0, b: 1, c: {d: 1}, e: 4};
const obj2 = {b: 3, c: {d: 2}};
deepMerge(obj1, obj2);

console.log(obj1) // { a: 0, b: 3, c: { d: 2 }, e: 4 }
console.log(obj2) // { b: 3, c: { d: 2 } }
@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Nov 30, 2019

@curbengh I know what happend. We all know that Object.assign({}, some obj) only does a shallow copy of obj. It means that nested properties of src will still be copied by reference. Thus in the case source obj has been touched.

Without a deep clone, we can not achieve leave source completely untouched with Object.assign.


Besides the affected source, the current approach has the most similar behaviors to lodash.merge, while deepmerge package will converts array into { '0': a, '1': b }.


https://github.com/TehShrike/deepmerge#arraymerge

With deepmerge's arrayMerge option, now deepmerge could act as lodash.merge when meets array.

@SukkaW SukkaW mentioned this pull request Dec 1, 2019
1 of 2 tasks complete
@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Dec 1, 2019

https://repl.it/repls/ThoroughIndigoAddons

According to the benchmark, deepmerge is only a little bit slower than lodash.merge. I believe it is because arrayMerge.

@SukkaW SukkaW added this to To do in Replace lodash with native API via automation Dec 1, 2019
@SukkaW SukkaW moved this from To do to In progress in Replace lodash with native API Dec 1, 2019
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

curbengh left a comment

just some comment on docs.

I benchmarked this and found it's faster than using deepmerge directly.

@SukkaW SukkaW requested a review from curbengh Dec 2, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Dec 2, 2019

I'm a little surprised chai-should.eql() also works on object, I previously assumed it's similar to === which doesn't work on object.

@SukkaW SukkaW merged commit f786c38 into hexojs:master Dec 3, 2019
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.1%) to 95.387%
Details
Replace lodash with native API automation moved this from In progress to Done Dec 3, 2019
@SukkaW SukkaW mentioned this pull request Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.