Proposal: Improve library's architecture (a.k.a. NextGen)

[iMoses]

TL;DR This proposal changes the API but keeps most of the original hueristics completely intact. Tests were added and adopted, but with the exception of deprecated methods (e.g. util methods), no tests were removed and all previously defined tests still pass.

You can check the code here: https://github.com/iMoses/node-config/tree/next-gen

Disclaimer: some breaking changes can be reverted while others cannot. If you feel strongly about some of the changes let me know and let's discuss the available possibilities.

Motivation

About half of today's open issues are related to immutability inconsistencies and sub-modules support. while many closed issues are related to non-intuitive methods of initializing the library and means of adding support for this and that, which can come down to more control over the library's internal mechanisms.

While looking for possible solutions I came to realize that the main issue with sub-modules is our lack of support for multiple instances, and the main issue with that is our reliance on environment variables for initialize the config instance.

The second major issue is that we initialize the main instance ourselves when loaded, which prevents us from introducing new features to manipulate the library's options before initialization.

These problems can only be solved by changing the initialization logic of the library, which wouldn't have been much of a problem if we weren't expected to return an instance already initialized and containing the resulted configuration files on top of the instance's root... (backwards compatibility) Which brings us back to the immutability issues that are complex due to the same logical problems.

If we change the Config instance API and remove direct access to the configuration object (at least from the root) we can enforce mutability and delay files loading until the first access to the configuration object, instead of the config instance as we do today.

It all comes down to that:

• Allow the creation of multiple instances
• Remove reliance on environmental variables
• Better enforce immutability
• Lazy-load configuration

Along with changes to the parser to support validators and middleware we can cover most of today's open issues in one fine swipe, which will require a major version with many breaking changes effecting only advance users.

I think it's worth it :)

Breaking changes

• Removed config.util methods
• Removed runtime.json support
• Removed custom-environment-variables.* support
• Changed location of asyncConfig and deferConfig
• asyncConfig now uses config.whenReady to resolve
• get() and has() are available only on the config instance - removes all reserved words
• Changed Parser API to a programmatic solution, as opposed to an external file
• getArgv() slightly differs from its predecessor - in case of multiple matching arguments it returns the last (previous logic was to return the first occurrence)
• Removed raw() as it is now obsolete (complex objects are not extended by default)

Removed config.util methods

We have many utilities which are exposed as part of our API and I claim that most of these shouldn't be exposed at all. This isn't the library's purpose, no one installs node-config so that they can use config.util.extendDeep, and exposing them as part of our API is forcing us to keep support for them or else we introduce a breaking change, which makes it a fragile API that is forced to carry legacy code.

My proposal removes config.util completely.

Internal utilities are placed at config/lib/utils with a disclaimer that we will not guarantee stability between versions. Users can decide to use them at their own risk. Anything worthwhile should be placed on top of the config instance, while we keep utils strictly internal.

Removed runtime.json support

Support was removed in an effort to dispose of deprecated code and inconsistent heuristics, and can be simply restored by calling config.parseFile manually.

config.parseFile(process.env.NODE_CONFIG_DIR + '/runtime.json');

Removed custom-environment-variables.* support

Support was removed in an effort to dispose of inconsistent heuristics.

The new architecture adds middleware support to the parser which can be used to apply templates. A template engine can be used to apply the same logic in a consistent manner, unlike custom-environment-variables.*, and replace it with a cross-extension ability to set values from environment variables and much more.

See Parser.lib.middleware.configTemplate() for examples.

Changed location of asyncConfig and deferConfig

Previously these methods were in separate files outside of the /lib directory to allow easy access from configuration files without trigger the initialization of the config instance. With the new architecture this isn't necessary anymore and we can simply export these methods as part of out instance.

Previous version:

const { asyncConfig } = require('config/async');
const { deferConfig } = require('config/defer');

New version:

const { asyncConfig, deferConfig } = require('config');

I added shim files for now with a deprecation warning.

asyncConfig now uses config.whenReady to resolve

Previous version:

const config = require('config');
const { resolveAsyncConfigs } = require('config/async');
resolveAsyncConfigs(config)
  .then(() => require('./main'));

New version:

const config = require('config');
config.whenReady.then(() => require('./main'));

get() and has() are available only on the config instance

The main reason for this is to remove reserved words completely. I don't think the library should reserve any keys.

Previous version:

const config = require('config');
const db = config.get('services.db');
console.log(db.has('host'));  // true

New version:

const config = require('config');
const db = config.get('services.db');
console.log(db.has('host'));  // Error: method has doesn't exist

Changed Parser API to a programmatic solution

Explained in the Parser API reference.

It changes quite a lot, but I figured since it's a very new feature users who already use it are early adopters and won't mind as much, since it's a much cleaner API with validators and middleware :)

getArgv() slightly differs from its predecessor

In case of multiple matching arguments it returns the last (previous logic was to return the first occurrence). It actually seemed like a bug-fix to me, since that's what I would've expected to be the correct behavior. I also added support for boolean and space separated cli-args.

// in case of `node index.js --NODE_ENV=dev --NODE_ENV=prod`
console.log(utils.getArgv('NODE_ENV'));  // prod

Supported syntax:
    --VAR_NAME=value    value separated by an equality sign
    --VAR_NAME value    value separated by spaces
    --BOOL_VAR          boolean - no value automatically returns true

API Reference

Config

Config is an internal class that's used to create configuration instances, as it did in previous versions.

Configuration instances are created by three means:

1. Config.create(options)
2. Config.subModule(moduleName)
3. A default instance that is initiated and returned by node-config

The default instance initialization options comes from env-vars and cli-args, using the same heuristics as previous versions.

module.exports = new Config(null, getDefaultInstanceOptions());

function getDefaultInstanceOptions() {
  return clearUndefinedKeys({
    configDir: utils.getOption('NODE_CONFIG_DIR', './config'),
    environment: utils.getOption('NODE_CONFIG_ENV') || utils.getOption('NODE_ENV'),
    hostname: utils.getOption('HOST') || utils.getOption('HOSTNAME'),
    appInstance: utils.getOption('NODE_APP_INSTANCE'),
    strict: Boolean(utils.getOption('NODE_CONFIG_STRICT_MODE')),
    freeze: !utils.getOption('ALLOW_CONFIG_MUTATIONS'),
    legacy: true,
  });
}

Common Usage

Users who've been using the library according to the Common Usage section recommendations won't be affected (in most cases) by these changes.

// autoload defaults on first access
const config = require('config');
console.log(config.get('some.key'));  // value
console.log(config.has('another.key'));  // boolean

You can also access the config property directly if you prefer it over the instance API.

const { config } = require('config');
console.log(config.some.key);  // value

Autoload and Mutation methods

Configuration instances will automatically load with the default options provided to the constructor (see Config.loadDefaults(legacy)), unless a mutation methods has been used in which case the autoload mechanism is disabled.

Mutation methods have access to modify the configuration object without accessing the external property. These are the only methods which are available to mutate the configuration object, and they are automatically locked once the configuration object is frozen.

• Config.loadFiles(options)
• Config.parseFile(filename)
• Config.extend(object, source)

That's important to keep in mind when using one of these methods. A common pitfall is to use a mutation method and forget about it canceling the autoload mechanism.

// overrides default options
// only the content of variables.yaml will be available
const config = require('config')
  .parseFile(__dirname + '/scripts/variables.yaml');

Which can be simply solved by executing the Config.loadDefault(legacy) manually.

// loads defaults before parsing variables.yaml
const config = require('config')
  .loadDefaults(true)
  .parseFile(__dirname + '/scripts/variables.yaml');

asyncConfig and deferConfig

All library files were moved to /lib, and since they can now be loaded directly from the config instance without triggering the autoload sequence it acts as a simpler API.

const { deferConfig, asyncConfig } = require('config');

Config.config

The is the jewel in the crown, the reason for this major change in the library's API.

In previous versions the configuration instance is initialized when the module is loaded and automatically reads options, loads files, run validations and freezes the configuration object, which caused many problems. Moving the configuration object to a separate property allows us to observe access and delay the autoload sequence until it is used.

This property is lazily loaded so until accessed the following will not be activated.

On first access this property triggers the resolve mechanism, composed of these steps:

• Executes loadDefaults(true) only in case none of the mutation methods were used
• Resolves all deferConfig values
• Disable all mutation methods
• Freeze sources and the configuration object

Once resolved the frozen configuration object is set to this property's value.

Note that Config.get() and Config.has() access this property, so it can trigger the resolve mechanism.

const config = require('config');
// only resolves once config.get is used
// until then we can use mutation methods (loadFiles, parseFile, extend)
console.log(config.get('services.db.host'));  // localhost

Config.whenReady

This property is lazily loaded so unless accessed the following will not take effect.

On first access this property triggers the resolve mechanism for both deferConfig and asyncConfig. The property returns a promise which resolves after all asyncConfig were replaced with their final values.

This is a replacement mechanism for resolveAsyncConfigs(config).

Config.options

A frozen copy of the options passed to the instance constructor, mainly for debugging purposes.

Config.parser

Property that's used to access the Parser instance used by the instance.

Config.sources

Returns an array of sources used to compose the configuration object. The array is composed of objects, each with a source string and a data object containing the source's content.

Note that when using sub-modules, any mutations caused by a sub-module will also contain the moudle property which hold the module name of the effecting sub-module. (see Config.subModule)

This is a replacement mechanism for config.util.getConfigSources().

const config = require('config');
config.parseFile('/absolute/path/to/file.js');
config.loadFiles({configDir: __dirname + '/ultra-config'});
config.extend(JSON.parse(process.env.MY_ENV_VAR), '$MY_ENV_VAR');
config.extend({defaultProp: 'MyValue'});
console.log(config.sources);

Would output:

[
  {source: '/absolute/path/to/file.js', data: {/* ... */}},
  {source: '/absolute/path/to/ultra-config/defaults.json', data: {/* ... */}},
  {source: '/absolute/path/to/ultra-config/local.yml', data: {/* ... */}},
  {source: '$MY_ENV_VAR', data: {/* ... */}},
  {source: 'config.extend()', data: {defaultProp: 'MyValue'}},
]

Config.create(options)

Create's a new configuration instance that's completely independent.

Should be used by package owners who wish to use node-config without effecting dependent packages who may use the library themselves. This method is also useful for testing purposes.

const myConfig = require('config').create({
  configDir: __dirname + '/config',
  appInstance: process.platform,
  environment: 'package',
});

options will be passed down to Config.loadFiles(options) in case of an autoload or a manual execution of Config.loadDefaults(legacy).

• configDir - path of the configuration dir
• appInstance - appInstance used to match files
• environment - environment used to match files, defaults to "development"
• hostname - hostname used to match files, defaults to os.hostname()

Additional options are:

• strict - passed down to strictValidations, replaces NODE_CONFIG_STRICT_MODE
• legacy - passed down to loadDefaults as part of the autoload mechanism
• freeze - if set to false the configuration object will never be locked

Config.get(key)

Same as in previous versions.

const config = require('config');
console.log(config.get('services.db.host'));  // localhost

Config.has(key)

Same as in previous versions.

const config = require('config');
console.log(config.has('services.db.host'));  // true

Config.loadFiles(options)

Uses Config.loadConfigFiles(options) and passes matching files to Config.parseFile(filename).

Accepts an options object which is passed down to loadConfigFiles:

• configDir - path of the configuration dir
• appInstance - appInstance used to match files
• environment - environment used to match files, defaults to "development"
• hostname - hostname used to match files, defaults to os.hostname()

const config = require('config')
  .loadFiles({
    configDir: __dirname + '/resources/configuration',
    appInstance: process.platform,
    environment: 'override',
    hostname: 'node-config',
  });

Config.parseFile(filename)

Extends the internal configuration object with the parsed content of filename.

const config = require('config')
  .parseFile(__dirname + '/runtime.json');

Config.extend(object, source)

Merges object into the internal configuration object and puts a new source into Config.sources with the given source. (source is optional and defaults to config.extend())

const config = require('config')
  .extend({some: {key: 'value'}})
  .extend(JSON.parse(process.env.MY_APP_CONF), '$MY_APP_CONF');

Config.loadDefaults(legacy)

Applies the options provided to the instance constructor on Config.loadFiles(options) and run strictness validation. If legacy is true then it also extends the configuration object with $NODE_CONFIG_JSON and --NODE_CONFIG_JSON, if available.

const config = require('config')
  .loadDefaults()
  .parseFile(`${process.env.HOME}/.config/services.json`);
console.log(config.get('services.db.host'));  // localhost

Config.collectConfigFiles(options)

Used internally by Config.loadFiles(options).

This method is the heart of the library which holds the heuristics for collecting configuration files given a set of options. It returns an array containing matching configuration files according to their resolution order.

Accepts an options object with the following keys:

• configDir - path of the configuration dir
• appInstance - appInstance used to match files
• environment - environment used to match files, defaults to "development"
• hostname - hostname used to match files, defaults to os.hostname()

See Config.loadFiles(options) for an example.

Config.subModule(moduleName)

Sub-modules is a mechanism to create new configuration instances which are derived from their initiating module instance. The moduleName is also the path in which our sub-module is located. If the path already exists it will be used, otherwise we create the path set an new object at the end of it.

Sub-modules shares reference between their config and their main-modules config properties.

const config = require('config');
const subModule = config.subModule('TestModule');
console.log(config.get('TestModule') === subModule.config);  // identical

Notice that moduleName can contain dot notations to describe a deeper path.

const config = require('config');
const subModule = config.subModule('TestModule.Suite1');
console.log(config.get('TestModule.Suite1') === subModule.config);  // identical

Sub-modules also share sources with their main-modules. Any mutation executed from a sub-module will register the module property on the source item containing the module's name, and the data propery will emulate the sub-module's path.

const config = require('config').loadDefaults();
const subModule = config.subModule('TestModule').extend({defaultKey: 'defaultValue'});
console.log(config.sources === subModule.sources);  // identical
console.log(config.sources);

