Support callbacks in config JS/TS/ files to avoid the need for require() in config files#863
Conversation
📝 WalkthroughWalkthroughReplaces the in-file defer implementation with a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Loader as Config Loader
participant ConfigFn as Config File (factory)
participant Util as lib/util
participant Defer as lib/defer
App->>Loader: require/load config file
Loader->>ConfigFn: invoke with ({defer: Defer.deferConfig, util: Util})
ConfigFn->>Defer: create DeferredConfig via deferConfig(...)
ConfigFn-->>Loader: return config object (may contain DeferredConfig or Promises)
Loader->>Util: Util.resolveAsyncConfigs(config)
Util->>Util: traverse config, call prepare on Deferreds, collect Promises
Util->>Defer: call resolve on DeferredConfig instances
Util->>Util: await Promise.all(collectedPromises)
Util-->>Loader: resolved config object
Loader-->>App: finalized config returned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4fd312d to
e6478c4
Compare
|
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. |
|
I think this is the only route to ESM support and I'd like to land this into 4.3 @markstos @lorenwest |
There was a problem hiding this comment.
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 viaObject.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.
There was a problem hiding this comment.
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.
74db19b to
ca283d0
Compare
…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 is prep work for fixing node-config#740
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.
ca283d0 to
df3866b
Compare
There was a problem hiding this comment.
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:errorOnceimplementation is fine; minor style nit onnew Error.
new Error;(line 713) works butnew Error()is more conventional.Error.captureStackTraceis V8-specific but appropriate for a Node.js-only library.test/0-util.js (1)
10-15: Duplicate import ofdeferConfig.Line 11 imports
deferConfigasdefer, and line 15 importsdeferConfigagain 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;
| justThis: defer(function () { | ||
| return "Welcome to this "+this.siteTitle; | ||
| }), | ||
| const { setTimeout } = require('node:timers/promises'); |
There was a problem hiding this comment.
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.
| 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
left a comment
There was a problem hiding this comment.
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
|
I did not end up exposing 'raw' with this. I suppose I probably should? |
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:
Summary by CodeRabbit
New Features
Documentation
Chores
Tests