setModuleDefaults behaves differently from loadFileConfigs() when the defaults collide with config utility functions

[jdmarshall]

I'm submitting a ...

• [ x ] bug report

What is the current behavior?

If your default.js file contains a key that collides with node-config's extendDeep() helpers, such as 'get', then config figures it out and leaves the value. However when you pass in values via setModuleDefaults, they don't get copied because the function already exists.

What is the expected behavior?

setModuleDefaults() and extendDeep() should have the same semantics, so that loadFileConfigs behaves the same as setting defaults.

Please tell us about your environment:

• node-config version: 3.3.7
• node-version: 14.20.0

Other information

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

[jdmarshall]

This seems to be caused by util.makeHidden(). You can't assign to an object once you've defined a property by the same name.

[lorenwest]

Are you suggesting we add support for Reserved Keywords by allowing get to be a valid config key?

[markstos]

setModuleDefaults() and extendDeep() should have the same semantics, so that loadFileConfigs behaves the same as setting defaults.

I disagree. node-config is intentional about making the module immutable, and it's not immutable if we open it back up for changes after it's been made immutable.

However, I'm leaving this open because the documentation could be clearer on this behavior difference.

[jdmarshall]

@markstos You've glossed over the real issue and picked on the commentary.

If your default.js file contains a key that collides with node-config's extendDeep() helpers, such as 'get', then config figures it out and leaves the value. However when you pass in values via setModuleDefaults, they don't get copied because the function already exists.

That's a bug, and has nothing at all to do with pinning.

[lorenwest]

@jdmarshall - is this a problem beyond the 3 Reserved Keywords of get, has, and util, or is it only a problem with these reserved words?

While it would be better to have zero reserved keywords, it seems like a pretty major undertaking to remove them.

So, yea, if you try to use one of the reserved keywords, undefined results may occur. I don't think we've thought of all the problems related to trying to use these keywords, or workarounds.

[lorenwest]

If the underlying issue is the inconsistency in the way it fails if you try to use one of the 3 reserved keywords, I'd be happy to have a conversation with someone willing to make this failure more consistent, and submitting a PR.

[jdmarshall]

I took a stab at solving this a couple weeks ago, but I couldn't get a simple repro case to behave consistently. I thought perhaps that meant the last couple of PRs changed some data flow that I didn't quite understand, but it turns out the problem does still occur within my real code.

Interestingly, I have two spots in the config where this should fail, but one does not. I'm trying to nail down what the difference is, but it seems to have something to do with whether the reserved words have siblings or not.

[jdmarshall]

Got it. PR filed. The crux is the lack of configurable=true in defineProperty() calls. These will get mopped up by makeImmutable() at the end of the process.