-
Notifications
You must be signed in to change notification settings - Fork 17
feat: introduce logger #367
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 introduces a new logger system with multiple transports (console and GitHub) to replace the existing reporter system in the linter. The new logger provides structured logging with levels, timestamps, and formatted output across different environments.
Key changes:
- Replaces the existing linter reporter system with a centralized logger infrastructure
- Adds support for console and GitHub Actions transports with proper formatting
- Updates CLI interface to use transport terminology instead of reporter terminology
Reviewed Changes
Copilot reviewed 19 out of 26 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
src/logger/ | New logger infrastructure with constants, utilities, transports, and singleton pattern |
src/linter/index.mjs | Updated to use new logger system instead of reporters |
src/linter/reporters/ | Removed old reporter files |
README.md | Updated CLI documentation to reflect transport terminology |
Comments suppressed due to low confidence (1)
src/logger/tests/logger.test.mjs:195
- This test is marked with
it.only
which means other tests in this file will be skipped during execution. This should be removed to ensure all tests run.
it.only('should log error message', t => {
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.
Will this work on generators? Since, they are each offloaded to a desperate thread?
What if, and it may be far too complicated, we send the logging data back to the parent thread, which has the instantiated logger class, to then handle the incoming data with the appropriate transport, drppping the need to statically have a getInstance
. (We could also re-instantiate the logger on the children, which may be simpler)
Like,
ThreadedLogger.error(...)
on("log", (group, level, message, etc) =>
Will this solution also work with progress bars? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
==========================================
+ Coverage 72.26% 74.71% +2.45%
==========================================
Files 96 107 +11
Lines 9486 10430 +944
Branches 586 679 +93
==========================================
+ Hits 6855 7793 +938
- Misses 2629 2635 +6
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@araujogui OOC is there a package that does this all for us? |
Well, pino is the best option I know |
What about import signale from "signale";
signale.error("...");
signale.pending("..."); It looks like it should be easy to globally configure (https://github.com/klaudiosinani/signale#settingsobj). Each generator can have it's own scope |
It seems like a good option, but I would like the team to discuss this better before I invest more time into this pr. @nodejs/web-infra |
Can we blocked this PR until nodejs/nodejs.org#8057 lands, and linting is removed from here, since it'll change up the transports? |
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.
LGTM! Left a few comments for improvements.
@ovflowd @avivkeller can we reach a consensus about whether using in-house logger or something else? before I put more time in this pr |
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.
I'm okay with landing this as is (and unlocking it, since remark-lint needs my TLC before it can land).
We can always change it later.
@ovflowd rebased, can i merge? |
I still don't love the whole getInstance, I'd prefer using a library that does this all for us / a no-instance alternative We really don't need transports, tbh. |
I'm not going to block, but now that we don't need transports (since we don't have linting), I think we should not add them in this PR
@avivkeller I believe the GitHub transport can come in handy because we mostly run this in CI |
Could you elaborate?
You mean the Singleton approach or? |
@araujogui IMO we don't need the getInstance method since we already do a default export. Can you remove the Singleton approach? The logger should export a named Logger function/constant (like you do) that allows you to create a new Logger instance and that instance is stored within a variable + have the default export being an instance. This ensures that:
But we don't need a getInstance or wrapper/storing ref to the instances globally. |
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.
Could you elaborate?
I poorly worded my comment. I didn't like that transports seemingly required this to use a getInstance
method, but, I was mistaken. My concerns have been self resolved.
You mean the Singleton approach or?
Yes.
Description
Introduces a logger with multiple transports (
console
andgithub
)Validation
npx doc-kit generate -i doc/api/*.md -t web -o out/
Related Issues
Fixes #329
Check List
node --run test
and all tests passed.node --run format
&node --run lint
.