Skip to content

withModuleDefaults()#864

Merged
jdmarshall merged 5 commits intonode-config:masterfrom
jdmarshall:withModuleDefaults
Feb 15, 2026
Merged

withModuleDefaults()#864
jdmarshall merged 5 commits intonode-config:masterfrom
jdmarshall:withModuleDefaults

Conversation

@jdmarshall
Copy link
Copy Markdown
Collaborator

@jdmarshall jdmarshall commented Jan 17, 2026

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 setModuleDefaults and withModuleDefaults sibling test suites and indented the old tests by one. Most of the new tests are copies of the setModuleDefaults tests 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 withModuleDefaults work, since the whole point of the new instance is to create a different loadout than the original.

Summary by CodeRabbit

  • New Features

    • Config now supports per-instance contexts with validated get/has, optional immutability, per-module defaults (in-place and non-destructive), reloadable file-based configs, deep prototype wiring, exportable serialized configs, visible config sources, runtime strictness checks, and environment/load snapshots.
  • Tests

    • Expanded coverage for cloning/snapshots, immutability enforcement, module defaults, non-destructive defaults, and related regressions.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 17, 2026

Important

Review skipped

This 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 @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

Refactors config into a class-based, per-instance Config (private LOAD_SYMBOL, ConfigUtils) with many instance methods (get/has/setModuleDefaults/withModuleDefaults/makeImmutable/etc.), switches export to an IIFE-created Config instance, adds Env.clone() and Load.clone(), and expands tests for cloning and module defaults.

Changes

Cohort / File(s) Summary
Core Config refactor
lib/config.js
Replaces function-based/global config with class Config wired to a per-instance Load via private LOAD_SYMBOL; adds ConfigUtils, per-instance API (get, has, setModuleDefaults, withModuleDefaults, makeImmutable, attachProtoDeep, toObject, getConfigSources, loadFileConfigs, getEnv, runStrictnessChecks, stripYamlComments), and IIFE-based export creating the default instance.
Utility cloning
lib/util.js
Adds Env.clone() to snapshot environment lookups and Load.clone() to create an independent Load with cloned Env, preserving options and copying instance properties.
Tests — Load.clone
test/0-util.js
Adds tests for Load.clone() covering presence, deep-copy semantics, isolation from original, and retention of original options; minor test scaffolding tweaks.
Tests — Config & module defaults
test/2-config-test.js
Extends tests for immutability after get(), nested mutation blocking, setModuleDefaults() and withModuleDefaults() behavior (prototype attachment, non-destructive defaults, multiple modules, source visibility), and getConfigSources() reflection.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • lorenwest
  • markstos

Poem

🐇 I hop through symbols, classes, and clone,

Defaults snug in burrows each module can own,
Prototypes stitched deep in each nested tree,
I lock fields from change so configs stay free,
Tests nibble carrots and cheer — hip-hop-hooray!

🚥 Pre-merge checks | ✅ 2 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title 'withModuleDefaults()' is overly narrow and does not capture the main change, which is a comprehensive refactor of Config into a class-based architecture with multiple new features and per-instance wiring. Consider revising the title to reflect the primary architectural change, such as 'Refactor Config to class-based architecture with withModuleDefaults' or similar, to better represent the scope of changes.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into master

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@jdmarshall jdmarshall changed the title With module defaults withModuleDefaults() Jan 17, 2026
@jdmarshall jdmarshall added this to the 4.4 milestone Jan 17, 2026
@jdmarshall jdmarshall force-pushed the withModuleDefaults branch 2 times, most recently from 74b25b7 to 4056cb0 Compare January 17, 2026 03:51
@jdmarshall jdmarshall marked this pull request as ready for review January 19, 2026 20:24
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

checkMutability is module-scoped, so the first get() on any Config instance disables immutability for all others (including those created by withModuleDefaults). 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

@jdmarshall
Copy link
Copy Markdown
Collaborator Author

Huh. Score one for CodeRabbit.

@jdmarshall
Copy link
Copy Markdown
Collaborator Author

jdmarshall commented Jan 20, 2026

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jdmarshall jdmarshall force-pushed the withModuleDefaults branch 2 times, most recently from 4e0ceaa to 2d36920 Compare January 20, 2026 01:45
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 variable sources.

The variable sources is 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.js due to the Config.prototype reference. Consider tracking this as a follow-up refactor.

Would you like me to open an issue to track refactoring makeImmutable to decouple it from the Config class?

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.prototype that prevents moving this function to util.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.

@jdmarshall
Copy link
Copy Markdown
Collaborator Author

Actionable comments posted: 1
🤖 Fix all issues with AI agents

🧹 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.prototype that prevents moving this function to util.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.

Are you scolding me about my own TODO messages, you impertinent robot? Don't make me go get your father.

@jdmarshall
Copy link
Copy Markdown
Collaborator Author

There's still one issue with this branch.

Object.keys(client)

Array(13) [value,siteTitle,latitude,longitude,a,b,c,fromList,list,welcomeEmail,map,original,util]

util is showing, which it does not in master.

@jdmarshall jdmarshall modified the milestones: 4.4, 5.0 Jan 20, 2026
@jdmarshall
Copy link
Copy Markdown
Collaborator Author

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.

Copy link
Copy Markdown
Collaborator

@markstos markstos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the use of this. Thanks for the work.

Prep work for removing reliance on FIRST_LOAD, in order to support node-config#572
@jdmarshall jdmarshall force-pushed the withModuleDefaults branch 2 times, most recently from 3841144 to 551396b Compare February 15, 2026 03:41
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
@jdmarshall jdmarshall merged commit 0a95bb1 into node-config:master Feb 15, 2026
2 checks passed
@jdmarshall jdmarshall deleted the withModuleDefaults branch February 26, 2026 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants