Fix case where multiple sections with same settings could modify shared configuration

[mridgway]

See the test fixture where previously "settings": ["lang:es"] would modify the shared object with foo: 3 causing ycb.read({ lang: 'fr' }) to also resolve to 3 even though it should be 1.

@lingyan

[drewfish]

Nice test case.

I'm wondering if the mergeDeep(cloneDeep()) could be optimized some how. Perhaps a special version of mergeDeep() where instead of just copy from to (ack) it does a deepClone() of to.
https://github.com/yahoo/ycb/blob/master/lib/mergeDeep.js#L12

edit: Or maybe this is where the cloneDeep() could be introduced:
https://github.com/yahoo/ycb/blob/master/lib/mergeDeep.js#L21

[mridgway]

I agree this might be heavy handed, but it is only run at instantiation time as well. Interestingly this was the last place that assumed that mergeDeep mutated to rather than returning a new object, so I will look into that. I will need to make sure that it doesn't affect the performance of the settings merge during read.

In a related note: in a previous PR I optimized mergeDeep to clone from inline rather than do the cloneDeep on the full object up front. It would be a similar performance gain, but only during YCB instantiation.

[lingyan]

Changing something that might affect read sounds a little scary. I'm ok with mergeDeep(cloneDeep) if it is only a one-time cost at instantiation.

[redonkulus]

👍

[mridgway]

Updated the mergeDeep function with a safe option that we can use in the non-critical code paths.

[lingyan]

minor: move var key; on line 14 earlier

[lingyan]

🚢