Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors config into a class-based, per-instance Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant ModuleIIFE as "Config Module IIFE"
participant Env as "Env"
participant Load as "Load"
participant Config as "Config Instance"
App->>ModuleIIFE: require/import config
ModuleIIFE->>Env: build Env from environment
ModuleIIFE->>Load: Load.fromEnvironment(env, options)
Load->>Load: initialize sources & config
ModuleIIFE->>Config: new Config(load)
Config->>Config: store load (LOAD_SYMBOL), attach utils, run strictness checks
ModuleIIFE-->>App: export Config instance
App->>Config: get(path)
Config->>Config: resolve path, enforce immutability, return value
Config-->>App: value
App->>Config: withModuleDefaults(name, defaults)
Config->>Load: Load.clone()
Load->>Load: produce independent Load copy
Config->>Config: new Config(clonedLoad) and apply defaults
Config-->>App: new Config instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
74b25b7 to
4056cb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/config.js (1)
12-14: Make mutability tracking instance-scoped to avoid cross-instance leakage.
checkMutabilityis module-scoped, so the firstget()on any Config instance disables immutability for all others (including those created bywithModuleDefaults). That can leave configs mutable unexpectedly. Move the flag to per-instance state.🔧 Suggested fix (instance-scoped mutability flag)
-const LOAD_SYMBOL = Symbol('load'); +const LOAD_SYMBOL = Symbol('load'); +const MUTABILITY_CHECK = Symbol('mutabilityCheck'); -const DEFAULT_CLONE_DEPTH = 20; -let checkMutability = true; // Check for mutability/immutability on first get +const DEFAULT_CLONE_DEPTH = 20; ... constructor(load) { this[LOAD_SYMBOL] = load; + this[MUTABILITY_CHECK] = true; Util.extendDeep(this, load.config); ... } ... - if (checkMutability) { + if (this[MUTABILITY_CHECK]) { if (!this[LOAD_SYMBOL].initParam('ALLOW_CONFIG_MUTATIONS', false)) { this.util.makeImmutable(this); } - checkMutability = false; + this[MUTABILITY_CHECK] = false; } ... - if (!load.initParam('ALLOW_CONFIG_MUTATIONS', false)) { - checkMutability = true; - } + if (!load.initParam('ALLOW_CONFIG_MUTATIONS', false)) { + this.#config[MUTABILITY_CHECK] = true; + }Also applies to: 117-128, 217-222
|
Huh. Score one for CodeRabbit. |
|
Turns out I cannot implement CodeRabbits PR suggestion, because it found a much wider bug. Even without this feature, the mutability check is a bit broken. Lack of pinning tests made the situation much worse here. Fixed in #865 |
4056cb0 to
a0dff47
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lib/config.js`:
- Around line 300-305: In makeImmutable(object, property, value) ensure the
assignment object = this.#config has a terminating semicolon; locate the
makeImmutable method in lib/config.js and add the missing semicolon after the
assignment to follow proper syntax and avoid potential parsing errors.
- Around line 397-400: The inline comment above the Object.preventExtensions
call concatenates two issue URLs; update the comment at the block that checks
"if (!partial)" (the comment immediately before
Object.preventExtensions(object)) to separate the two links with a space or
newline so they are distinct (e.g., add a space or place each URL on its own
line) ensuring the comment reads "...issue/505 // https://.../865" or uses two
separate lines for the two URLs.
In `@lib/util.js`:
- Around line 800-809: The clone() method on Load currently creates newEnv and a
copy via new Load(this.options, newEnv) but then calls Util.extendDeep(copy,
this), which will overwrite copy.env with the original instance's env; change
the cloning flow to preserve the freshly created env by excluding or skipping
the env property when deep-extending—e.g., remove or ignore this.env from the
source object passed to Util.extendDeep or perform the deep-extend only for
other properties (use a shallow delete/omit of env from this or build a temp
source without env), ensuring copy.env remains newEnv; update references to
Load.clone and Util.extendDeep accordingly.
In `@test/2-config-test.js`:
- Around line 470-472: The variable `sources` is assigned from
config.util.getConfigSources() but never used; either remove the unused
declaration or use it in the subsequent assertion to avoid a redundant call.
Update the snippet that calls config.util.getConfigSources() again to reference
`sources` (e.g., use `sources[0].name` in the first assertion) or simply delete
the `let sources = ...` line if you prefer the existing inline calls; adjust
related assertions on `newConfig` only as needed.
4e0ceaa to
2d36920
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/config.js`:
- Around line 339-343: The loop in value.forEach is missing a terminating
semicolon after the call to this.makeImmutable(item), which can cause lint/parse
errors; update the forEach callback in the block that contains
Util.isObject(item) || Array.isArray(item) so the statement reads with a
semicolon after this.makeImmutable(item) to properly terminate the expression.
- Around line 568-576: The code currently uses filename.match(env) which does
substring matching and creates false positives; change both checks (the NODE_ENV
loop using load.options.nodeEnv and the NODE_APP_INSTANCE check) to match using
a word-boundary-safe RegExp built from the env value (e.g., new RegExp('\\b' +
escapeRegExp(env) + '\\b')) and add an escapeRegExp helper to properly escape
special regex chars before constructing the RegExp; update the calls that
reference filename.match(env) to use this RegExp form and ensure you apply the
same change for the NODE_APP_INSTANCE validation that references
load.getEnv("nodeAppInstance") (or the corresponding variable) so only exact,
delimited environment names match.
♻️ Duplicate comments (3)
lib/config.js (2)
300-305: Missing semicolon after assignment.Line 304 is missing a semicolon after the assignment. This was flagged in a previous review but appears unaddressed.
🔧 Suggested fix
} else if (object === undefined) { - object = this.#config + object = this.#config; }
397-400: Concatenated comment URLs.The two issue URLs on line 398 are concatenated without a separator. This was flagged in a previous review but appears unaddressed.
🔧 Suggested fix
if (!partial) { // Ensure new properties can not be added, as per: - // https://github.com/lorenwest/node-config/issues/505// https://github.com/node-config/node-config/issues/865 + // https://github.com/lorenwest/node-config/issues/505 + // https://github.com/node-config/node-config/issues/865 Object.preventExtensions(object) }test/2-config-test.js (1)
470-472: Unused variablesources.The variable
sourcesis assigned but never used. This was flagged in a previous review.🔧 Suggested fix
- let sources = config.util.getConfigSources(); assert.notEqual(config.util.getConfigSources()[0].name, "Module Defaults"); assert.equal(newConfig.util.getConfigSources()[0].name, "Module Defaults");
🧹 Nitpick comments (1)
lib/config.js (1)
354-359: TODO comment indicates technical debt.The comment notes this function cannot be moved to
util.jsdue to theConfig.prototypereference. Consider tracking this as a follow-up refactor.Would you like me to open an issue to track refactoring
makeImmutableto decouple it from theConfigclass?
2d36920 to
6491946
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/0-util.js`:
- Around line 1554-1585: The final describe block for "Load.clone()" is missing
a terminating semicolon after the closing parenthesis/brace; add the semicolon
immediately after the closing ) of the describe('Load.clone()', ...) block to
fix the syntax (locate the describe block that contains tests checking
Load.clone(), scan(), clone(), getSources() and append the missing semicolon).
🧹 Nitpick comments (1)
lib/config.js (1)
358-361: TODO comment indicates coupling that may need future refactoring.The comment on line 359 notes a dependency on
Config.prototypethat prevents moving this function toutil.js. This is acceptable for now but should be tracked for future cleanup.Consider creating a shared interface or passing the prototype names as a parameter to decouple this method from the Config class, enabling future relocation to the utility module.
d6b5eac to
66d93d5
Compare
Are you scolding me about my own TODO messages, you impertinent robot? Don't make me go get your father. |
|
There's still one issue with this branch.
util is showing, which it does not in master. |
1155668 to
d3f54d8
Compare
|
I would like to land this one also because working on this feature finally revealed to me that makeImmutable can in fact be moved to util. The parts tying it to config.util are all duplicate effort that I can now remove and finally get the rest of the functionality moved over to util.js. |
markstos
left a comment
There was a problem hiding this comment.
I see the use of this. Thanks for the work.
…rack state. Fix for both bugs in node-config#865
Prep work for removing reliance on FIRST_LOAD, in order to support node-config#572
3841144 to
551396b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/config.js`:
- Around line 236-243: The runtime ReferenceError is caused by using an
undefined identifier `Config` inside withModuleDefaults; replace that usage with
the actual class name `ConfigClass` so the copy is constructed from the runtime
class (i.e. change new Config(...) to new ConfigClass(...)), keeping the rest of
the logic intact (references: withModuleDefaults, LOAD_SYMBOL, this.#config,
copy.util.setModuleDefaults).
- Around line 203-235: Update the JSDoc example for withModuleDefaults to show
that it returns a new Config instance: replace the setModuleDefaults example
that calls CONFIG.util.setModuleDefaults(...) and accesses CONFIG.MyModule with
an example that assigns the returned Config (e.g., const newConfig =
CONFIG.util.withModuleDefaults("MyModule", {...})) and then demonstrates reading
properties from that newConfig, and update the text to state that
withModuleDefaults returns a new Config instance so callers know to use the
returned object; refer to the withModuleDefaults function and the Config type in
the description.
In `@test/2-config-test.js`:
- Around line 431-552: The tests fail because withModuleDefaults references an
undefined symbol "Config" instead of the actual class "ConfigClass"; update the
code in the util.withModuleDefaults implementation (where it constructs the new
config object) to use ConfigClass (or the correct exported class name used
elsewhere in the file) when creating the new instance and when checking
instanceof, ensuring all occurrences of "Config" are replaced with "ConfigClass"
(or the module's exported constructor) so withModuleDefaults no longer throws
ReferenceError.
551396b to
9378ea5
Compare
This allows creating a snapshot Config instance with a single Module Defaults override. This implementation is antagonistic to chaining multiple calls to this function. While this was not specifically a goal of this commit, it is feature that should be used lightly.
…implementation. The old code added 'util' to the prototype to sneakily add it to the objects without exposing it to looping.
9378ea5 to
b535895
Compare
This is a PR to address #572
This should not be landed until 4.3 is released.
Hide Whitespace is recommended, since I turned Config and ConfigUtil into classes, and also made
setModuleDefaultsandwithModuleDefaultssibling test suites and indented the old tests by one. Most of the new tests are copies of thesetModuleDefaultstests but asserting that the changes are not shared back to the original object.This is around 160 lines of new tests, 30 lines of JSDoc, and moving an unexported 30 line function out of the middle of a block of exposed methods. The rest is new code.
This PR ended up being a good time to try again to convert Config to a class, which was a bit stranger than I expected it to be. Some fun infinite loop issues when I enumerated the changed set of properties caused by not mangling the object prototype as broadly as the original implementation does.
I also had to get rid of two more file-scope variables to make
withModuleDefaultswork, since the whole point of the new instance is to create a different loadout than the original.Summary by CodeRabbit
New Features
Tests