fastify / deepmerge

Merges the enumerable properties of two or more objects deeply. Fastest implementation of deepmerge
Other
80 stars 11 forks source link

Initial implementation #1

Closed Uzlopak closed 2 years ago

Uzlopak commented 2 years ago

This is my initial implementation.

I still have to implement the .all method and write the Readme.md.

I am very proud of the typings ;).

Uzlopak commented 2 years ago

@mcollina Ready for review.

Uzlopak commented 2 years ago

@mcollina

I can not really say how much code was lifted. I have the feeling that despite the function names nothing is the same. Maybe we should just give credits to deepmerge till commit with hash e5a27710088a7b5c8c7b3ef0293dd979cac5c8c7 and thats it. Can you please make suggestions regarding how we can properly credit the maintainers of deepmerge?

Latest benchmarks:

deepmerge regex with date x 1,224,894,574 ops/sec ±0.51% (98 runs sampled)
deepmerge with primitive x 1,230,057,600 ops/sec ±0.48% (96 runs sampled)
deepmerge simple Arrays x 23,050,743 ops/sec ±1.09% (91 runs sampled)
deepmerge complex Arrays x 1,006,611 ops/sec ±1.60% (91 runs sampled)
deepmerge existing simple keys in target at the roots x 5,604,934 ops/sec ±1.01% (90 runs sampled)
deepmerge nested objects into target x 4,639,441 ops/sec ±1.80% (92 runs sampled)

I got a big performance regression, because i fixed a prototype pollution vulnerability, which also exists in the original package. But it is still faster than the original package.

benchmarks against the original deepmerge-package of TehShrike:

deepmerge regex with date x 6,950,092 ops/sec ±1.78% (87 runs sampled)
deepmerge with primitive x 810,560 ops/sec ±0.79% (93 runs sampled)
deepmerge simple Arrays x 6,133,764 ops/sec ±0.94% (92 runs sampled)
deepmerge complex Arrays x 203,232 ops/sec ±0.64% (95 runs sampled)
deepmerge existing simple keys in target at the roots x 1,686,064 ops/sec ±0.28% (91 runs sampled)
deepmerge nested objects into target x 27,660 ops/sec ±0.93% (51 runs sampled)

I reported the vulnerability to snyk, who knows when they will react:

Hi

In the deepmerge package the first parameter to the deepmerge function does not have a prototype pollution check. 

proof of concept:

const deepmerge = require("deepmerge")
const user = { }
const malicious = JSON.parse('{ "__proto__": { "isAdmin": true} }')
const mergedObject = deepmerge(malicious, user)

console.log(mergedObject.isAdmin) // true

Best Regards
Aras
Uzlopak commented 2 years ago

@mcollina

Lol, i am currently walking in the city and i get multiple thoughts: Just out of curiosity: for some reason original deepmerge does not merge inherited properties. I dont see what the purpose is to do this.

Also I think the check for isEnumerableKey makes no sense as Object.keys always just lists the enumarable keys of an Object.

I think i need to inveetigate this further...

Uzlopak commented 2 years ago

Ok, I guess I nailed it now:

deepmerge regex with date x 1,245,254,838 ops/sec ±0.22% (97 runs sampled)
deepmerge with primitive x 1,247,457,712 ops/sec ±0.17% (93 runs sampled)
deepmerge simple Arrays x 21,946,312 ops/sec ±2.03% (90 runs sampled)
deepmerge complex Arrays x 1,039,748 ops/sec ±1.08% (92 runs sampled)
deepmerge existing simple keys in target at the roots x 10,913,699 ops/sec ±0.95% (94 runs sampled)
deepmerge nested objects into target x 5,747,734 ops/sec ±0.73% (97 runs sampled)
Uzlopak commented 2 years ago

So I played around alot, but tbh I have no idea how to optimize it any further. So I guess, you can review it without expecting changes.

I also benched other deepmerge packages, and I am proud to say, that I did not find one package which was faster than this implementation. Maybe an unfair comparison, because this is "just" merging ES5 structures but hey, they could have set it behind feature-flags to only use the merging options needed.

What we can consider for the future is to maybe add the ability to merge ES6 Maps and Sets. But i didnt see that we need that feature for the places we can replace them in our packages.

mcollina commented 2 years ago

Code looks good once CI passes

Uzlopak commented 2 years ago

@mcollina Please run again the CI-pipeline