Reentrancy bugs

[jdmarshall]

Multiple or nested calls to loadFileConfigs can result in file-scoped variables in lib/config.js being overwritten before being used.

CONFIG_DIR is especially problematic, but I also fixed runtime.json support because it represents the last use of CONFIG_DIR, after parseFiles() has been called.

[markstos]

I'm not sure how much this comes up, but the fix looks reasonable and test coverage confirms both the problem and the fix, so I'm merging.

QA Log

• Reviewed changes
• Ran only the new test coverage without the changes and confirmed that a re-entrant test failed.

[markstos]

I'll merge once the conflicts with master are resolved.

[jdmarshall]

Someone got excited about config as code on a project I work on.

setModuleDefaults(loadFileConfigs(...))

might be a wiser choice in some spots but that is not without a different but potentially equal set of problems.

• sequential concerns, because lib/config.js now refers to a mix of state from the NODE_CONFIG_DIR and configDir
• canonicalization I can't see the entire config state for my app until every single module has been require()d.
• Orphaned defaults when there are two copies of node-config in node_modules
• can you even call setModuleDefaults() after the first get()? If not then every module has to be loaded before any of them make a single decision based on config

Some of these are less correctness concerns and more ergonomics (how do I explain it to people so they make the right inferences when they have a problem or a feature?) but the alternative ends up with a lot of copies of node-config being requireUncached() due to variable clobbering. This doesn't scale well either and so I'm finally trying to do something about it (hence the PRs).

As I said in my last commit, it would be lovely if there was a lib/utils.js that was entirely pure functions. Those make testing a cakewalk. I took a stab at this a couple years ago when I first encountered this set of problems, and I hit snags. Some of the intervening PRs have reduced that surface area, and both of my PRs so far chip away at that a little more.

As it stands, Config and loadFileConfigs are at cross purposes to each other. One expects to be called exactly once, the other expects to be called N times, and they share scope.

[markstos]

@jdmarshall The node-config project is ripe for a major overhaul. It could use a TypeScript rewrite, integrated linting, replacing vows another test framework, and fixing code architectural problems.

@iMoses started tackling several problems. You might be interested in particular with an unmerged PR that deal with util updates: #571 There were good ideas there, but it was a large PR at over 1,000 lines of change and the conversation about how to best move forward with it eventually broke down.

Ultimately there will likely need to be some breaking changes to apply the best fixes for some things.

I'm not the author of node-config myself, but a long-time co-maintainer. Along with the original author, I don't have much time to devote to this either. The project needs more maintenance help, but suffers from being popular project so there's a steady stream of issues and PRs to react to.