-
Notifications
You must be signed in to change notification settings - Fork 488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved submodule support #670
base: master
Are you sure you want to change the base?
Improved submodule support #670
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to solve some some long-standing and difficult problems. I like that it's a backwards compatible solution.
I like to get @lorenwest take on it, who is temporarily less available for personal reasons. I appreciate your patience as we get you more feedback on this.
Thanks!
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. |
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. 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:
Let's say we have There are two clear problems:
I have an idea that may help both. Let me know what parts of it make sense and what doesn't.
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. |
Goals
config.get
was called) and the applications configuration became immutable (closes [feature] Improved submodule support #572 - Late Loading)Implications (Backwards Compatibility)
setModuleDefaults
will now throw an error if it's called more than once for the samemoduleName
.getModuleConfig(moduleName: string, options: object | undefined)
method was added to return a newConfig
instance which merges the provided options into the module's registered default configs and returns a new Config instance. This method will throw an error if thesetModuleDefaults
was never called for the given module.Solved Issues
I started this branch to solve the Late Loading portion of #572, but am of the opinion that all acceptance criteria of #572 are met and the ticket can be closed. I saw an opportunity to improve module configuration support which closes #226 as well. Let me know if you'd rather me split this into two PRs.
Issue #572, Late Loading
I found that the setModuleDefaults method would merge the module defaults into the root config object. This is obviously a problem when my module is dynamically loaded after
config.get
was called and the root config object was immutable and wouldn't accept my module defaults.moduleConfigs
object to store module default configs. The module defaults will get any existing overrides in the global config merged in. Once set on the globalmoduleConfigs
object, only that modules entry will be made immutable immediately.config.get
method was updated to look in themoduleConfigs
object for keys if they come back undefined on the global config object.Issue #226, Allow multiple instances of modules
Please raise issue with any misunderstanding that I have about the current problems regarding multiple instances of modules. I read through issues #226 and I believe that I've found that adding one new method
getModuleConfig
provides a clean way to create new Config instances which can share the lifecycle of the module instances.Issue #572, Submodule config as default
With the
loadFileConfigs
method, It appears that this is already supported. As a user of this library, I've never found it difficult to simply add the following lines to my submodule's entry point:This is hardly a burden and the APIs clearly supports this already.
Issue #572, Dual purpose
Again, if developers write their modules to load configs from the expected file locations, I don't see why this isn't already supported. If the ask is to allow modules to access config keys without specifying the root module name, then the approach I provided in the examples may help. Again, I don't see how this isn't already supported.
Side effects
Accessing module default configs
Since the default module settings exist separate from the global config object, then you can access the root module configs from any Config instance. I see this as a unexpected benefit. This means that the
getModuleConfig
instances created will allow you to access the module's default configurations.Examples
Module returns one class
Here's an example module that would provide this behavior. I added something similar to the unit tests in this PR.
This allows the application to then use the module multiple times. Each instance of the TestModule class being able to have unique configuration and override as needed. The trick simply being that each instance would need to manage the retrieved configuration from
getModuleConfig
.Module returns multiple classes with nested config scopes per class