Would output:

[
  {source: '/path/to/config/default.js', data: {/* ... */}},
  //  ...
  {module: 'TestModule',
   source: 'config.extend()', 
   data: {TestModule: {defaultKey: 'defaultValue'}}},
]

Parser

I took some opinionated decisions that we can open a discussion about.

I removed the register methods for TypeScript and CoffeeScript. I don't think that's an action we should take, interfering with the registration options of a transpiler. Users of TypeScript and CoffeeScript should be familiar with these concepts. We basically treat these extensions as regular JavaScript files, and we expect the users to register these before they call the node-config.

I removed our support for multiple packages at the same parser function. Since it's now very easy to override parsers I think we should only offer the most common and let anyone who wants another package simply switch the parser.

I added a POC of a template engine as a default middleware (also a way to show off middleware). I'd like to discuss the idea further since this is only a POC version.

Parser.parse(filename)

Used internally by the mutation method Config.parseFile(filename).

• Extracts the extension from filename
• Calls the matching parser according to the extension
• Runs the result through our middleware and reduce the results
• Returns the result

Parser.readFile(filename)

Reads a file and returns its content if all validators successfully passed, otherwise it returns null.

Used internally by parsers to read a file and pass it to the parsing library.

lib.iniParser = function(filename) {
  const content = this.readFile(filename);
  return content ? require('ini').parse(content) : null;
};

Parser.readDir(dirname)

Returns an array containing the content of the dirname directory.

lib.dirParser = function(dirname) {
  return this.readDir(dirname).map(filename => {
    filename = Path.join(dirname, filename);
    return [filename, this.parse(filename)];
  });
};

Parser.collectFiles(configDir, allowedFiles)

Used internally by Config.collectConfigFiles.

// returns an array of matching filenames within __dirname
// keeps files order even when multiple directories are given
config.parser.collectFiles(`${__dirname}/http:${__dirname}/www`, ['index.html', 'index.htm', 'index.php', 'index.js']);

Parser.validators

An array of validators that are executed as part of Parser.readFile(filename).

Defaults to:

validators = [
  lib.validators.gitCrypt(!utils.getOption('CONFIG_SKIP_GITCRYPT')),
]

You can override the array to replace or clear the active validators.

const config = require('config');
config.parser.validators = [];
config.parser.validators = [
  // validate content is unicode
  (filename, content) => isUnicode(content),
];

Parser.middleware

An array of middleware that are executed as part of Parser.parse(filename).

Defaults to:

middleware = [
  lib.middleware.configTemplate(lib.middleware.configTemplate.defaultHandlers),
]

Important! The configTemplate middleware is only a POC at this point and should not be counted on. It is used here only as an example, but structure and functionality (or existence) may change.

You can override the array to replace or clear the active middleware.

const config = require('config');
config.parser.middleware = config.parser.middleware.concat([
  // check if es-module and has default then return content.default
  (filename, content) => content.__esModule && content.default ? content.default : content,
]);

Parser.definition

An array of parser definitions that are used with both Config.collectConfigFiles(options) and Parser.parse(filename).

It is recommended not to handle this property directly. Instead you can use Parser.set(ext, parser) and Parser.unset(ext) to add, remove and override parser, and Parser.resolution to set the resolution order in a simple manner.

Defaults to:

definition = [
  {ext: 'js', parser: lib.jsParser},
  {ext: 'ts', parser: lib.jsParser},
  {ext: 'json', parser: lib.jsonParser},
  {ext: 'json5', parser: lib.jsonParser},
  {ext: 'hjson', parser: lib.hjsonParser},
  {ext: 'toml', parser: lib.tomlParser},
  {ext: 'coffee', parser: lib.jsParser},
  {ext: 'iced', parser: lib.jsParser},
  {ext: 'yaml', parser: lib.yamlParser},
  {ext: 'yml', parser: lib.yamlParser},
  {ext: 'cson', parser: lib.csonParser},
  {ext: 'properties', parser: lib.propertiesParser},
  {ext: 'xml', parser: lib.xmlParser},
  {ext: 'ini', parser: lib.iniParser},  // this is new!
  {ext: 'd', parser: lib.dirParser},  // this is new!
]

Parser.resolution

A dynamic property that returns an array of parser.definition extensions. It's the same as executing parser.definition.map(def => def.ext). What's special about this propery is its setter which allows for a simple way to filter out and reorder parser.definition.

const config = require('config');
config.parser.resolution = ['js', 'json', 'json5', 'yaml', 'd'];

This removes parsers who are not included in the array and set this order for the existing parsers. Note that setting an array with an undefined (unknown) extension would result in an exception.

Parser.set(extension, parser)

Adds, or overrides in case it exists, a parser to Parser.definition.

const config = require('config');
const hoconParser = require('hoconparser');
config.parser.set('hocon', function(filename) {
  const content = this.readFile(filename);
  return content ? hoconParser(content) : content;
});
// autoload will now support hocon files

Parser.unset(extension)

Removes a parser from Parser.definition by extension.

const config = require('config');
// remove support for yaml and yml files
config.parser.unset('yaml').unset('yml');

Parser.reset()

Clears all default

const config = require('config');
config.parser.reset();
console.log(config.parser.validators);  // []
console.log(config.parser.middleware);  // []
console.log(config.parser.definition);  // []
console.log(config.parser.resolution);  // []

Parser.lib

A frozen object containing all the default parsers, validators and middleware.

const config = require('config');
config.parser.set('rss', config.parser.lib.xmlParser);  // equals to
config.parser.set('rss', function(filename) {
  return this.lib.xmlParser(filename);
});

Parser.lib.validators.gitCrypt(strict)

lib.validators.gitCrypt = strict => function(filename, content) {
  if (/^.GITCRYPT/.test(content)) {
    if (strict) throw new Error(`Cannot read git-crypt file ${filename}`);
    console.warn(`WARNING: skipping git-crypt file ${filename}`);
    return false;
  }
  return true;
};

Parser.lib.middleware.configTemplate(commandHandlers)

Okay, first the repeating disclaimer that this is just a POC.

I'd created an extremely simple, case-insensitive DSL that allows piping of command and applying flag modifiers.

It currently contains two commands and three options.

• "File::" - Command used to read a file using Parser.parse(filename). Returns an object
  • "string" - Modifier used to read the file using Parser.readFile(filename) instead. Returns a string
• "Env::" - Command used to set value of corresponding environment variable
  • "json" - Modifier used to parse the value using JSON.parse(str)
• "secret" - Modifier that can be used on any value to defines its property as non-enumerable. Replaces what used to be config.util.makeHidden()

example.json5:

{
  dbHost: 'localhost',
  dbPass: 'ENV::DB_PASSWORD',
  httpServer: 'ENV::HTTP_SERVER|json',
  ssl: {
    key: 'FILE::/credentials/ssl.key|string',
    pem: 'FILE::/credentials/ssl.pem|string',
  },
  runtime: 'FILE::ENV::CUSTOM_RUNTIME_JSON_PATH',
  secretKey: 'ENV::SPECIAL_SECRET_KEY|secret'
}

example.yaml:

dbHost: localhost
dbPass: ENV::DB_PASSWORD
httpServer: ENV::HTTP_SERVER|json
ssl:
  key: FILE::/credentials/ssl.key|string
  pem: FILE::/credentials/ssl.pem|string
runtime: FILE::ENV::CUSTOM_RUNTIME_JSON_PATH
secretKey: ENV::SPECIAL_SECRET_KEY|secret

Related issues

Solved or no longer relevant due to new architecture:

• #549 - Immutability
• #546 - SubModules
• #543 - SubModules
• #517 - SubModules
• #514 - Immutability
• #478 - Obsolete
• #472 - Obsolete
• #471 - Immutability
• #424 - SubModules
• #412 - Obsolete
• #338 - Obsolete
• #329 - Immutability
• #226 - SubModules
• #225 - SubModules

We can provide a middleware solution:

• #539 - Middleware
• #536 - Middleware
• #521 - Middleware / Parser
• #400 - Middleware
• #372 - Middleware
• #324 - Parser

[lorenwest]

How do you propose migration of existing users?

If you could install this and use features as-needed vs. rewrite, I'd be supportive.

[markstos]

Breaking changes can considered as part of different categories:

1. Already deprecated. No action needed.
2. Expected to be used a very small number of users. Document the change.
3. Expected to be used by a significant number of users. Provide a plugin, clear migration path, or just don't break it.

Given that framework, here's my specific feedback on the breaking changes:

Removed runtime.js support.

it was deprecated in 2014 when there was a push towards immutability. There have been no bug reports about this since 2014. I think it's fair to consider it "not used".

Removed custom-environment-variables.* support

A proof-of-concept for a replacement was provided. I would like to a 1-to-1 plugin or built-in feature provided that continues to support the feature. The PoC demonstrates this could be done with a custom middleware parser.

Changed location of asyncConfig and deferConfig

This two changes can be considered separately. asyncConfig is so new that backcompat is not a concern.

defer() is somewhat widely used. I'm OK with this, but it also seems fairly easy to leave shim files at config/async.js and config/defer.js (and config/raw.js?). The old structure can be deprecated during one major release and removed in later release. The docs can immediately updated to highlight the new recommended syntax so new users start using the new syntax immediately.

I realize it's a one-line change in a config file, but I also see that in my project I have over 100 config files where I would need to make the one line change. :)

get() and has() are available only on the config instance

I think I use this feature somewhere. Without this feature, some people will restore to raw property access: ex: db.host. That's worse because it won't catch a typo like db.hots, or db.cluster.hosts might throw an error where db.has('cluster.hosts') might throw an error.

This feature is not essentially and I realize removing it may simplify some things, but it does have benefits. Can it be implemented as a plugin?

Other feedback (related to get() and has() on sub-objects)

• The docs for Config.create should be updated to document which options are required
• What's the recommended way to create a new config object where you have the configuration data as an object instead of in files? Should there be an option to Config.create() for this purpose that ignores files all other? Such a solution might be an alternative to having get() and has() on sub-objects. For example:

const dbConfig =  config.create({ config: config.get('customer.db') })
dbconfig.get('hosts')

getArgv() slightly differs from its predecessor

This behavior was probably "undefined" before. Impacted user s are expected to a very small number. The new behavior does seem more sensible.

[iMoses]

If you could install this and use features as-needed

@lorenwest I'm not sure what you mean by that.. I listed all the breaking changes so we can discuss the necessity and migration process of each, and most of them demostrate a way to reproduce the same functionality.

BTW, I'm under the assumption that most users are not using any of the advanced features, meaning that they won't have any migration process at all!

Removed config.util methods

Most effected users will be power users who have used config.util methods. If I userstand correctly, the most commnly used methods are serModuleDefaults and loadConfigFiles both of which are now natively supported and provide better stability and finer control using Config.loadFiles(), Config.parseFile(), Config.extend() and Config.subModule().

Removed runtime.json support

I agree with @markstos, this one is already depracated for a long while, and a user can easily use Config.parseFile() to migrate their code and continue using this feature.

Removed custom-environment-variables.* support

This is the biggest change and I think we should discuss the template engine a bit before deciding on the migration process for this feature. Honestly, this is the only breaking change I'm not really sure what to do with, but i truly believe that we should provide a more consistent solution via a template engine.

get() and has() are available only on the config instance

@markstos your second suggestion is actually already supported, you were just looking at the wrong feature because that's exactly what Config.subModule() is for:

const dbConfig =  config.subModule('customer.db');
dbconfig.get('hosts')

Others

What's left is getArgv() which shouldn't really effect anyone and is basically a bug fix in my opinion, asyncConfig and Parser changes, both so new that other than early adopters we won't acctually break anything.
I added shim files with depracation warning to config/async, config/defer and config/raw to ease the migration process.

[iMoses]

@lorenwest I added back support for custom-environment-variables.* as @markstos suggested.

It's now part of the loadDefault(legacy) legacy mode (that's the autoload default) and there's a special middleware for dealing with matching filenames that is set by default. One less thing to worry about.

That basically leaves us with two major changes.

• get() and has() will not be set on values. The sub-module mechanism can cover for it
• config.util is gone - that's a big part of the motivation for this change

Is this good enough? Any input?

Plus, I was hoping we'd discuss the template engine here, meaning I wanna hear if you think support these options is enough or do you have any other ideas for a template engine. for example, the current implementation doesn't support partials and can apply "secret" only to env-vars and files.

I know we're all working people and you don't have much time for this, but I'd appreciate any sort of a response that can help us move forward.

Config.prototype.loadDefaults = function(legacy = false) {
  this.loadFiles(this.options);
  if (legacy) {
    const extendWith = (value, source) => {
      try { value && this.extend(JSON.parse(value), source); }
      catch(e) { console.error(`${source} is malformed JSON`); }
    };
    // these lines are new, to handle custom-environment-variables
    const legacyFiles = [];
    this.parser.resolution.forEach(extName =>
      legacyFiles.push(`custom-environment-variables.${extName}`));
    this.parser.collectFiles(this.options.configDir, legacyFiles).forEach(this.parseFile);

    extendWith(process.env.NODE_CONFIG, '$NODE_CONFIG');
    extendWith(utils.getArgv('NODE_CONFIG'), '--NODE_CONFIG');
  }
  strictValidations(this.sources, this.options);
  return this;
};

// new middleware, set by default
lib.middleware.customEnvironmentVariables = function(filename, content) {
  if (Path.basename(filename).startsWith('custom-environment-variables.')) {
    utils.collect(content, Boolean).forEach(([ value, key, object ]) => {
      if (typeof value === 'string') {
        object[key] = process.env[value];
      }
      else if (value.__format === 'json') {
        try {
          object[key] = JSON.parse(process.env[value.__name]);
        }
        catch(e) {
          throw new Error(`Failed to parse "${value.__name}" environment variable`);
        }
      }
    });
  }
  return content;
};

I hope this also demonstrates how easy it is to provide solutions for these problems with the new infrastructure :)

P.S. would you prefer it if I make a PR at this point?

[sudosoul]

Haven’t reviewed yet, but +1 for thinking on custom-environment-variables.* support

…

On Jul 31, 2019, at 7:31 PM, iMoses ***@***.***> wrote: @lorenwest I added back support for custom-environment-variables.* as @markstos suggested. It's now part of the loadDefault(legacy) legacy mode (that's the autoload default) and there's a special middleware for dealing with matching filenames that is set by default. One less thing to worry about. That basically leaves us with two major changes. get() and has() will no be set on values. The sub-module mechanism covers for it config.util is gone - a big part of the motivation for this change Is this good enough? Any input? Plus, I was hoping we'd discuss the template engine here, meaning I wanna hear if you think support these options is enough or do you have any other ideas for a template engine. for example, the current implementation doesn't support partials and can apply "secret" only to env-vars and files. I know we're all working people and you don't have much time for this, but I'd appreciate any sort of a response that can help us move forward. Config.prototype.loadDefaults = function(legacy = false) { this.loadFiles(this.options); if (legacy) { const extendWith = (value, source) => { try { value && this.extend(JSON.parse(value), source); } catch(e) { console.error(`${source} is malformed JSON`); } }; const legacyFiles = ['custom-environment-variables']; this.parser.collectFiles(this.options.configDir, legacyFiles).forEach(this.parseFile); extendWith(process.env.NODE_CONFIG, '$NODE_CONFIG'); extendWith(utils.getArgv('NODE_CONFIG'), '--NODE_CONFIG'); } strictValidations(this.sources, this.options); return this; }; lib.middleware.customEnvironmentVariables = function(filename, content) { filename = Path.basename(filename); if (filename.startsWith('custom-environment-variables.')) { utils.collect(content, Boolean) .forEach(([ value, object, key ]) => { if (typeof value === 'string') { object[key] = prototype.env[value]; } else if (value.__format === 'json') { try { object[key] = JSON.parse(prototype.env[value.__name]); } catch(e) { throw new Error(`Failed to parse "${value.__name}" environment variable`); } } }); } return content; }; I hope this also demonstrates how easy it is to provide solutions for these problems with the new infrastructure :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[iMoses]

• What's the recommended way to create a new config object where you have the configuration data as an object instead of in files? Should there be an option to Config.create() for this purpose that ignores files all other? Such a solution might be an alternative to having get() and has() on sub-objects. For example:

@markstos BTW, I forgot to give you a direct answer about that.

The autoload mechanism only functions if none of the mutation methods were called. Once called you must call loadDefaults(legacy) manually to reproduce its effects when you need them.

If someone wants to use the library without files the recommended way is to use the mutation methods: config.extend(object), config.parseFile(filename) and config.loafFiles({configDir, environment, appInstance, hostname}).

const config = require('config');
config.extend({a: 'b'});
config.get('a');
// no files were loaded!

OR

const config = require('config').create();  // not the default instance!
config.extend({a: 'b'});
config.get('a');
// no files were loaded!

FYI, There are technically three undocumented options that Config support: config, module and parser. These are used internally as part of the sub-module mechanism and might be used in the future by external tools to extend this library, but I don't want to advertise them because I consider them harmful.

[lorenwest]

You're right that we're all working people and have lives, and frankly this is a bit overwhelming. This amounts to a major rewrite with multiple design changes, many of which weren't discussed until this thread.

I can't get past the starting gate - why? I understand your motivations and aren't in the same place as you with regard to the need for this much change.

For example - sub-modules. If you see the solution to subModules as multiple instances, then multiple instances could be used as motivation for a change. But multiple instances aren't how my company solved for sub-modules, so at least in my case, multiple instances are a distraction at best, and un-necessary complexity at worse. It changes client code by requiring them to have independent variables for each sub module. This may (or may not) be cleaner, but in my opinion that ship has sailed, and forcing people to change is inappropriate.

Middleware and configurable configuration. If you think it's good to have one config experience at one company (or one project) and a different configuration experience at a different company (or project), you may think pluggable middleware and pluggable template engines are a good thing. For configs I happen to think consistency trumps flexibility, so I'm not convinced the config library needs to be infinitely configurable like other .js libraries. Those libraries need configurations found in node-config to configure them. At what point will someone see the need to write a config library for node-config?

Before embarking I suggested we discuss issues independently - you may have gotten a feel for the direction of node-config before embarking on such an ambitious project.

I understand you've put a lot of thought into these solutions, and truly appreciate your effort. There's a lot of goodness here. In my opinion the best way to move forward is to use this thread as a framework for discussion of individual features, and add functionality one feature at a time. Most users of this library aren't screaming for immediate change.

[iMoses]

^ Most users of this library aren't screaming for immediate change.

@lorenwest I truly don't understand you.. Let's start with the fact that most users of this library won't even feel this change. The default heuristics of the library remains (99%) intact and users who have no need for the advanced features won't be affected at all. All tests still pass (excluding the removed config.util).

So we're only talking about the power users who requested these features to begin with. Half of the open issues here, some dating back 4-5 years, relate to the patchy implementation of immutable config which still causes multiple errors for people using the config.util methods (including setModuleDefaults) and many issues which are related to the fact that the library always provide a single instance and loads only with external options from env-vars and cli-args, which makes this library unusable for package owners who don't wanna pollute user configuration in case they too wanna use this library and unstable for people who have to override and revert env-vars just to change configuration from within nodejs.

I don't think that anyone should wait until they must push "immediate changes" and have a major release. I'm looking at new PRs and I see that most of them are very patchy, not because contributers are bad programmers but because the library is currently in a bit of a mass. We have a lot of util methods, most of which violate the DRY principle and could have been merged into a single utility which I have shown in my code. The new parser feature, although better than nothing, is far from being comfortable, and these changes I'm offering truly address each and every one of these issues and then some.

Of course it's a major change and requires a major version, it's meant to take this library to the next major stage - supporting programmatic configuration and improving the API. This is my proposal and this is the main issue I wanted to discuss, but if you wanna start discussing other matters I'm all in, let's talk.

Now we can discuss anything you'd like, but honestly it feels like I need to force a response out of you on these matters. For example, why only give examples and not start listing the subjects you'd like to discuss according to priority so we can embark in that discussion?

I'm not attached to the code but I do think it makes things easier that I can simply show you what I thought about and we can go from there. You can even play with the library, install it locally to judge the changes yourself, there's no downside to it other than my lost time that shouldn't be your concern.

Let's start with the burning issues and move downwards from there.. I'm still unsure from your responses what is your exact opinion on each of these changes.

P.S.

• About your sub-modules note please review Config.subModule and not Config.create. The method doesn't change the logic and although it does create a new instance, the instance shares it's configuration object and sources with its main module, so it isn't independent in any way.
• Regarding middleware, I view it as a better internal mechanism that we can or cannot expose so that power users would be able to modify it. We can chose not to allow external editing of middleware and I still see it as a good feature which make for a better infrastructure.

Notice that I'm and was very cooperative with all of your requests, not just on this issue. I don't mind the feedback, I appreciate it and want to make you sure about these change before agreeing to anything. I just need your cooperation with flagging your concerns so we can discuss/solve them and move forward.

[iMoses]

There's one more point I want to make..

The biggest breaking change here is replacing config.util with a stable API (anything else we can revert). You may see it as big changes, but technically we're just providing the same functionality we had with config.util methods but in a more stable and consistent manner. I honestly believe that this deserves a major release.

[markstos]

@lorenwest I agree that "node-config shouldn't need a config", but I don't see that being proposed here. What I do see is a fresh approach that addresses whole categories of bug reports we receive. About two dozen have been carefully documented. If we didn't want node-config to address those issues, why have we left those two dozen issues open, waiting for someone to move them forward?

This is a lot of change at once, but when a problem is approached with a new design an incremental approach is not a good fit.

Your mentioned your company went another direction with sub-module support. Is there is a specific alternate approach that you propose as an alternative what's laid out here?

This is a solid "next-gen" proposal for node-config and avoiding a fork would allow us to pool maintenance effort.

[lorenwest]

Here's a proposal for the rollout:

Phase 1 - code restructure. Separate code structure changes into one or more minor releases with 100% backward compatibility. This forms the foundation for the work requiring major revision change, and gets it installed and street tested independently. It forces us to separate features requiring a major revision from those we can introduce without disruption.

Phase 2 - deprecation warnings. Another point release introducing (squelchable) log statements announcing deprecation for incompatible usage in the upcoming major revision.

Phase 3 - major release. Bundle features/commits requiring a major release into one NPM publish. By this time we've discussed each feature independently, and have a clear understanding of the cost/value of introducing the features as non-backward compatible. Also, a fair amount of time has passed with the code restructure in the wild that new issues are reduced to new features.

I appreciate your guidance on the code restructure, and think the phase 1 changes can go through quickly. My company has a deep understanding and appreciation for the value of sub-module configs, and I want to start a separate thread on that feature. It may or may not need a major revision update.

The motivation for this proposal is to introduce separate concerns into a managed rollout. This allows us to clearly understand each feature, mitigate risk, and minimize community disruption.

ps: @iMoses - for some reason github doesn't recognize you as a contributor if branches from a different repository are merged into this repository. I suggest future pull requests to come from branches of this repository so the auto-generated contributor list recognizes you as a core contributor.

[iMoses]

ps: @iMoses - for some reason github doesn't recognize you as a contributor if branches from a different repository are merged into this repository. I suggest future pull requests to come from branches of this repository so the auto-generated contributor list recognizes you as a core contributor.

This is a bug caused by me pushing changes from an email address that wasn't registered in Github (my work email). I fixed it but I have a feeling that the contributers are cached and will change only on the next commit to this repository. (hover the contributers above the code and see that I appear there)

Regarding your proposal, I'll see what can be done, though most of the new features relay on the files being lazily loaded, which in turn requires the extraction of the configuration object from the root of the instance into a separate property, and that's already a breaking change. Without this feature we can't use the new parser, meaning no middleware and validators, no mutation methods, no create or sub-module methods... I'm not sure what else will be left.

I can see how we can slowly add new functionality, instead of publishing it all at once, but it seems that the initial requirement for it all is a breaking change by itself and that would have to be one of the first things to be fixed/changed.

