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

Already on GitHub? Sign in to your account

Gearing up for 0.5 - Feature discussion #109

Closed
lorenwest opened this Issue Feb 25, 2014 · 38 comments

Comments

Projects
None yet
5 participants
Owner

lorenwest commented Feb 25, 2014

Node-config has been on the 0.4 release for a long time, and a few features have been held back due to backward compatibility issues.

We're gearing up for the 0.5 release, and now is the time to request features that we've held off due to compatibility.

Planned for 0.5:

  • Change load ordering from hostname.EXT --> deployment.EXT to deployment.EXT --> hostname.EXT @xaka
  • Make configurations immutable by default, with a configuration to reverse this behavior @markstos
  • Add a configuration to disable runtime.json reading/writing #91 @triccardi-systran

Any others?

  • When configuration is immutable, we should open for write runtime.json, to avoid unnecessary IO and file creation.
  • If configuration is immutable by default, then we should probably not read runtime.json by default either. Shouldn't we?
    (With 0.4 too many times we have configuration files modifications not applying because of this tricky runtime.json sneaking in, it's always frustrating to loose so much time to understand the issue).
  • Feature request: database backend.
    I understand it's not trivial to implement all the features of the current other backends, but we could do this only for immutable config, it would still be really useful for example when we have multiple process but not a fancy deployment mechanism like Chef or Puppet.
    It can be done without any modification on node-config (and I can confirm it works), but it requires reimplementing the deep merge, so it's easier to have it inside node-config.
    Then updates features could be added afterward, and maybe realtime updates later if it's even feasible/pertinent.
Collaborator

markstos commented Feb 25, 2014

@triccardi-systran I agree that if configuration is immutable, there's no need to support changing it at runtime, and that would simplify some things.

My use case might also make use of a database backend. For example, we may have N customers, each with their own configurations which override a small number of values. Perhaps each customer would sign up, customize their app through a web interface, and then their custom configuration would be stored in a database.

However, we are also doing a certain amount of automation using Grunt. So potentially we could also use Grunt to read the database and write the custom configuration details to a .json file, keeping node-config simpler.

I don't have a clear idea whether DB-backend support belongs in node-config, but I do know I may soon need something like it one way or another.

xaka commented Feb 25, 2014

What about $get(path, defaultValue) and $set(path, value)?

I've just run into the issue when i'm not sure if configuration exists and so if i do var value = config.a.b.c, i'll get a runtime exception and as a workaround i'd have to write var value = ((config.a || {}).b || {}).c which is awful and var value = config.$get('a.b.c') looks better.

If defaultValue is not provided, we can raise an exception with a better message than default undefined does not have 'c' property.

Owner

lorenwest commented Feb 26, 2014

Great feature request. I'm not a fan of $ in front of functions, as I've grown to know $ as a proxy for jQuery, but get/set is appropriate for this feature.

On the other hand, I feel configurations are fairly static. They're all defined in defaults, and individual configurations are overridden in other config files. Adding support for configurations that don't exist opens the door to other things - like monitoring - for configurations that don't exist.

Thoughts?

xaka commented Feb 26, 2014

I think monitoring would be cool and helpful, for instance you might use it to change the logging level or the bandwidth. BTW, $ is for purpose of making sure config methods do not interfere with config options.

Owner

lorenwest commented Feb 26, 2014

Good point about using $ to prevent config/method clashes. Others have requested adding monitoring for variables defined at runtime, but I'm not a fan of defining configurations at runtime. I prefer configs being well known variables - akin to constant declarations. Using the config package as a poor man's DB takes node-config into a different direction.

Do you have a good example of a configuration where the config variable name isn't known until runtime?

Collaborator

markstos commented Feb 26, 2014

I also prefer and use configs like "constant declarations". Mutable
globally accessible variables have been a persistent source of bugs
for programmers and are best avoided or disallowed.

On 2/25/14, Loren West notifications@github.com wrote:

Good point about using $ to prevent config/method clashes. Others have
requested adding monitoring for variables defined at runtime, but I'm not a
fan of defining configurations at runtime. I prefer configs being well
known variables - akin to constant declarations. Using the config package
as a poor man's DB takes node-config into a different direction.

Do you have a good example of a configuration where the config variable name
isn't known until runtime?


Reply to this email directly or view it on GitHub:
#109 (comment)

Mark Stosberg
Senior Systems Engineer
RideAmigos
Owner

lorenwest commented Apr 4, 2014

Adding #114 to the list of 0.5 features.

Collaborator

markstos commented Apr 16, 2014

Loren and others:

Here's a couple other feature requests for increased safety. Neither
would be backwards compatible, but thankfully npm's use semantic version
make backwards compatible reason to do, since people aren't
automatically upgraded to new major versions.

  1. When no config filess are found, I would prefer than an exception be
    thrown, rather than return an empty config file. The empty-object
    behavior can cause the problem not to be noticed till later, when
    the consequences are worse. It can lead to an "Garbage-In, Garbage
    Out" state where you get a confusing error about something being
    undefined further on, instead of an explicit error that "no config
    files were found".

    If you really want an empty config, it doesn't seem too much to ask
    to have people create an empty JSON file that contains just "{}".

    I ran into this as a first impression when I created files in the
    top level directory instead of "./config", and was surprised when
    an empty config object was returned.

  2. Likewise, currently a typo in the name of a config key can cause the
    same kind of GIGO bugs as above, since the typo will silently turn
    into a undefined value, which will get passed along, and may
    silently disable a feature somewhere:

    new ImportantObject({
    importantOption : configs.typoedkey,
    });

    I would prefer that the config system require that a key /exist/ in
    the config file in the common usage, and an exception be thrown if
    the requested key does not exist.

    new ImportantObject({
    // BOOM! Immediate, useful feedback
    importantOption : configs.config('typoedkey'),
    });

    Since this system loads and merges multiple config files, it should
    not be a great imposition to make sure that a key is at least present
    in the main 'default' config file.

    Less commonly, in a system with plugins, you may wish to check to see
    if a key exists, without having an exception thrown. A separate method
    could be used for that:

    if (config.has('optionalKey')) {
    // Do something with config.config('optionalKey').
    }

For years, I used a config system that silently let typo'ed keys slip
by. Then, I updated the system to throw exceptions on typo'ed keys. It
was immediately helpful and caught issues before they got further.
(If I recall, the change may have caught some config lookups that were
never working right, and no one had noticed the GIGO problem).

The benefit is much like adding "use strict" to your code.

What do you think, Loren? Do these features seem like a good fit for
node-config?

These would be my personal preferred behaviors, but along with
immutability, they could also form a "strict mode" for node-config.

Mark

Owner

lorenwest commented Apr 16, 2014

Hi Mark,

Both of these suggestions are great, and I'll make sure to do them in the 0.5 release. Item #2 would introduce a minor backward incompatibility because any method I add is in the same namespace as configs, so a conflict could occur. I'll make sure to pick a verb (who names their configs with verbs?) to lessen the likelihood of a conflict.

Thank you,
-Loren

Collaborator

markstos commented Apr 16, 2014

Great.

If you end up making a breaking change, the convention is to bump the major version (so, 1.x.x): http://semver-ftw.org/ 'npm' should respect that and not automatically upgrade people to it.

Owner

lorenwest commented Apr 16, 2014

You're right. As a reformed semverist, the new version will be 1.0.0 vs. 0.5.0.

Node.js versioning is not a great example for us module authors, although within the 0.x.x range the semver spec does offer some leeway.

Throwing exceptions is not that standard in nodejs. The feature may be useful in some cases, but I think it should probably not be applied by default.

For the absent config file (proposed change 1.) it does make sense I think. But for the (proposed change 2.) non existing keys 'config' should behave like a plain object: return undefined.

It's usual to pass sub-objects of 'config' to a function, and the function has no idea if it's a plain object or a 'config' object: it just uses it as a plain object, and that's a great feature. We would loose this with the proposed change 2.

But there should be a way to enable it when requiring 'config'.

This leads to a related point: require('config') directly returns a config object with user values as children, so we have to pollute user namespace with some 'config' functions, and we just discussed to add even more ones.
It would be much cleaner to isolate 'config' methods from user values, something like that maybe:

// basic api: return the global object
var config = require('config').config;

// advanced api: configure the object
var config = require('config').getConfig({immutable: true, throw_exceptions: true});

// or a common api, that would slightly complexify the basic case, but we also can improve from the two above:
// basic
var config = require('config')();
// advanced
var config = require('config')({immutable: true});

I'm not familiar with all the 'config' methods added to the user values, so I don't know if they can all fit on this model. If not I hope we can still find a clean alternative.

Collaborator

markstos commented Apr 17, 2014

Throwing exceptions is useful when a programmer has made a mistake, which would be the case for a typo'ed config.

In terms of the API, I would lean towards two simpler APIs rather than one hybrid/complex one.

If the safety/strictness features don't become the defaults, I would be inclined to wrap what's called the Advanced API here like this:

var config = require('config-strict');

(Which would do very little, but turn on the strictness features as above, with a more convenient, memorable syntax).

Owner

lorenwest commented May 19, 2014

Would anyone care to discuss the removal of runtime configuration changes for the upcoming release?

I am considering removing the runtime.json functionality as it comes with more cost than value, and I expect it won't be used much if we move towards immutable by default.

The runtime.json feature is only useful as a poor db-backed persistent mutable configuration.

I don't know if it's currently multi-process safe, or multi-machine safe (with something like NFS).

If we want a persistent mutable configuration, we need at least a db-backed version, and optionally a file-backed one, for simple setups. The file-backed version only is treacherous: it lets think that node-config can handle such usage, but in fact it only works for simple deployment cases.

Personal experience: in one project we needed a multi-machine safe persistent non-mutable configuration, and we currently use mongodb for that: at node startup we merge node-config into the db for new keys from the config file, then patch the config object with the db values (the file is for bootstrap; the db is the reference). The config is mutable, but we require a node process restart to take the modifications into account (we just write on db, but don't patch the config object, nor watch for db updates). I's not the best thing to do, but it was enough and easy.

Owner

lorenwest commented May 20, 2014

Node-config has been around since node v0.4, and with so many professional persistence mechanisms available, I feel it's no longer the job of node-config to manage persistence of configuration mutations.

Many people would argue against configuration mutation in the first place, and aside from one-time dynamic configuration like @triccardi-systran mentions, I'm not a fan of configuration mutations either.

So starting with node-config v1.0.0, if persistence of configuration mutation is desired, it will be up to the app developer to coordinate that persistence using their trusted mechanism. Turning on/off mutations will be supported, so the app bootstrap can turn on mutations, merge DB bound configurations into the config object, then turn mutations off.

Please speak up if you have strong feelings for or against this direction.

I'm for this solution.

Collaborator

markstos commented May 20, 2014

Sounds fine with me.

On 5/20/14, Thomas Riccardi notifications@github.com wrote:

I'm for this solution.


Reply to this email directly or view it on GitHub:
#109 (comment)

Mark Stosberg
Senior Systems Engineer
RideAmigos
Owner

lorenwest commented Jun 4, 2014

Thoughts on a few 1.0 items:

Immutable only

Configuration parameters cannot be changed once loaded, and is a core feature of node-config 1.0 and beyond. For backward compatibility, if runtime.json exists it will be loaded, but it will not be written by or monitored by node-config.

Configuration objects may have new properties added to them (from a DB backend for example), but will not allow changes to existing properties. This extends recursively to all sub-configuration objects.

Exceptions on load

Decided to output a console.error vs. throwing an exception if no configurations are found when loading node-config. This allows npm modules (using node-config) to work well with apps that use node-config as well as apps that don't use node-config for configuration.

config.get(parameterName)

This may be used instead of accessing the property directly as a way to enforce name checking. It will throw an exception if the specified parameter doesn't exist. Namespace collision is traded for simplicity with the assumption that get is a terrible name for a configuration parameter, and isn't likely to clash.

No set method since configurations are immutable. Also, no default value passed as a parameter, because default values should be contained in default.json vs. spread around in code.

get is sub-object aware, and attached to all sub-objects so you can do this:

var config = require('config');
db.connect(config.get('dbConfig.host'), config.get('dbConfig.port'));

or

var dbConfig = require('config').dbConfig;
db.connect(dbConfig.get('host'), dbConfig.get('port'));

config.has(parameterName)

This can be used to see if a configuration property exists without throwing an exception.

DB Backend

Not adding DB awareness to node-config.

Collaborator

markstos commented Jun 4, 2014

Sounds excellent to me.

Thank you.

On 6/3/14, Loren West notifications@github.com wrote:

Thoughts on a few 1.0 items:

Immutable only

Configuration parameters cannot be changed once loaded, and is a core
feature of node-config 1.0 and beyond. For backward compatibility, if
runtime.json exists it will be loaded, but it will not be written by or
monitored by node-config.

Configuration objects may have new properties added to them (from a DB
backend for example), but will not allow changes to existing properties.
This extends recursively to all sub-configuration objects.

Exceptions on load

Decided to output a console.error vs. throwing an exception if no
configurations are found when loading node-config. This allows npm modules
(using node-config) to work well with apps that use node-config as well as
apps that don't use node-config for configuration.

config.get(parameterName)

This may be used instead of accessing the property directly as a way to
enforce name checking. It will throw an exception if the specified
parameter doesn't exist. Namespace collision is traded for simplicity with
the assumption that get is a terrible name for a configuration
parameter, and isn't likely to clash.

No set method since configurations are immutable. Also, no default
value passed as a parameter, because default values should be contained in
default.json vs. spread around in code.

get is sub-object aware, and attached to all sub-objects so you can do
this:

var config = require('config');
db.connect(config.get('dbConfig.host'), config.get('dbConfig.port'));

or

var dbConfig = require('config').dbConfig;
db.connect(dbConfig.get('host'), dbConfig.get('port'));

config.has(parameterName)

This can be used to see if a configuration property exists without throwing
an exception.

DB Backend

Not adding DB awareness to node-config.


Reply to this email directly or view it on GitHub:
#109 (comment)

Mark Stosberg
Senior Systems Engineer
RideAmigos

Configuration objects may have new properties added to them (from a DB backend for example), but will not allow changes to existing properties. This extends recursively to all sub-configuration objects.

This is not enough for my use case: I need to be able to override existing properties when loading the data from the DB; albeit only on startup.

You previously proposed Turning on/off mutations will be supported, but we could instead work with a special function like merge, to merge a db tree into the config tree. This seems cleaner: no need to toggle a global state on config, and still avoid accidental modifications of the tree: you have to explicitly call merge.
We use this http://lodash.com/docs#merge to exactly do that today.

Collaborator

markstos commented Jun 4, 2014

Thomas,

Could you do your DB-merge first with lodash merge, and then pass the
result to node-config?

I don't think this case will come up often, and adding a line or two
of code to deal with a less common case seems like a decent trade-off
vs having extra extra code/docs/test in node-config to support it.

On 6/4/14, Thomas Riccardi notifications@github.com wrote:

Configuration objects may have new properties added to them (from a DB
backend for example), but will not allow changes to existing properties.
This extends recursively to all sub-configuration objects.

This is not enough for my use case: I need to be able to override existing
properties when loading the data from the DB; albeit only on startup.

You previously proposed Turning on/off mutations will be supported, but we
could instead work with a special function like merge, to merge a db tree
into the config tree. This seems cleaner: no need to toggle a global state
on config, and still avoid accidental modifications of the tree: you have to
explicitly call merge.
We use this http://lodash.com/docs#merge to exactly do that today.


Reply to this email directly or view it on GitHub:
#109 (comment)

Mark Stosberg
Senior Systems Engineer
RideAmigos

@markstos sure, but how do you do the "pass the result to node-config" part? Either when constructing the Config object, but currently there is no pre-configuration done: requiring 'config' directly gives you the object; or after the config object is constructed, that's what I proposed with merge, but it could indeed be a set, and the merge is done outside of node-config.

Collaborator

markstos commented Jun 4, 2014

Ah, good point. It does appear that if a merge is going to be
supported, it needs some kind of explict support. Something like:

// Normal case, note the updated require signature
var CONFIG = require('config')();

// Merge case
var CONFIG = require('config')({ merge : otherObject });

I'm not certain I like that design, and I'm not sure if merge is
necessary, but that's what came to mind.

I guess there is another existing workaround: Look up the config from
the database and write it to one the miles that node-config would
merge, before node-config is loaded.

Wait, since "JavaScript files" are one of the formats supported, and
it appears the JavaScript is actually executed. Would it be a
clean-enough solution to have of the ".js" files you load dynamically
look up the database configuration and return it?

I'm just brainstorming here, but I think the last option threw out is
most appealing to me for handling this case.

On 6/4/14, Thomas Riccardi notifications@github.com wrote:

@markstos sure, but how do you do the "pass the result to node-config" part?
Either when constructing the Config object, but currently there is no
pre-configuration done: requiring 'config' directly gives you the object; or
after the config object is constructed, that's what I proposed with merge,
but it could indeed be a set, and the merge is done outside of
node-config.


Reply to this email directly or view it on GitHub:
#109 (comment)

Mark Stosberg
Senior Systems Engineer
RideAmigos
Owner

lorenwest commented Jun 4, 2014

The merge() functionality would be nice, however the mechanism used for making attributes immutable cannot be un-done, and alternate implementations for immutability are complex.

Fortunately there are ways to meet your use case. You could write the DB values to one of the local.json files, which aren't designed to be a part of your VCS. Or, before calling var config = require('config'); you could stringify the DB configurations into process.env.NODE_CONFIG as if they came in from the environment.

And depending on your needs, you may be able to separate DB configurations from FS configurations, and simply add them onto the config object without clashing. If you add new attributes and want to make them immutable, call config.makeImmutable(config) after you've added to the config object.

The process.env.NODE_CONFIG method is acceptable, even if it's less than ideal. (I just tested with a large string in process.env.NODE_CONFIG, and if it's set from the process there seems to be no limit on the size, so no issue there).

The only other viable solution would be to have parameters on Config constructor, but as explained earlier, it's alas probably not a good idea.

Owner

lorenwest commented Jun 4, 2014

Changing require('config') to be a constructor function vs. the config object would make it a more flexible interface, but it would break all current usage of node-config.

It's one thing to remove functionality that shouldn't have been there in the first place (mutating configs), and a completely different thing to force a code change in order to use v1.0.

The v1.0 compatibility message is If you're not changing your configurations at runtime, the 1.0 upgrade should be painless.

tomyam1 commented Jun 11, 2014

Just found this module and totally love it.
My small request is a getter for CONFIG_DIR.
In case no configuration files where loaded, I want to show a message like "No conf. files where loaded. Looked in directory #{CONFIG_DIR}"

Owner

lorenwest commented Jun 11, 2014

Hi @tepez,

The finishing touches are being put onto the v1.0 release, and it should have what you're looking for. First, if there are no configurations found, it outputs a warning to stdout.

In addition, it exposes a .get function on the config objects so if you do config.get('Customer.dbName') and that configuration doesn't exist (due to mis-spelling or a bad CONFIG_DIR), an exception will be thrown as a fail-fast mechanism.

If you'd like the empty config logic early, you can put this in your application now:

    var config = require('config');
    if (Object.keys(config).length === 0) {
      console.error('No configurations found');
    }

Hope that helps,
-Loren

tomyam1 commented Jun 11, 2014

Thanks @lorenwest .
I'm already checking for empty config by config.getConfigSources().length === 0.
I wanted the getter for CONFIG_DIR so I can create a message on my own, if I want to.
Running without configurations is for some applcations so I think the emptiness check should be done by the users of node-config and not by node-config itself.

Another feature I'd love to see in future releases is reloading configurations. I need this for unit testing so I can a clean configuration before each test. Clearing the require cache (issue 34) seems a bit hacky.

Owner

lorenwest commented Jun 11, 2014

Good call on exposing CONFIG_DIR - I'll add a method for that before releasing.

The length check doesn't fail the config but only outputs a warning to stderr, and I believe it's a healthy warning for any app that chooses to use node-config but has no configurations. Users of node-config aren't generally thinking about that issue until it's too late.

As for reloading configurations, I've considered adding support and don't believe it's something that should be supported directly by node-config. The reason is the way people use node-config is by putting var config = require('config'); at the top of their .js files. Once they've got a reference to this object, you cannot reach in and change that reference.

As of v1.0, all values within config objects are immutable - for the same reason. Once someone has a reference to a value, changing that value in the object doesn't change the module referencing the old value. Supporting the ability to change values was a bad decision in earlier versions of node-config, because you end up with partial configuration changes throughout the app.

If you're sure of what you're doing, the hacky mechanism in issue 34 will work.

Owner

lorenwest commented Jul 23, 2014

Version 1.0.0 has gone through a soaking period and has been released to NPM, resolving this issue. Thank you for all of your input.

@lorenwest lorenwest closed this Jul 23, 2014

FWIW because of the immutability imposed in 1.0.0 we replaced node-config by nconf.

Owner

lorenwest commented Sep 3, 2014

Thank you for your input @triccardi-systran. Immutability is new in 1.0 and while the stability it offers is important, merging additional configs is also important. There is a discussion on thread #126, where we're considering allowing configuration changes before the first .get() is issued. That allows bootstrap sections of code to merge in whatever they like, while maintaining the philosophy that a config won't change once accessed. Would that have solved your use case?

@lorenwest: it would probably have solved our use-case indeed.
But I won't discuss this further as we don't use node-config anymore, and nconf is sufficient for our usage so we don't plan on switching back.

nconf also has the advantage over node-config of natively having customizable backend storages, so our db backend use-case is standard and doesn't require a lot of duplicated work.

Owner

lorenwest commented Sep 3, 2014

@triccardi-systran - Appreciate the input!

Collaborator

markstos commented Sep 4, 2014

I think allowing changes before first .get() adds some useful flexibility,
while still preserving the bulk of the safety of immutability.

A use case: For testing sometimes I do want a test suite to be able to
toggle a config value, or just make sure an explicitly set a certain way.
For example, set the "ENABLE_HTML_ESCAPING" flag to test an HTML escaping
routine.

It would be nice to have a way to do handle that edge case, but still
generally be able to expect immutability.

On Wed, Sep 3, 2014 at 1:11 PM, Thomas Riccardi notifications@github.com
wrote:

@lorenwest https://github.com/lorenwest: it would probably have solved
our use-case indeed.
But I won't discuss this further as we don't use node-config anymore, and
nconf is sufficient for our usage so we don't plan on switching back.

nconf also has the advantage over node-config of natively having
customizable backend storages, so our db backend use-case is standard and
doesn't require a lot of duplicated work.

Reply to this email directly or view it on GitHub
#109 (comment)
.

Mark Stosberg
Senior Systems Engineer
RideAmigos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment