Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to merge in additional configs? #126

Closed
max-degterev opened this issue Jul 24, 2014 · 7 comments
Closed

Allow to merge in additional configs? #126

max-degterev opened this issue Jul 24, 2014 · 7 comments

Comments

@max-degterev
Copy link

Hi!

I have an app that has main config (default and per environment) and also subapps with default and per environment configs. With previous version I was able to simply deepExtend original config with my additional ones when needed. After 1.0 release it's no longer possible due to the fact that config object is immutable.

It would be very nice to have a legit way to load additional configs on demand. Is it something you could add in the future?

@lorenwest
Copy link
Collaborator

Hi Max,

Merging additional configs is supported in v1. The wiki page isn't complete, and it's focused on external configs from a DB.

Let me know if that wiki page has enough in it to get you going, and if you have any questions.

@blackdynamo
Copy link

@lorenwest I have a similar setup as @suprMax.

I have looked at the wiki page, and it states:

Just make sure configs read from the DB aren't also defined in files because file based configs are immutable.

Unfortunately my sub processes have specific services that may override the defaults for that host. (Maybe I am crazy and should be doing something better 😺 ) I have come up with a solution, technically a mixin. Maybe you could implement this right onto the util.

var config = require("config");

function ConfigurationManager(){
    this._config = config;
}

ConfigurationManager.prototype.get = function(){
    return this._config;
};

ConfigurationManager.prototype.extend = function(options){
    this._config = config.util.makeImmutable(config.util.extendDeep(config.util.cloneDeep(this.get()), options));
    return this._config;
};

module.exports = ConfigurationManager;

It takes the config from the files and sets that as the current config. Then when you use extend, it will extend the current config with given options and return that.

@lorenwest
Copy link
Collaborator

Any external utility (such as this configuration manager) can deep copy the config, mixin it's own configurations, and expose the altered configurations. The main issue is every client must use the configuration manager. 3rd party libraries that use require('config') won't see the configuration manager, or the altered configurations.

Configurations were made immutable because once a module holds reference to a configuration value, changing it doesn't change the reference that module holds. Adding this functionality allows for configuration mutations - the very thing makeImmutable was written to prevent.

You can still do what you want. Just use different configuration names for externally sourced configurations.

If you absolutely need to externally extend file configurations, write to a local.json file from the DB before the process starts. This keeps the contract with sub-modules - that configuration values won't change from underneath them.

@max-degterev
Copy link
Author

@blackdynamo I ended up writing a small module myself and symlinking it in node_modules:

fs = require('fs')

_ = require('underscore')
_.mixin(deepExtend: require('underscore-deep-extend')(_))

readConfig = (path)->
  contents = fs.readFileSync(path, encoding: 'utf8')
  if contents then try JSON.parse(contents)

defaults =
  codename: process.env.APP or 'boilerplate'
  env: process.env.NODE_ENV
  version: require('../package').version

configPaths = [
  "./config/default.json"
  "./config/#{defaults.env}.json"

  "./apps/#{defaults.codename}/config/default.json"
  "./apps/#{defaults.codename}/config/#{defaults.env}.json"
]

configs = []
for filePath in configPaths
  if fs.existsSync(filePath)
    configs.push(json) if json = readConfig(filePath)

module.exports = _.deepExtend(defaults, configs...)

Hope this helps.

@blackdynamo
Copy link

@lorenwest Thanks for the reply.

I wasn't saying that config should incorporate the configuration manager I wrote. I was suggesting adding an extend function on the util, that allows you to make the conscious decision to extend the current config.

I understand mutability, and understand and agree with why the change was made. Adding an extend util just give people the choice to consciously extend the current configuration. I would only use this at start up. Immutable config is a huge improvement which prevents accidental changes to config during runtime.

I guess using modules could work, just requires people to do some refactoring.

@lorenwest
Copy link
Collaborator

On one hand we have the desire to extend configurations from external sources such as a database. On the other hand we want to guarantee that config values won't change.

Modules generally load on the first tick of the process. The problem with external configurations is they generally require an asynchronous callback. By the time the callback is made, most modules have already loaded, and it's too late to make configuration changes. Node-config uses the synch version of FS operations for this reason.

The only way I can see that it's ok to mutate configurations is to have the app developer mutate them before loading other modules that reference node-config. That's not very difficult, but it's easy to get wrong. Having a different process create the local.json resolves this issue, but I wouldn't mind discussing other ways of resolving it within one process.

How about if node-config allowed configuration mutations before the first call to get(). That way, app developers can extend configurations as long as they postpone loading modules that make use of node-config.

I haven't thought through this in detail, but if it worked would that solve your use case?

@lorenwest
Copy link
Collaborator

Closing this issue with new support for mutating configurations before the first get(), and the new $ALLOW_CONFIG_MUTATIONS environment variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants