Support callbacks in config JS/TS/ files to avoid the need for require() in config files by jdmarshall · Pull Request #863 · node-config/node-config · GitHub
Skip to content

Support callbacks in config JS/TS/ files to avoid the need for require() in config files#863

Merged
jdmarshall merged 10 commits into
node-config:masterfrom
jdmarshall:deferredESM
Feb 12, 2026
Merged

Support callbacks in config JS/TS/ files to avoid the need for require() in config files#863
jdmarshall merged 10 commits into
node-config:masterfrom
jdmarshall:deferredESM

Conversation

@jdmarshall

@jdmarshall jdmarshall commented Jan 15, 2026

Copy link
Copy Markdown
Collaborator

This fixes #740 and provides a solution for a number of issues preventing ESM migration.

At some point Loren added a future compatibility wiki page to the project.

Since wiki edits do not really show up in the project activity, I was not aware of this, and based on recent events, I don't believe @markstos was either. Several of the clauses have been violated in recent months, which some users have already noted.

One of the clauses in this document has no support in the current codebase:

Ample Warning

A minimum of one year will be given between the time of an incompatibility announcement (below) and the release of the version containing the incompatibility.

If it's possible to detect your application using a future incompatibility, a one-time warning per application run will be placed onto console.error for each type of incompatibility.

Once the incompatibility is introduced, an error will be placed into console.error if an incompatible usage is detected.

Summary by CodeRabbit

  • New Features

    • Deferred configuration gains asynchronous (Promise) resolution, lazy evaluation, and utilities to resolve async configs and emit one-time errors.
    • Configuration modules may be exported as factory functions that receive injected helpers.
  • Documentation

    • Deprecation notices and runtime warnings added to guide migration from legacy APIs.
  • Chores

    • Copyright years updated to 2026.
  • Tests

    • Expanded tests for async/deferred resolution; several legacy assertions were removed or simplified.

@coderabbitai

coderabbitai Bot commented Jan 15, 2026

Copy link
Copy Markdown

@jdmarshall jdmarshall marked this pull request as draft January 15, 2026 04:44
@jdmarshall jdmarshall marked this pull request as ready for review January 15, 2026 04:53
@jdmarshall

jdmarshall commented Jan 19, 2026

Copy link
Copy Markdown
Collaborator Author

This feature is critical to breaking the chicken and egg problems with the config singleton and moving this module to ESM. I have a PR almost ready to go for ESM migration that requires this PR to land. I’d appreciate a timely review while I still have momentum.

Because it deprecates several features, I'd like this to be in 4.3 so that people have plenty of time to adapt before I remove those deprecations in 5.0, which will absolutely be necessary in order to get import to work. You cannot import from modules that are currently being imported, and that is exactly what async.js and defer.js are doing.

Using the callback mechanism means we are exposing the parts of node-config that are stateless or have private state, and thus have already finished importing by the time deferment is resolved.

@jdmarshall jdmarshall added this to the 4.3 milestone Jan 19, 2026
@jdmarshall

Copy link
Copy Markdown
Collaborator Author