If you look at the new API it may seem big (although it's less code then before the change), but if you see what was actually changed we're not talking about too much, and anything I was able to fix without this change I already did (new parser, async-config, deferred bug, multiple dirs..). From the top of my head it seems that the only thing I can add to the current version is support for ".d" extension for directories and making "raw" obsolete by skipping complex object entirely.

Also notice that depracation warnings were already added for the change in location of async, defer and raw and backward compitability is automatically enabled for most features. We're only talking about removing config.util and not having get() and has() on configuration values prototypes.

[iMoses]

@lorenwest honestly, I'm going over the code to see if there's anything I can push before the breaking change and I think the code is too messy to push anything "next-gen" without breaking something. If you prefer to have multiple minor breaking changes than a single big change that can be done, but I don't see the benefit of it, quite the opposite.

I can't move util to a different file because attachProtoDeep references the Config class directly and changing this would break both its API and setModuleDefaults API which relay on it. If I try to add the new dir parser it relays on loadFileConfigs but doesn't require all of it's logic, so we need to break that one as well.. I also can't add support for ".d" directories extension without duplicating more logic or creating more utility methods, and can't make "raw" obsolete without losing support for get() and has() on values prototypes.. A big part of my reason for this rewrite was to refactor all spaghetti code, and we have a lot of it..

We can introduce many minor methods and expose a lot more functionality to get there, but this will only increase the size of the change once we push to a new version because none of the "next-gen" functionality can be implemented without breaking anything..

The only thing I can do is update defer and async which stayed mostly the same. I agree with @markstos that "when a problem is approached with a new design an incremental approach is not a good fit". I think it's only gonna make things more complicated both for us and the users.

There's another option which I dislike but thought maybe you'd prefer.. I can try and shim important methods from config.util with the new methods to reduce even further the breaking changes and add deprecation warning to these as well.

Perhaps the best approach it to go out with the breaking change, considering we're still passing all tests, shim whatever we can and postpone new functionality to later minor releases (such as support for ".d"). That way we introduce as little changes as possible, even though we're still breaking a part of the API, and continue with additions and removal of deprecated methods later on..

Maybe writing really long docs is not the best method for us to discuss this.. If you're up to it we can have a quick call and I'll present to you in details my solutions..

[markstos]

@iMoses Thanks for thinking through how an incremental approach might work in this. It sounds painful, and ultimately results in a breaking-change release anyway. +1 for getting on a call with Loren West to discuss in real time.

[markstos]

@iMoses Could you document the signature for validators? The docs for Parser.validators say that it is an "array of validators", but I didn't see the definition of validation. I presume it's a function but could you document the signature? Thanks.

[iMoses]

@iMoses Could you document the signature for validators? The docs for Parser.validators say that it is an "array of validators", but I didn't see the definition of validation. I presume it's a function but could you document the signature? Thanks.

validators and middleware

Both are arrays of functions that are automatically executed at different parts of the parser's pipeline. Just like parser functions, they accept filename and content as arguments and are called in the context of the parser instance.

Validators are executed as part of the readFile(filename) method. After a file was read and its content extracted all of the validators are executed. In case all validators returned true the content of the file will be returned, but if any of the validators returned false the operation will silently fail and null will be returned by readFile(filename). It is also possible to fail "loudly" by throwing an exception from a validator function.

Note that validators are only executed when reading files using readFile(filename), meaning that JS files which are imported using require(filename) are currently not validated. (that's my opinionated decision which can be discussed)

And example of a validator: (notice that gitCrypt is a validator creator)

lib.validators.gitCrypt = strict => function(filename, content) {
  if (/^.GITCRYPT/.test(content)) {
    if (strict) throw new Error(`Cannot read git-crypt file ${filename}`);
    console.warn(`WARNING: skipping git-crypt file ${filename}`);
    return false;
  }
  return true;
};

Middleware are very similar except that they are always executed after the parser methods and are expected to return the parser's content. They are executed as part of the parse(filename) method.

[markstos]

@iMoses Validators are a great feature. Recently we had a significant bug in production that a validator could have caught. We needed logic to validate configuration: "If we disable the frontend for this feature, the backend should always be disabled".

On top of this API we could build a config schema validator.

In Perl I used to maintain schema validation module that had a pretty good API. You can see the docs for it below. I'll link to a section that focuses on dependencies, which node-config currently has no notion about:

https://metacpan.org/pod/Data::FormValidator#dependencies

You can see there are a number of options for declaring dependencies. You can declare a dependency on a single field, or on a group of fields. You can declare that fields that match a RegEx must have particular dependencies. You can also declaration optional dependencies or that a dependency must have "some" of a list of fields to be present.

The same module also handles "constraints"-- requirements that an email value must be an email format and so on.

When complex configurations are maintained by humans, a validating middleware could be very helpful.

A middleware patterned after Perl's Data::FormValidator would allow expressing the validation logic in a declarative syntax.

[iMoses]

@markstos side-note: I'm still experimenting and adjusting the API..

I decided to test a different signature for the validators and middleware by simply reversing the arguments order. Most of these mechanisms relay solely on the file's content and the filename is needed only in case we wanna throw a details error, so it makes sense to me to put the content first in case validators or middle don't need the filename at all.

I also improved the parser mechanism a bit. If a parser function takes one argument it's gonna get the filename alone, but if it take more than one then the second argument will be the content of the file from Parser.readFile to prevent unnecessary boilderplate code. In case it takes the files content, a parser will only execute if content has passed validations.

Ragrding your proposal, it sounds like a classic case for middleware..

[markstos]

Those sound like good refinements.

…

On Wed, Aug 7, 2019 at 5:07 AM iMoses ***@***.***> wrote: @markstos <https://github.com/markstos> side-note: I'm still experimenting and adjusting the API.. I decided to test a different signature for the validators and middleware by simply reversing the arguments order. Most of these mechanisms relay solely on the file's content and the filename is needed only in case we wanna throw a details error, so it makes sense to me to put the content first in case validators or middle don't need the filename at all. I also improved the parser mechanism a bit. If a parser function takes one argument it's gonna get the filename alone, but if it take more than one then the second argument will be the content of the file from Parser.readFile to prevent unnecessary boilderplate code. In case it takes the files content, a parser will only execute if content has passed validations. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#569>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAGJZNLZE4X577F7U5ZBCLQDKGFTANCNFSM4IHJKYMA> .

-- Mark Stosberg Director of Systems and Security RideAmigos

[nolazybits]

Hello team,
any news on this?

[iMoses]

@nolazybits There's currently no real interest in refactoring the library's API, which makes this proposal obsolete as it relays on breaking changes to solve related issues..

I'm using my own fork on projects I'm involved with, and if there's interest from the community for this version I can publish my fork as a new library, but I don't think this proposal will be approved as part of this library any time soon..

[nolazybits]

@iMoses I'll be very keen please. Happy to help too

[markstos]

As one of the maintainers, I can see there is interest in refactoring the library's API. @iMoses proposed a number of interesting changes, so this large conversation got split into some more focused conversations on specific features. One was submodule support. You can see there was a fairly productive conversation about solving that hard problem here: #572 with a thread that ran 35 comments deep when the conversation dropped off.

One of the last comments from @iMoses was "life happened, my company closed down and I don't have as much time". Around that time, all conversation about module updates stopped.

One of the challenges in the collaboration was that "sub-module support" is a recurring feature request that as maintainers we'd like in a "next gen" edition of the module. However, @iMoses took the lead on a proposed overhaul, but didn't personally need the "sub module" feature so lost energy for contributing to that conversation. Understandable!

The conversation can revived by reviewing the various related issues here.

Forks are welcome as well. No single config module is going to suit everyone.

[nolazybits]

Hey @markstos thanks for the comment.
The main feature I am looking at for now is #472
I like also the validation aspect (although I have implemented it, it would be nice to be part of this libary)
So I am keen to use @iMoses lib if he is publishing it to NPM and keen to help either him on his effort or here if needed if you guys consider this PR :)

Thanks again to all of you for this nice lib!

[nolazybits]

Hey @iMoses my email is on my profile, could you please send me an email so we can start talking about the next-gen.
I'm using the next-gen on a framework I am building and it works really well 👍
Thanks

[nolazybits]

This is really a nicer way to setup the config.
I would really like to see this work merged here. For now my project is using iMose github repo but would love to use the official one... Any luck?

example of config.json
default.json

{
  "nodebrick-core": {
    "logger": {
      "mode": "local",
      "log_level": "warn"
    }
  },
  "nodebrick-api": {
    "web_port": "ENV::NODEBRICK_API_APP_PORT"
  },
  "nodebrick-database": {
    "connections": [
      {
        "name": "default",
        "database": "ENV::NODEBRICK_DATABASE_POSTGRES_DATABASE_NAME",
        "host": "ENV::NODEBRICK_DATABASE_POSTGRES_HOST",
        "port": "ENV::NODEBRICK_DATABASE_POSTGRES_HOST_PORT|json",
        "username": "ENV::NODEBRICK_DATABASE_POSTGRES_USERNAME",
        "password": "ENV::NODEBRICK_DATABASE_POSTGRES_PASSWORD",
        "schema": "ENV::NODEBRICK_DATABASE_POSTGRES_SCHEMA_NAME",
        "type": "postgres"
      }
    ],
    "connectionOptions": [
      {
        "name": "default",
        "namingStrategy": "SnakeCaseNamingStrategy",
        "synchronize": false,
        "migrations": [
          "./migrations/*.{ts,js}"
        ],
        "migrationsRun": true
      }
    ]
  }
}

development.json

{
  "nodebrick-core": {
    "logger": {
      "log_level": "debug"
    }
  },
  "nodebrick-database": {
    "connectionOptions": [
      {
        "synchronize": true,
        "migrationsRun": false
      }
    ]
  }
}

I can override nested properties, I can use ENV var or not depending on my environment, ...

Please add this proposal 🙏

[garrettmac]

love this!

[Codermar]

Hey @iMoses. Have you made your fork public yet? Has it been published? If so, can you please share? Thanks.

[nolazybits]

Anyone, any news on this please?

[hershmire]

Is there any update around this? Looking through your issues, it appears discussion around big changes has been going on for a few years now but I haven't seen any resolution to a direction or future plans. Are there any plans for bigger changes/improvements or is this library in maintenance mode at this point? I've used this library extensively at some larger companies and now faced with deciding whether this is still the best option moving forward at a different place. Just looking for some idea on this lib's future.

[iMoses]

No updates. If you wanna help making this version a separate library you have my blessing and some help, but unfortunately I do not have enough time to take care of everything an open-source library maintenance requires.
Furthermore, this version was paused mainly for trying to solve the "multi-module" problem while my approach today would've been to drop this feature entirely and perhaps offer it as a separate library, even an extension.

[markstos]

Are there any plans for bigger changes/improvements or is this library in maintenance mode at this point?

The project is community-maintained at this point. It works well enough for the busy maintainers. We are open to reviewing and publishing improvements, but if you want a feature to be released in node-config, expect to build, test and document it after some discussion that it's desirable. If large companies are using it extensively, it seems they ought to be willing to pay dev to make sure it meets their needs.

[jdmarshall]

I've tried on a couple of occasions to factor out parts of config.util to not need 'this' to equal config, and it's sticky.

There's a lot of code in this file that would be better served as standalone, pure functions. It would vastly simplify the unit testing situation. With less state it's easier to cover the cartesian product of different conditional blocks.

The coupling between loadConfigs and configSources is but one example. Getting the data off disk and storing it into the 'database' should be two separate operations.

That would also make it easier for people to write integration tests for their config directories. For instance I have code that checks for unresolvable DNS names, and right now that involves duplicating functionality of node-config in order to do so.

[iMoses]

@jdmarshall are you talking about my next-gen version or the existing one? Because I'm pretty sure I decoupled the utils but feel free to prove me wrong

[jdmarshall]

Because I'm pretty sure I decoupled the utils but feel free to prove me wrong

The file scoped variables mean they are still coupled. Utilities are better specified as pure functions that take in state and return it, not accumulate it off stack in or for some other object.

The code for accumulating the config state can be in config.js, but loading and finding should be utilities, divorced from the config object. Once that exists, you can break the singleton behavior of the current code.

[iMoses]

@jdmarshall I have no idea what you're talking about and how does it relate to this proposal. Seems like you're talking about the existing code base.

[jdmarshall]

Updated for context.

How are you going to talk about improving an architecture without discussing (the moving target of) the current architecture?

I like most of what you’ve done with lib/utils. There’s a bunch of mostly pure functional code in config.util that’s interrupted by shared state mutation, those needed to be teased apart.

For our application we’re trying to load defaults for close to 100 modules. I’ve already filed a couple of PRs to reduce re-entrance bugs that we ran into via someone else either not knowing about setModuleDefaults or running into the same bug I filed a PR for last week. The biggest problem was singleton nature of the code. I have a workaround that loads node-config twice, ones for ./config and once to pull in all of the defaults. I wouldn’t have todo that if the utility could be accessed without the singleton being involved.

[jdmarshall]

Having looked closer at your code, other than the open PR I filed to fix premature attribute locking, I only have one real critique.

Util.reduceObjects() is a function named not by what it does but how it does it. Some code style guides recommend avoiding reduce entirely, and I happen to agree. It’s hard to follow reduce() in a debugger, which hampers new people understanding whatever code is using it, versus just doing a loop. It hurts readability and produced virtually the same amount of code, so it’s not really defensible on DRY principles.

What you’re really doing there is an object traversal. You’re not reducing anything. And the main place you’re using it has a lot of state already. Splitting some of that state to a coroutine just makes it harder to fit into working memory, which is again worse for DX.

[iMoses]

@jdmarshall

1. I did discuss the current architecture in this thread and explained in details my reasons for each of these changes.
2. The utils in my code are completely pure as far as I recall, but if I'm mistaken feel free to point it out.
3. I agree that the singleton nature of the library is one of its biggest issues (also discussed here), and I address exactly that in this proposal.

[iMoses]

Having looked closer at your code, other than the open PR I filed to fix premature attribute locking, I only have one real critique.

Util.reduceObjects() is a function named not by what it does but how it does it. Some code style guides recommend avoiding reduce entirely, and I happen to agree. It’s hard to follow reduce() in a debugger, which hampers new people understanding whatever code is using it, versus just doing a loop. It hurts readability and produced virtually the same amount of code, so it’s not really defensible on DRY principles.

What you’re really doing there is an object traversal. You’re not reducing anything. And the main place you’re using it has a lot of state already. Splitting some of that state to a coroutine just makes it harder to fit into working memory, which is again worse for DX.

You are giving a code review to a 3 years old code that is obviously not gonna be used? Seriously, what's the point? No offense but I see no reason to continue this discussion.

[jdmarshall]

I presumed based on the fact that this was still open that you were still toying with that idea. Also I confess I didn't check the dates.

Any thoughts on cherry-picking some of the bits for a 4.0 release? In particular I like the constructor, and the separate util. Parser is already kind of pulled out. Do you have a wish list?

[markstos]

Lots of good ideas here and also lots of breaking changes. Perhaps best published as a mostly-compatible fork.

@lorenwest and I have been volunteering as node-config maintainers for about a decade, largely fielding issues and PRs about features we don't use ourselves. A major release with a lot of breaking changes would help a lot of folks and improve a lot of things, but it would also surely bring with it a big upswing in issue tracker traffic, which we are already not keeping up with.

Published under a new name, the maintenance load would ramp more slowly... and some other folks could take the lead on the next decade of maintenance!

[x80486]

Not much to say for the implementation, but on the usability side, would be great if environment variables could be expanded, and somehow have some syntax to make sure they are set: ${REQUIRED_ENV_VAR:?error} or to default to some value in case they are not provided: ${REQUIRED_ENV_VAR:-default_value}.

This syntax is used by Docker, hence it's already familiar for most developers — so bonus points on that.

[markstos]

@x80486 The syntax you are showing is Bash syntax, which you are welcome to use before passing environment variables to node-config if you are variables are coming from the bash shell. The feature is called Shell Parameter Expansion, and you can read about it here:

https://tiswww.case.edu/php/chet/bash/bashref.html#Shell-Parameter-Expansion

Most developers I've met are not aware of the bash shell parameter expansion feature.

node-config already includes .has() method to check if an environment variable is defined, while the cascading config system already supports setting and overriding defaults. No further changes are planned there.

[x80486]

Probably that's true for the Node.js world, but in Java and Go this is quite the standard these days.

Frameworks like Micronaut, Quarkus, and Spring Boot supports this as a built-in feature — think of it like node-config already integrated.

In Go I've tested a couple of libraries as well, like gookit/config.

While the technologies are different, I think there is some resemblance in the way they all manage configuration in the end. Basically, the external/underlying library uses YAML syntax (among others) and you can directly use environment variables, so they get expanded automatically, defaulting (or failing) to some other values, when specified:

datasource:
  url: ${JDBC_URL:jdbc:mysql://localhost:3306/db}
  username: ${JDBC_USER:root}
  password: ${JDBC_PASSWORD:}

[markstos]

If you want to use a config file format that looks like YAML but processes environment variables, some plugin could written which parses a file format like that, but it's not something that needs to be in the core node-config project.

At the top of this thread, in the section on Parsers is a comment about how it would be easier to define external parsers so people can parse different file formats like this.

You could take the work on this branch and publish as a new config module which would have this feature, then create your own YAML parser which also supports this bash-style syntax for environment variables. What the project is missing now is not such features, but people willing to the do work of maintaining one of the popular config modules for Node.