Conversation
mcollina
left a comment
There was a problem hiding this comment.
Thanks for opening a PR! Can you please add a unit test?
|
For sure. I was planning on doing that anyways, just made a draft to know if you are ok with the solution. |
|
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. |
This is true.
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. |
|
|
||
| function isMergeableObject (value) { | ||
| return typeof value === 'object' && value !== null && !(value instanceof RegExp) && !(value instanceof Date) | ||
| return typeof value === 'object' && value !== null && !(value instanceof RegExp) && !(value instanceof Date) && !(value instanceof Buffer) |
There was a problem hiding this comment.
I think we have the same issue with Stream objects
closes #10
Checklist
npm run testandnpm run benchmarkand the Code of conduct