I think this is the only route to ESM support and I'd like to land this into 4.3 @markstos @lorenwest

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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/util.js`:
- Around line 1001-1005: The current factory-path treats a returned Promise as a
config object (configObject = fn({ defer: deferConfig, util: Util })) which
breaks downstream code; change this to call the factory and detect if the result
is a Promise (e.g. result && typeof result.then === 'function') and either await
it (so make the enclosing function async and assign configObject = await result)
or explicitly throw a clear error if async factories are unsupported; update
references to configObject/fn/deferConfig/Util and any callers of the enclosing
function to handle the async behavior accordingly.
- Around line 346-372: _iterate currently skips Promise elements inside arrays
because property.forEach(_iterate) never reaches the instanceof Promise check;
change the array branch in _iterate (where it checks property.constructor ===
Array) to iterate by index instead of forEach, and for each element do: if
element is a Promise, push a promise that assigns the resolved value back into
property[i] (e.g., promises.push(element.then(v => { property[i] = v })));
otherwise call _iterate on the element to recurse; keep the final await
Promise.all(promises) so those pushed resolvers write the resolved values back
into config; refer to _iterate, the array branch, promises array, and the final
Promise.all(promises).
🧹 Nitpick comments (2)
lib/util.js (2)

7-8: Inconsistent file extension in require paths.

Line 7 uses './defer.js' (with extension) while line 8 uses '../async' (without). Consider being consistent.


347-349: Heads-up: constructor-based type checks may break for exotic objects.

prop.constructor === String (and similar checks on Object/Array) will fail for objects created via Object.create(null) or from different realms. This is a pre-existing pattern in the codebase so not introduced by this PR, but worth noting if async config resolution encounters such objects.

Comment thread lib/util.js
Comment thread lib/util.js

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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 `@lib/util.js`:
- Line 328: The forEach callback is implicitly returning the value of
defer.resolve(), causing a lint warning; update the deferred.forEach callback so
it uses a block body that calls defer.resolve() without returning it (i.e.,
replace the concise arrow callback with a block-style arrow function that simply
invokes defer.resolve();), targeting the deferred array iteration where
deferred.forEach((defer) => defer.resolve()) is used to ensure no value is
returned from the callback.

Comment thread lib/util.js Outdated
…fig()

This should answer node-config#740 and also opens the door for migrating to ESM modules.

This does not at present address the issue of async.
And move some integration tests to unit tests.

Prep work for adding async support to defer()
This will probably be better overall for anyone doing an incremental
migration since the new code will run last.
https://github.com/node-config/node-config/wiki/Future-Compatibility

"If it's possible to detect your application using a future incompatibility,
a one-time warning per application run will be placed onto console.error for
each type of incompatibility.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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/3-config/default.js`:
- Line 1: The file imports setTimeout from 'node:timers/promises' but never uses
it; remove the unused import to eliminate the dead dependency and any linter
warnings — delete the "const { setTimeout } = require('node:timers/promises');"
statement (or replace it with needed imports if other timers are required) so no
reference to setTimeout remains in the file.
🧹 Nitpick comments (2)
lib/util.js (1)

710-717: errorOnce implementation is fine; minor style nit on new Error.

new Error; (line 713) works but new Error() is more conventional. Error.captureStackTrace is V8-specific but appropriate for a Node.js-only library.

test/0-util.js (1)

10-15: Duplicate import of deferConfig.

Line 11 imports deferConfig as defer, and line 15 imports deferConfig again under its original name. Consider consolidating into one import and using both names.

-const {deferConfig: defer} = require("../defer");
+const {deferConfig: defer, deferConfig} = require("../defer");
 const {asyncConfig} = require("../async");
 const util = require('../lib/util.js').Util;
 const Load = require('../lib/util.js').Load;
-const deferConfig = require('../defer').deferConfig;

Comment thread test/3-config/default.js
justThis: defer(function () {
return "Welcome to this "+this.siteTitle;
}),
const { setTimeout } = require('node:timers/promises');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Unused import: setTimeout from node:timers/promises is never referenced in this file.

-const { setTimeout } = require('node:timers/promises');
-
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { setTimeout } = require('node:timers/promises');
🤖 Prompt for AI Agents
In `@test/3-config/default.js` at line 1, The file imports setTimeout from
'node:timers/promises' but never uses it; remove the unused import to eliminate
the dead dependency and any linter warnings — delete the "const { setTimeout } =
require('node:timers/promises');" statement (or replace it with needed imports
if other timers are required) so no reference to setTimeout remains in the file.

@markstos markstos left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Regarding the "Forward-Compatibilty Clause", I had forgotten about that, as the world was largely standardized on semver for managing expectations about compatibility.

QA Log

  • Reviewed diff

@jdmarshall jdmarshall merged commit bd4d95d into node-config:master Feb 12, 2026
2 checks passed
@jdmarshall

Copy link
Copy Markdown
Collaborator Author

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.

Cannot use defer with esbuild (fix: export defer from config)

2 participants