search

uber-node / zero-config

A zero configuration configuration loader
MIT License
116 stars 10 forks source link

remove entire config extend per get #34

Closed Matt-Esch closed 8 years ago

Matt-Esch commented 8 years ago

It seems that we currently deep clone the entire config for each get. This is clearly suboptimal in cases with large config. I have written a safe function which extends the returned subset only. I would consider removing this feature all together in future, however this is a reasonable non-breaking patch.

cc: @Raynos

Raynos commented 8 years ago

lgtm.

anson627 commented 8 years ago

the purpose of deep clone is to make a defensive copy, so the caller cannot change internal config object.

Matt-Esch commented 8 years ago

@anson627 yes but there is no need to clone the ENTIRE object to achieve that. Most of our gets return strings. Cloning the entire config object for string gets is wasteful. We would be better off applying a recursive Object.freeze instead of cloning on read.

anson627 commented 8 years ago

I agree with the optimization. Object.freeze sounds like a better option.