loadFileConfigs incorrectly adds to getConfigSources

[exogen]

I'm submitting a ...

• bug report

What is the current behavior?

The documentation for getConfigSources says:

The config.util.getConfigSources(); method can be used to see all sources contributing to the ultimate configuration, and the order in which they were are applied.

However, this isn't accurate if you call loadFileConfigs:

const otherConfig = config.util.loadFileConfigs('./other-config-dir');

config.util.getConfigSources().forEach(source => console.log(` - ${source.name}`));

Prints:

 - [...]/demo-app/config/default.js
 - [...]/demo-app/config/development.js
 - [...]/demo-app/other-config-dir/default.js

This is inaccurate, as otherConfig hasn't been merged at all with config yet, so it is not a source for what is contained in config. It hasn't affected config in any way, only otherConfig.

If you then use this value to set defaults via setModuleDefaults, it's still inaccurate, printing:

 - Module Defaults
 - [...]/demo-app/config/default.js
 - [...]/demo-app/config/development.js
 - [...]/demo-app/other-config-dir/default.js

Now that file is technically a source, but it's in the wrong spot – its contents are actually part of Module Defaults (coming first), the opposite of what is implied by the docs (they are not the final override in the cascade).

What is the expected behavior?

Configs loaded with loadFileConfigs should not appear in getConfigSources.

Potentially, the methods that one can use to merge config data (e.g. extendDeep, setModuleDefaults) could take a special argument or field (using Symbol?) for the developer to specify the source the config data came from, in which case they could safely appear there in the correct order no matter how the developer merges them in.

Please tell us about your environment:

• node-config version: 3.0.0
• node-version: 10.13.0

Other information

(e.g. are you using an environment besides Node.js?)

[NguyenMatthieu]

Hi.

I've just found this issue while investigating a memory leak in one of the applications in my work. We use the loadFileConfigs utility to load configurations from a different folder than "config" but using the same precedence and loading patterns that the node-config package uses (because we consider it a good, well accepted standard for configuration overrides).
(Basically we use it to handle two sets of configurations, client and server side).

Calling the loadFileConfigs repeatedly adds to the configSources potentially very large objects, and because the Config object is a singleton, the configSources never get cleaned up, or garbage collected, and in turn, the memory can grow out of proportion.

Since loadFileConfig does not add to the global config object, shouldn't it also not add to the global configSources?

Thanks,

[lorenwest]

Hi @NguyenMatthieu, you bring up a good point. My guess is most people don't use those config sources.

It wouldn't bother me if a parameter were added to loadFileConfigs that would allow it to do its job without adding to the configSources object.

Leaving open for someone (maybe you @NguyenMatthieu ?) to add as a feature along with a test. I'd be happy to review and merge.

[NguyenMatthieu]

HI @lorenwest and thanks for the quick reply.
I'd be happy to work on a PR (so long as my company gives me the green light). With regards to the solution, I am thinking how to maintain backwards compatibility.
It seems to me that the default behaviour of loadFileConfigs and parseFile should actually to not add to the configSources, UNLESS used within the constructor, since only the constructor adds to the global 'config' object. Similarly, setModuleDefaults also adds to the global config object, and accordingly adds to the configSources legimitately, but in a special entry anyways.

Would you be ok with making the loadFileConfigs and parseFile functions accept either a "string" (legacy behaviour) or an "options" object:
{
configDir (optional),
addToConfigSources (optional)
}
(fullFileName instead of configDir for parseFile)

We would only push to configSources when receiving an options object that would have the addToConfigSources flag explicitly set to true, and the constructor would just be updated to do loadFileConfigs({addToConfigSources: true}).

The default behaviour for people using "loadFileConfigs" would change to "not adding to configSources" by default, and that should address the actual original issue here, without breaking the original functionality (unless I missed something).

wdyt?

[lorenwest]

We can't change behavior for people already using the loadFileConfigs directly. For better or worse, they may be using it for the side effect of understanding the accumulated configSources.

That's why I suggested an additional parameter for the new behavior. As much thought as you're putting into this issue now, in 3 or 5 years you will appreciate that somebody doesn't change it in a way that made you re-think this in as much detail as you are now :)

[NguyenMatthieu]

@lorenwest hmmm, ok. Any preference for the new parameter name? skipConfigSources, doNotTrack, etc...

Also, what about my proposal to use an "options" object holding that parameter as well as the original input? (still preserving support for the original string parameter)?

I know it's a matter of preferences and coding styles entirely, and I'll follow what you and other maintainers suggest as an approach ;)

[lorenwest]

As for the parameter name, skipConfigSources is good. I also wouldn't mind the new behavior being an options object because it maintains compatibility with the non-object parameter. That seems like a healthy change.

[NguyenMatthieu]

Hi @lorenwest. Sorry it took a while to get approval on my side to work and submit the PR. I've sent it. Let me know if there's anything you need.
I'm also thinking that if accepted, the documentation on the config utilities should be updated (and maybe also a new version published). Should I prepare a PR on the wiki as well?

[lorenwest]

Thanks for the contribution. Published in 3.3.4

[NguyenMatthieu]

@lorenwest Happy to Help ;)