prevent modification of require() cache for overrides #1365
Conversation
There was a problem hiding this comment.
if a user has a conf.js with some general overrides defined but also provides a --conf configuration file override, then _.defaultsDeep will only copy values from the --conf file where conf.js has left undefined
Wrong
e.g. take c.strategy = 'trend_ema' in conf.js then override it using --conf=whatever.conf (lets say by c.strategy = 'rsi') c.strategy will be rsi because it is already present in the object of defaultsDeep
--conf is the object not a source
| @@ -28,30 +28,31 @@ module.exports = function (cb) { | |||
|
|
|||
| // 3. Load conf-sample.js and merge | |||
| var defaults = require('./conf-sample') | |||
| _.defaultsDeep(conf, config, defaults) | |||
There was a problem hiding this comment.
imho redundant as --conf already takes precedence over conf.js
confgets filled by--conf(the object)config(content ofconf.js) anddefaults(content ofconf-sample.js) sources fillconfwith all new properties
The priority should always be --conf -> conf.js -> conf-sample.js
We call --conf config overrides for a reason.
There was a problem hiding this comment.
Indeed, you're correct. I misdiagnosed a different issue for this. However, I'm still willing to submit this as-is because _.defaultsDeep() mutates the source object, which in this case is the object from the require() cache and I think it's a bad practice to modify that object since it gets require()d elsewhere in the app and that code expects it to be unmodified.
There was a problem hiding this comment.
I initially had _.defaultsDeep(conf, ... as an empty object and sourced it using overrides, config, defaults) in yesterdays PR but reworked it as it looked like an overkill.
mutates the source object
According to the docs this only mutates the object (not the sources) which i am fine this as this is indeed our running config. The require alone wouldn't suffice anyway, unless someone parses an entire working config using --conf of course.
There was a problem hiding this comment.
By mutating the object (which is what is going to be returned on subsequent require() calls to the override path), we end up providing a full copy of the config to the commands that process the overrides for their internal s.options i.e.: https://github.com/DeviaVir/zenbot/blob/unstable/commands/sim.js#L52 - instead of just the subset in the overrides file, which will be a much larger set of properties than it currently retains.
There was a problem hiding this comment.
I actually prefer the idea of what you described as your initial implementation, using an empty object as the source. I might update this to just do that.
krystophv
changed the title
This modifies the
_.defaultsDeepcall to extend a local empty object instead of an object that comes from areauire()cache.Also specifies that we get all the mongo config values from the
zenbot.confcopy.