-
Notifications
You must be signed in to change notification settings - Fork 21
Feature/admin forth/1137/lets remove heavy debug and in #445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Feature/admin forth/1137/lets remove heavy debug and in #445
Conversation
…ector, and socketBroker modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes the logging infrastructure by replacing scattered console.log, console.error, and environment-based debug statements with a structured logging system using pino and pino-pretty. The implementation introduces three logger instances (logger, afLogger, dbLogger) for different logging contexts, each configurable via environment variables.
Changes:
- Introduced centralized logging module using pino with three distinct loggers for user logs, AdminForth internal logs, and database query logs
- Replaced
console.log/console.errorandprocess.env.HEAVY_DEBUG && console.log()patterns with appropriate logger calls throughout the codebase - Added pino and pino-pretty dependencies to package.json
- Updated documentation and templates to use the new logger
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| adminforth/modules/logger.ts | New centralized logging module with three logger instances |
| adminforth/package.json | Added pino and pino-pretty dependencies |
| adminforth/servers/express.ts | Replaced console.log with afLogger calls |
| adminforth/modules/socketBroker.ts | Converted debug logs to afLogger.trace |
| adminforth/modules/restApi.ts | Replaced console.log with afLogger calls |
| adminforth/modules/codeInjector.ts | Converted debug and info logs to afLogger |
| adminforth/index.ts | Exported logger and replaced console.log with afLogger |
| adminforth/dataConnectors/*.ts | Replaced database debug logs with dbLogger |
| dev-demo/resources/*.ts | Updated to use logger from adminforth |
| dev-demo/index.ts | Replaced console.log with logger calls |
| adminforth/spa/src/components/*.vue | Removed commented-out console.log statements |
| adminforth/documentation/**/*.md | Updated examples to use logger |
| adminforth/commands/**/*.js | Replaced console.log with afLogger calls |
Files not reviewed (1)
- adminforth/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.log('SPA dependencies not found'); | ||
| console.log('current directory', import.meta.dirname); | ||
| afLogger.warn('SPA dependencies not found'); | ||
| afLogger.info('current directory', import.meta.dirname); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using afLogger.warn instead of console.log for 'SPA dependencies not found' is correct, but the following line should also use afLogger.warn or afLogger.info instead of afLogger.info for 'current directory' since it's related to the same warning context.
| afLogger.info('current directory', import.meta.dirname); | |
| afLogger.warn('current directory', import.meta.dirname); |
| @@ -0,0 +1,29 @@ | |||
| import pino from 'pino'; | |||
| import { PinoPretty } from "pino-pretty"; | |||
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement 'import { PinoPretty } from "pino-pretty"' is importing a type that is not being used in the code. This import should be removed as PinoPretty is never referenced in the file.
| import { PinoPretty } from "pino-pretty"; |
| } | ||
| this.expressApp.get(`${slashedPrefix}assets/*`, handler); | ||
| process.env.HEAVY_DEBUG && console.log('®️ Registering SPA serve handler', `${slashedPrefix}assets/*`); | ||
| afLogger.trace(`®️ Registering SPA serve handler', ${slashedPrefix}assets/*`); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The afLogger.trace call has a mismatched quote character - it starts with a backtick but contains a single quote in the middle: '®️ Registering SPA serve handler', ${slashedPrefix}assets/*. This should use consistent template literal syntax throughout.
| await this.client.connect(); | ||
| this.client.on('error', async (err) => { | ||
| console.log('Postgres error: ', err.message, err.stack) | ||
| afLogger.error(`Postgres error: ${err.message} ${err.stack}`); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PostgreSQL connector is using afLogger.error for error logging, but should be using the dbLogger for database-related errors to maintain consistent logging layers. The error handling code at line 20 should use dbLogger.error instead.
| afLogger.error(`Postgres error: ${err.message} ${err.stack}`); | |
| dbLogger.error(`Postgres error: ${err.message} ${err.stack}`); |
| // execute hook if needed | ||
| for (const hook of listify(resource.hooks?.create?.beforeSave)) { | ||
| console.log('🪲 Hook beforeSave', hook); | ||
| console.debug('🪲 Hook beforeSave', hook); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The console.debug call should be replaced with logger.debug for consistency with the rest of the logging changes in this PR.
| console.debug('🪲 Hook beforeSave', hook); | |
| afLogger.debug('🪲 Hook beforeSave', hook); |
| afLogger.error(`Mongo error: ${err.message}`); | ||
| }); | ||
| console.log('Connected to Mongo'); | ||
| afLogger.info('Connected to Mongo'); | ||
| } catch (e) { | ||
| console.error(`Failed to connect to Mongo: ${e}`); | ||
| afLogger.error(`Failed to connect to Mongo: ${e}`); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MongoDB error logging is using afLogger for database connection errors. For consistency, these should use dbLogger instead, similar to the other database connectors. This applies to lines 39, 41, and 43.
| } | ||
| }); | ||
| devServer.stderr.on('data', (data) => { | ||
| console.error(`[AdminForth SPA ERROR]:`); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The console.error call should be replaced with afLogger.error for consistency with the rest of the logging changes in this PR.
| console.error(`[AdminForth SPA ERROR]:`); | |
| afLogger.error(`[AdminForth SPA ERROR]:`); |
| @@ -1,5 +1,5 @@ | |||
| import AdminForth, { AdminForthDataTypes } from 'adminforth'; | |||
| import type { AdminForthResourceInput, AdminForthResource, AdminUser } from 'adminforth'; | |||
| import type { AdminForthResourceInput, AdminForthResource, AdminUser, logger } from 'adminforth'; | |||
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the template file, the import statement is importing 'logger' as a type from 'adminforth', but logger is not a type - it's a value/export. The import should be: import { Filters, logger } from 'adminforth'; without the 'type' keyword before AdminForthResourceInput.
| import type { AdminForthResourceInput, AdminForthResource, AdminUser, logger } from 'adminforth'; | |
| import type { AdminForthResourceInput, AdminForthResource, AdminUser } from 'adminforth'; | |
| import { logger } from 'adminforth'; |
| if (!silent) { | ||
| parsed.capturedLogs.forEach((log) => { | ||
| console.log(...log); | ||
| afLogger.log(...log); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The afLogger.log method is being called, but pino loggers don't have a .log() method. This should be changed to an appropriate log level like afLogger.info() or afLogger.debug().
| afLogger.log(...log); | |
| const [level, ...args] = log; | |
| if (level && typeof afLogger[level] === "function") { | |
| afLogger[level](...args); | |
| } else { | |
| afLogger.info(...log); | |
| } |
| try { | ||
| await fs.mkdir(customDirPath, { recursive: true }); | ||
| console.log(chalk.dim(`Ensured custom directory exists: ${customDirPath}`)); | ||
| afLogger.warn(chalk.dim(`Ensured custom directory exists: ${customDirPath}`)); |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The afLogger.warn call should be afLogger.info since this is informational logging about ensuring a directory exists, not a warning condition.
| afLogger.warn(chalk.dim(`Ensured custom directory exists: ${customDirPath}`)); | |
| afLogger.info(chalk.dim(`Ensured custom directory exists: ${customDirPath}`)); |
No description provided.