feat: add deep-merge util

[naseemkullah]

Which problem is this PR solving?

• Create / or port / a deep merge function that we can use for merging configs etc. #1496

Short description of the changes

• Create a utility in and export from the core package to be shared across packages to deep merge two objects.

[codecov]

Codecov Report

Merging #1503 into master will increase coverage by 0.48%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1503      +/-   ##
==========================================
+ Coverage   92.69%   93.18%   +0.48%     
==========================================
  Files         152      156       +4     
  Lines        4409     4857     +448     
  Branches      846      987     +141     
==========================================
+ Hits         4087     4526     +439     
- Misses        322      331       +9     

Impacted Files	Coverage Δ
...ackages/opentelemetry-core/src/utils/deep-merge.ts	100.00% <100.00%> (ø)
packages/opentelemetry-plugin-http/src/utils.ts	97.42% <0.00%> (ø)
packages/opentelemetry-plugin-http/src/http.ts	97.88% <0.00%> (ø)
packages/opentelemetry-plugin-https/src/https.ts	100.00% <0.00%> (ø)

[naseemkullah]

Thanks for comments @vmarchaud, ready for re-review.

[dyladan]

Did this implementation come from somewhere that we might need to include the original license, or was it written from scratch?

[dyladan]

This function does no sort of cycle checking, which could cause an infinite CPU loop. This needs at the very least to be documented, but it might be nice to actually do some cycle checking. You don't need to do anything fancy like a Floyd's algorithm, but a simple maxDepth optional parameter that defaults to 10 or something and decrements each time you recurse might be a good sanity check.

[naseemkullah]

Thanks, TIL

maxDepth param added... Can you provide a scenario where this could happen btw?

[obecny]

whats happened to previous version of this function I think it had more cases and code.
simple example

deepMerge({a: [4, 5, 6]}, { a: [3]})
// should be {a: [3,4,5,6]} but it returns [3,5,6]

deepMerge({a: 1}, {a: {c: 1}})
// should be {a: {c: 1}} but it returns {a: 1}

[dyladan]

whats happened to previous version of this function I think it had more cases and code.
simple example

deepMerge({a: [4, 5, 6]}, { a: [3]})
// should be {a: [3,4,5,6]} but it returns [3,5,6]

The code comments state that arrays should be replaced, not merged. In this case, I would expect {a: [3] }.

[naseemkullah]

whats happened to previous version of this function I think it had more cases and code.
simple example

deepMerge({a: [4, 5, 6]}, { a: [3]})
// should be {a: [3,4,5,6]} but it returns [3,5,6]

The code comments state that arrays should be replaced, not merged. In this case, I would expect {a: [3] }.

Right, the comments are misleading, @obecny is correct.

[naseemkullah]

Did this implementation come from somewhere that we might need to include the original license, or was it written from scratch?

Was based off what I saw in a blog post, not sure which, I found this and this which both have the custom function this is based off of, it may have been one of these. It was changed quite a bit though. Shall I attribute them?

[naseemkullah]

ready for re-review @obecny @dyladan

[obecny]

why do you set it first as an array and then override it with value ? If the last line
merged[prop] = value;
should win then this condition is not needed as it is exactly the same action as else so you could simplify it to

if (bothPropsAreObjects(target, source, prop)) {
} else {
}

unless this is a bug and should be fixed differently.

[naseemkullah]

Not sure about this one, as it stands the merged[prop] at that time holds the target array as a value, but we want it to hold the source object's value however (as this util will replace arrays entirely). i.e. we want to replace its array value entirely, that's why i reset it first, unless there is a function to replace arrays I am unaware of?

[obecny]

but you are assigning to merged[prop] 2 different values - one after another so it doesn't make sense to overrride it first with array ([]) and then with value

[naseemkullah]

Ah yes thanks. the initialization does seem unnecessary. I thought [4] merged into [1,2,3] would result in [4,2,3] without the initialization but does not seem to be the case.

[naseemkullah]

fixed, PTAL

[obecny]

ok so now just last concern, you have here 3 conditions

1. if (bothPropsAreArrays(target, source, prop)) {
2. } else if (bothPropsAreObjects(target, source, prop)) {
3. else

Condition 1 and 3 returns the same, so as I mentioned at the beginning you can simplify it to

if (bothPropsAreObjects(target, source, prop)) {
} else {
}

[naseemkullah]

ah right, so I tried that, in in fact both props being arrays does satisfy bothPropsAreObjects, in that case we do get the [4] merged into [1,2,3] resulting in [4,2,3] .

The alternative would be to have the function be bothPropsAreObjectsButNotArrays or something less verbose that depicts that

[naseemkullah]

Do you prefer a bothPripsAreObjectsButNotArrays approach or the order-being-important 3 case approach as is now?

[obecny]

a ok then that makes sense now if arrays are being detected also as an objects, I would leave it as it is then, just add comment that the first condition is needed t prevent array being parsed as objects, thx

[naseemkullah]

I just refactored the if statement to make it apparent without comment, PTAL

[obecny]

lgtm