Replace essential module system with isFatal errors and bootstrap libraries#112
Replace essential module system with isFatal errors and bootstrap libraries#112
Conversation
…p libraries Absorbs config, logger, errors, and lang into core as bootstrap libraries loaded before any modules initialise. Replaces the two-phase essential/ non-essential loading with single-pass loading where any error can be marked isFatal in its JSON definition to halt the app. waitForModule handles all load ordering.
Merges all error JSON and config schema properties from the absorbed modules (errors, config, lang, logger) into core. Logger config keys use loggerX prefix (e.g. loggerLevels, loggerMute). Lang defaultLang included as-is.
Empty loggerLevels array now implies mute. Removes loggerMute and loggerDateFormat config options. Timestamps always use ISO format.
Modules can define conf/deprecated.json to map old config keys to new ones. Supports _module (check a different module section) and _fatal (throw instead of warn). Core includes deprecation files for absorbed logger and lang modules.
|
The docs and doc plugins from the absorbed modules still need to be merged into core: adapt-authoring-config:
adapt-authoring-errors:
( |
Config deprecation warnings are superseded by the migrations module's new conf migration type, which can actually rewrite config files rather than just warning about stale keys.
Migrates config keys from their old module sections to adapt-authoring-core: - adapt-authoring-logger.levels → adapt-authoring-core.logLevels - adapt-authoring-logger.showTimestamp → adapt-authoring-core.showLogTimestamp - adapt-authoring-logger.mute → removed - adapt-authoring-logger.dateFormat → removed - adapt-authoring-lang.defaultLang → adapt-authoring-core.defaultLang
Adds resolveDirectory() to Config which handles $ROOT, $DATA, $TEMP variable substitution. This was previously done by jsonschema's isDirectory keyword, but config now loads before jsonschema. JsonSchemaModule's isDirectory keyword now delegates to this method.
- Call runMigrations() in App.start() between loadConfigs() and loadLibraries(), so config migrations run before schema validation - Fix Schemas().init() not being called in Config.storeSchemaSettings(), which caused all schema defaults to be silently skipped - Improve config validation error formatting with chalk colours Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Create Logger with defaults at the start of App.start(), before loadConfigs/migrations/loadLibraries. Config and migrations now use the logger instead of raw console.log + chalk. Logger is reconfigured with user settings once Config is loaded. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Simplify getModuleOverrides with filter/startsWith - Inline colourise and getDateStamp into log method - Merge early returns in log method Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merge load() into loadModules() by defaulting the modules parameter to all configured dependencies. App.start() now calls loadModules() directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The _isReady check is sufficient to guard against double-start. The setReady override was only resetting _isStarting. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rootDir is determined by cwd/ROOT_DIR env var at construction time. Allowing config to override it after the fact is circular — config, dependencies, and metadata have already been loaded using the original rootDir by this point. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move app, args, dependencyloader setup to constructor - Inline loadLibraries into start() - Move verbose DIR/GIT logs to end of start() - Simplify init() to just error handling and failed module reporting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use instanceof Error instead of constructor name string check - Use fs.readFile with utf8 encoding directly - Remove getPhrasesForLang (callers can use phrases[lang] directly) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace lodash _.isFunction with typeof checks - Replace fs-extra with core readJson util - Combine two sorts in loadConfigs into one - Use Object.fromEntries for init times - Lazy-create DependencyError in waitForModule - Remove eslint directive and cosmetic cleanup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace plain Error throws in DependencyLoader with AdaptError definitions: DEP_ALREADY_LOADED, DEP_FAILED, DEP_INVALID_EXPORT, DEP_MISSING, DEP_NO_ONREADY, DEP_TIMEOUT. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace lodash _.isFunction with typeof check - Replace lodash _.cloneDeep with native structuredClone - Replace minimist with native util.parseArgs - fs-extra was already unused after readJson util was added Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add adapt-authoring-migrations as dependency, static import - Remove tests for removed methods (logError, getConfig, getPhrasesForLang, colourise, getDateStamp, setReady/_isStarting) - Update DependencyLoader tests to use AdaptError codes - Update Config storeUserSettings test for new signature - Update getArgs tests for util.parseArgs shape - Fix lint: remove padded block in DependencyLoader Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
All App tests rely on App.instance which triggers a real boot, making them integration tests rather than unit tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move instance variable definitions into init() to fix race condition where super() triggers init() before constructor body runs. Make Config, Errors, and Lang use consistent constructor patterns with shared options object. Convert Errors and Lang to synchronous. Use log function instead of logger instance across all libraries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Config schemas lack $id, causing AJV "already exists" collisions when multiple schemas compile into the same validator instance. Adds a temporary workaround to set $id from the module name until config schemas define their own. Also logs non-file-not-found schema errors instead of swallowing them silently. See adapt-security/adapt-authoring-jsonschema#66 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove logWarn param from instance translate/translateError signatures to match how callers actually use them (lang, key, data) - Store defaultLang from config rather than requiring it per-call - Merge translateError into translate (handles Error keys inline) - Extract substituteData for readability - Remove static translate/translateError (instance methods are the API) - Pass defaultLang config value from App Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only used in one place, no need for a separate static method. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use [v].flat() to handle both scalar and array values in a single code path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously, when any module failed to load, the moduleLoadedHook fired with (error, undefined), causing every pending waitForModule promise to reject — even for unrelated modules. This led to misleading errors (e.g. reporting mongodb as failed when ui was the actual failure). Now the failed module name is passed through the hook, and waitForModule only rejects if the specific module being waited for is the one that failed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Load lang phrases from lang/<language>/<namespace>.json instead of lang/<language>.<namespace>.json. Plain lang/<language>.json files still work for simple cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces the “essential vs non-essential” module loading mechanism with isFatal error-based halting, and moves foundational utilities (config, logger, errors, lang) into core bootstrap libraries available before modules initialise.
Changes:
- Introduces new core bootstrap libraries:
Config,Logger,Errors,Lang, plusAdaptErrorwithisFatal. - Refactors
DependencyLoaderto single-pass loading and fatal/non-fatal failure handling, and simplifiesAbstractModuleconfig/logging access. - Adds migrations + documentation pages/plugins and updates test suites accordingly.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils-getArgs.spec.js | Adjusts CLI arg parsing expectations for new getArgs() implementation. |
| tests/Logger.spec.js | Adds unit tests for new Logger library helpers + basic logging. |
| tests/Lang.spec.js | Adds unit tests for new Lang library phrase loading/translation. |
| tests/Errors.spec.js | Adds unit tests for error registry behavior (currently mismatched with implementation API). |
| tests/DependencyLoader.spec.js | Updates loader tests for isFatal behavior and new error codes. |
| tests/Config.spec.js | Adds unit tests for new Config library (currently calls new Config() without required opts). |
| tests/App.spec.js | Removes the App test suite. |
| tests/AdaptError.spec.js | Adds unit tests for new AdaptError class. |
| tests/AbstractModule.spec.js | Updates module logging/config tests to reflect bootstrap libraries. |
| package.json | Replaces dependencies (adds chalk/adapt-schemas/migrations) and removes others (currently breaks fs-extra usage). |
| migrations/3.0.0-conf-migrate-logger-config.js | Migration for legacy logger config keys. |
| migrations/3.0.0-conf-migrate-lang-config.js | Migration for legacy lang config keys. |
| lib/utils/getArgs.js | Switches from minimist to node:util.parseArgs. |
| lib/Logger.js | Adds new configurable console logger with levels/overrides + hook. |
| lib/Lang.js | Adds phrase loading and translation utilities. |
| lib/Hook.js | Drops lodash usage and switches deep clone to structuredClone. |
| lib/Errors.js | Adds error registry that loads module errors/*.json into AdaptError getters. |
| lib/DependencyLoader.js | Single-pass loading, fatal checks, updated wait semantics (currently has a resolve-on-failure bug). |
| lib/Config.js | Adds config loading/env/schema validation (currently doesn’t tolerate missing opts/log). |
| lib/App.js | Bootstraps logger/config/errors/lang before loading modules; runs migrations. |
| lib/AdaptError.js | Adds AdaptError class with isFatal and metadata/data support. |
| lib/AbstractModule.js | Simplifies config access and logs directly via bootstrap app.logger. |
| index.js | Exports new libraries from the package entrypoint. |
| errors/node-core.json | Adds Node core error definitions. |
| errors/errors.json | Adds dependency/config/lang-related core error definitions with isFatal flags. |
| docs/plugins/errorsref.md | Adds template page for generated error reference. |
| docs/plugins/errors.js | Adds docs generator for errors reference (currently has recursive formatting bug). |
| docs/plugins/configuration.md | Adds template page for generated configuration reference. |
| docs/plugins/configuration.js | Adds docs generator for configuration reference from schemas. |
| docs/error-handling.md | Adds error-handling guide (currently contains duplicate headings/typos/outdated example). |
| docs/configure-environment.md | Adds environment configuration documentation. |
| conf/deprecated-logger.json | Maps deprecated logger config keys to new core keys. |
| conf/deprecated-lang.json | Maps deprecated lang config key to new core key. |
| conf/config.schema.json | Extends core schema with logging + default language config. |
Comments suppressed due to low confidence (1)
package.json:22
package.jsonno longer listsfs-extra, but it's imported by multiple test files (and at least one docs plugin). A clean install will fail at runtime when runningnpm test. Either addfs-extraback (likely as adevDependency) or migrate the tests/tools tonode:fs/node:fs/promises.
"dependencies": {
"adapt-authoring-migrations": "github:adapt-security/adapt-authoring-migrations#feature/boot-phase-migrations",
"adapt-schemas": "^3.1.0",
"chalk": "^5.4.1",
"glob": "^13.0.0"
},
"devDependencies": {
"@adaptlearning/semantic-release-config": "^1.0.0",
"standard": "^17.1.0"
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Thanks for the review — here's the response to each comment: 1. 2. 3. 4. 5. 6. 7. 8. 9. Also fixed lint issues in All 271 tests pass and |
- Fix waitForModule to only reject for the specific waited-for module - Fix dataToMd recursive accumulator duplication in docs plugin - Add defaults to Config/Lang/Errors constructors for test compatibility - Add .catch() on Logger logHook to prevent unhandled promise rejections - Fix error-handling.md duplicate heading, typo, and outdated references - Update Errors tests to use constructor instead of non-existent .load() - Clean up unused imports in Config tests - Add fs-extra as devDependency - Fix lint issues in App.js Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Agent-Logs-Url: https://github.com/adapt-security/adapt-authoring-core/sessions/772703ac-791f-4bfa-9beb-854b2c913014 Co-authored-by: taylortom <1059083+taylortom@users.noreply.github.com>
…ries' into feature/isfatal-and-absorb-libraries Co-authored-by: taylortom <1059083+taylortom@users.noreply.github.com>
All six doc files were already merged in commit |
Moves the NODE_ENV default from bin/start.js into core so all consumers get it automatically. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Calls setReady with the error before process.exit(1) so that onReady listeners (e.g. tests) receive the rejection before the process dies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Breaking
essentialApis,essentialTypeno longer used)forceoption fromDependencyLoader.loadModules()app.config,app.logger,app.errors,app.langare now bootstrap library instances, not AbstractModule subclasseswaitForModule('config'),waitForModule('errors'), orwaitForModule('logger')no longer need to — these are available immediatelyLang.translate()signature changed from(defaultLang, logWarn, lang, key, data)to(lang, key, data)— callers no longer passlogWarnordefaultLangLang.translateError()removed — useLang.translate(lang, error)instead (Error keys handled automatically)Lang.storeStrings()static method removedLang.translate()andLang.translateError()removed — use instance methodsNew
AdaptErrorclass withisFatalproperty, set from error JSON definitionsConfiglibrary — loads user settings, env vars, and validates module schemas at bootstrapLoggerlibrary — configurable console logging with levels, colours, and module overridesErrorslibrary — loads and registers all error definitions from moduleerrors/*.jsonfilesLanglibrary — loads and merges language phrases from modulelang/*.jsonfiles, with translate utilitiesApp.loadLibraries()before any modules initialiseUpdate
DependencyLoaderuses single-pass module loading —waitForModulehandles all orderingDependencyLoader.loadModulescheckserror.isFatal/error.cause.isFatalto decide whether to halt or continueDependencyLoader.waitForModuleonly rejects when the specific waited-for module fails, not on any unrelated failure — prevents misleading error cascadingDependencyLoaderpasses failed module name throughmoduleLoadedHookfor targeted error reportingAbstractModule.log()calls logger directly (no more deferred hook-based logging)AbstractModule.getConfig()simplified — no try/catch needed since config is always availableDependencyLoader.log()calls logger directly (no moreconsole.logfallback)DependencyLoader.getConfig()simplified — no_isReadycheck neededLang.translate()simplified — removedlogWarncallback parameter, uses storedlogfunction internallyLang.translate()now handles Error keys directly (mergedtranslateErrorfunctionality)LangusesdefaultLangfrom core config instead of requiring it per-callLang.substituteData()extracted for readability, uses[v].flat()to normalise array/scalar handlingLang.storeStrings()inlined intoloadPhrases— no longer a separate static methodApppassesdefaultLangconfig value toLangconstructorConfig,Lang, andErrorsconstructors now default their options argument so instances can be constructed with no arguments (e.g. in unit tests)Logger.log()guards against unhandled promise rejections fromlogHook.invoke()with a.catch()handlerdocs/configure-environment.md,docs/plugins/configuration.js,docs/plugins/configuration.mdfromadapt-authoring-configdocs/error-handling.md,docs/plugins/errors.js,docs/plugins/errorsref.mdfromadapt-authoring-errorsdocs/plugins/errors.jsdataToMd()recursive accumulator duplication — each recursive call now starts from an empty stringdocs/error-handling.md— removed duplicate heading, corrected## User errorstypo, updated outdatedErrorsModule/waitForModulereferences to useapp.errorsTesting
AdaptError.spec.js,Config.spec.js,Errors.spec.js,Lang.spec.js,Logger.spec.jsDependencyLoader.spec.js— replacedforceoption tests withisFataltestsAbstractModule.spec.js— simplified log/getConfig testsErrors.spec.js— uses constructor (new Errors(...)) instead of non-existentErrors.load()Lang.spec.jsupdated to use instance methods withcreateLanghelperMigration notes for other modules
Configlibrary. Route serving public config should move toadapt-authoring-serverErrors/AdaptError. Modules should mark critical errors as"isFatal": truein theirerrors/*.jsonLogger.mongodbloggershould update to useapp.logger.logHookLang.req.translatemiddleware should move toadapt-authoring-middlewaretranslationUtils()middleware to bindreq.translateon API requests (previously inadapt-authoring-lang)essentialTypefromadapt-authoring.json, mark connection/init errors as"isFatal": trueessentialApisfromadapt-authoring.json, remove deprecated module dependencies🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.