[improvement-854] Added variable debug message functionality for#864
[improvement-854] Added variable debug message functionality for#864denete wants to merge 5 commits intoaccius:Stagingfrom
Conversation
accius
left a comment
There was a problem hiding this comment.
PR Review — Debug Logging System
Hey Dee, thanks for tackling #854 — this is a good direction. A few things to address before merge:
Bug: Scoped logger fights its own console override
logger.js calls console[level](...) — but consoleOverride.js already replaced those with noops. So if the log level silences console.info, then logger.info('MyScope', ...) is also silenced even when the scope matches.
Fix: The logger should use the saved originalConsole references instead of the overridden console.* methods. Either export originalConsole from consoleOverride.js, or pass it into createLogger().
Bug: Level index mapping is off
consoleOverride.js uses two arrays that don't align:
const order = ['error', 'warn', 'info', 'log']; // 4 items
const allowedIndex = ['none', 'error', 'warn', 'info', 'debug', 'all'].indexOf(config.logLevel); // 6 itemsThe comparison index > allowedIndex compares positions in two different-length arrays with different semantics. For example, log=warn gives allowedIndex=2, which would allow order[0] (error) and order[1] (warn) but also order[2] (info) — that shouldn't be shown at warn level.
Default log=none silences errors
With no query params, all console output is suppressed including console.error. In production, this means real errors are invisible unless someone knows to add ?log=error. Recommend defaulting to warn or error so problems are always surfaced — devs can use ?log=none explicitly if they truly want silence.
Unused hooks and config
useFeature, useDebugFlag, useRole, useForcedState and the corresponding config fields (features, disable, debug, role, state, perf) are parsed but nothing consumes them. I'd prefer to ship only what's needed now (log level + scope) and add these when there's a use case — keeps the surface area small.
No existing code migrated
This adds the framework but none of the existing console.log calls are converted. The only runtime effect right now is silencing all output. Would be good to see at least a few noisy components migrated as proof-of-concept — maybe the ones that originally motivated #854.
Looks good
- Clean separation: config parsing, logger, console override, React context
- Scoped logging is a nice idea for isolating component output
- Query param approach is zero-config for end users
- Good README documentation
|
@accius All comments addressed. |
accius
left a comment
There was a problem hiding this comment.
Thanks for the update Dee — the core logic in consoleOverride.js and debugConfig.js is clean and well-structured. A few things to address:
DebugProvider should be a plain function call, not a component
DebugProvider calls overrideConsole() on every render — including StrictMode double-renders and any re-render of the tree above it. Since it doesn't use any React features (no state, no context, no effects), it should just be a direct call in main.jsx before the React tree mounts:
import { getDebugConfig } from './debug/debugConfig';
import { overrideConsole } from './debug/consoleOverride';
overrideConsole(getDebugConfig());
ReactDOM.createRoot(document.getElementById('root')).render(...)This avoids re-execution on renders and removes the misleading "Provider" pattern. DebugProvider.jsx can be deleted entirely.
console.debug is not overridden
Some libraries use console.debug — it will leak through regardless of the level setting. If the goal is to silence noisy console output, this is a gap worth closing.
Minor
- The README is a nice touch 👍
- The level map and method map in
consoleOverride.jsare well done
messages sent to the console. This overrides current console.* and also provides for future scoped logger.* functionality.
addressed other comments
console.trace to overridden methods
|
Updated with feedback from PR comment. |
What does this PR do?
Added variable debug message functionality for messages sent to the console. This overrides current console.* and also provides for future scoped logger.* functionality.
Type of change
How to test
(Notes on the usage of querystring values are included in src/debug/README.md)
Examples:
Checklist
server.js: caches have TTLs and size caps (we serve 2,000+ concurrent users)var(--accent-cyan), etc.).bak,.old,console.logdebug lines, or test scripts included