Add check for `Buffer` - Githubissues

fastify / deepmerge

Merges the enumerable properties of two or more objects deeply. Fastest implementation of deepmerge

Other

80 stars 11 forks source link

Add check for `Buffer` #11

Closed mikaelkaron closed 1 year ago

mikaelkaron commented 2 years ago

closes #10

Checklist

[ ] run npm run test and npm run benchmark
[ ] tests and/or benchmarks are included
[ ] documentation is changed or added
[ ] commit message and code follows the Developer's Certification of Origin and the Code of conduct

mikaelkaron commented 2 years ago

For sure. I was planning on doing that anyways, just made a draft to know if you are ok with the solution.

Uzlopak commented 2 years ago

The issue with this PR is, that it is a breaking change. Also it would mean that it would probably not work in Browser because Browser does not have a builtin Buffer. So a instanceof Buffer would throw.

Also i can not agree with the assessment, that Array.isArray(Buffer.from([])) or Buffer.from([]) is returning true. So the Array Merge logic does not get applied. I assume It happens only if there is a buffer polyfill existing which is using Array as parent class.

mikaelkaron commented 2 years ago

The issue with this PR is, that it is a breaking change. Also it would mean that it would probably not work in Browser because Browser does not have a builtin Buffer. So a instanceof Buffer would throw.

This is true.

Also i can not agree with the assessment, that Array.isArray(Buffer.from([])) or Buffer.from([]) is returning true. So the Array Merge logic does not get applied. I assume It happens only if there is a buffer polyfill existing which is using Array as parent class.

I was to quick on my assessment there, the array merge logic does indeed not get applied. I'll try to rework the MR to manage these two points.