Thank you for this important addition, and the thoroughness of this PR.

I won't have a chance to read through the code, but I agree with the thoughts and concerns addressed.

@markstos will probably give the final thumbs up, but it's a 👍🏽 from my perspective.

Thanks for the input @lorenwest ! I want to give this a second read-through before merging.

Hi @markstos, I hope all is well. Do let me know if there's anything that I can do to help you on this. Whether it's a session to walk through what I've put together or to help with the updated Wiki. Is it as simple as submitting a change here and then you are pinged to accept the change?

setModuleDefaults will now throw an error if it's called more than once for the same moduleName.

To an extent I'm looking at the same issue now and I'm not sure what the right answer is. It is certainly possible to end up with multiple copies of the same module in your project. And in general node-config has a last writer wins strategy, so having setModuleDefaults fail on the second call seems to be at odds with that pattern.

Meanwhile, you probably have 2 copies of a module because they're different major/minor version numbers, and one of the reasons you might increment these numbers is due to a change in the configuration.
Any module that reads back its own configuration would be in trouble in this case.

Unfortunately I don't have a better suggestion at this time.

Very good. Let's explode this issue and see if I can make sense of it.

For example:

application
├── config
│   └── default.json
└── node_modules
    ├── config
    ├── lib1 (1.1)
    │   └── config
    │       └── default.json
    └── lib2
        └── node_modules
            └── lib1 (1.0)
                └── config
                    └── default.json

Let's say we have lib1 which is evolving and has configuration that's evolving too. We'll naturally get applications and other libraries which will reference different versions of them over time. In the scenario that I've drawn above, application will have config files that's catering to lib1@1.1, but it has a dependency lib2 which is pulling in lib1@1.0.

There are two clear problems:

1. If we do throw an error for subsequent calls to setModuleDefaults for the same module, then there's no way to cater to this natural evolution of lib1. Which I do agree is a bit of a failure.
2. application's config/default.yaml will override configs for both lib1@1.0 and lib1@1.1. How can it target which versioned config to override... or should it be able to? I don't see an existing solution for how to do that today. It feels related to Find a solution to the multiple-instance problem for submodule defaults #226.

I have an idea that may help both. Let me know what parts of it make sense and what doesn't.

1. I update this PR so that setModuleDefaults/getModuleConfig will still honor the last writer wins strategy for the signature of:

   setModuleDefaults(moduleName: string, defaultProperties: object): object
   getModuleConfig(moduleName: string, overrideProperties? object): Config

   This way you can at the very least allow for a module to evolve and version itself and at the worst, it will require the module developer to artfully craft their configurations to always be future compatible. Then we still have parity with the current implementation and haven't broken backwards compatibility.

2. Attempt to find a way to namespace registered module defaults by version and simultaneously find a configuration syntax that allows you to target historic versions of the module. Please beat this up. If it doesn't make sense yet, I'm hoping that it has legs and can evolve into a solution.

   /**
    * If version is provided then the Config will store a separate default for each version specified.
    * This will allow module defaults to be set for each version registered by the module owner 
    * (I would only recommend using major.minor as version assuming that the config doesn't 
    * change for bugfixes and to reduce the number of fragmented configurations)
    */
   setModuleDefaults(moduleName: string, defaultProperties: object, version? string): object
   
   /**
    * This would only be used by the submodule that registered it's defaults and will safely always 
    * return the configuration that it registered overlayed by the `application` configs
    */
   getModuleConfig(moduleName: string, overrideProperties? object, version? string): Config

   In an attempt at how to update the config syntax to target versions, my brain starts with something like this:
   application/config/default.json

   {
     "lib1": {
       "theBestFeature": true,
       "logLevel": "info"
     },
     "lib1:1.0": {
       "anOldFeature": false
     }
   }

   All parser types will essentially convert to json so they can all represent this format in some fashion. the getModuleConfig can take the default submodule config and then overlay the versioned config on top. So if lib1@1.0 and lib1@1.1 both used the getModuleConfig with version specified they would both respectfully get these configuration overrides set from application:
   lib1@1.0

   {
     "theBestFeature": true,
     "anOldFeature": false,
     "logLevel": "info"
   }
   

   lib1@1.1

   {
     "theBestFeature": true,
     "logLevel": "info"
   }

   If we wanted to, we could also throw warnings if config specifies a version that no longer exists so the application has a chance to clean them up. That may be overkill, but could be done and provided a env var to suppress the warning if needed. The more I think through this, it's likely better to continue on Find a solution to the multiple-instance problem for submodule defaults #226 and not be solved in this ticket.

I know that was a lot... let me know if any of that makes sense.

Is this something that's still getting looked at for consideration?

I think that breakdown is interesting but in the end all of this is complicated by node-config being a singleton in a world that knows why singletons are a bad idea.

The “right” solution here is a 4.0 version that supports a constructor call (I have code somewhere that sniffs whether new is being called, I can find it again if anyone wants to know the trick), and then things like behavior by caller can be implemented as arguments. Because at the end of the day it’s not getModuleConfig that needs to behave differently, it’s get(), and adding more arguments for each caller is a race you cannot win.

if you accept that config is not a collaboration between peers creating a “stone soup” of configuration open to anyone to read, and instead is a collaboration between a module and its client, then the config is ten different answers to ten different modules, rather than all things to all modules. You can sort that relationship out at constructor time, rather than at call time or via action at a distance as is currently the case.

In my code I try to work around this lack by scanning node_modules for config directories and loading module defaults based off of finding the dirname for each module and calling setModuleDefaults, with data loaded by a second instance of node-config with an empty NODE_CONFIG_DIR. it’s ugly but not compared to our original attempt at this which was just madness. What I don’t have is a solution to the problem we are discussing here, of multiple versions of the same library wanting differing config. Of libraries wanting their own definition of “the truth” rather than a single global one. But the hierarchical node_modules directory is itself not a global truth model. Each module is free to load a different copy of a dependency. The tools we use should work the exact same way.

I don’t have an implementation that answers this thread, just a global namespace. But the only time I really use all of this config at the top level is for debugging code and tests, and if node-config sorts this problem set out, I would happily move that scanning code into my validation and debugging code.

I suspect the right solution then is to modify first reader wins such that module defaults (configSources?) are part of an instance of node-config, but NODE_CONFIG_DIR gets loaded once, on the first require call. Those two data streams get merged in the constructor and marked as read only per instance, instead of globally.