-
-
Notifications
You must be signed in to change notification settings - Fork 741
feat(logger): support for NO_COLOR
on cloudflare workers
#4094
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
feat(logger): support for NO_COLOR
on cloudflare workers
#4094
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #4094 +/- ##
==========================================
- Coverage 91.47% 91.36% -0.12%
==========================================
Files 168 168
Lines 10791 10753 -38
Branches 3088 3107 +19
==========================================
- Hits 9871 9824 -47
- Misses 919 928 +9
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I also have another plan, #4095. |
If it's ready to review, please ping me:) |
9d25f4c
to
d036a85
Compare
Old Cloudflare workers are missing import('cloudflare:workers')
@yusukebe This is now ready for review. |
ab269b1
to
4a2e1c1
Compare
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!
Thanks @ryuapp ! I'll merge this later and include the next minor release (maybe shipped soon). |
…r-on-cloudflare-workers
navigator !== undefined && navigator.userAgent === 'Cloudflare-Workers' | ||
? // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore | ||
'NO_COLOR' in ((await import('cloudflare:workers')).env ?? {}) // ?? {} is for backward compat |
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.
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.
@ysm-dev Can you create an issue?
closes #3751
Note
Currently,
process.env
is available on Cloudflare Workers withnodejs_compat
flag, so there may not be a need to support it.https://developers.cloudflare.com/workers/runtime-apis/nodejs/process/#processenv
Logger now uses
getColorEnabledAsync()
to support retrieving cloudflare env. Since we couldn't support this without dynamic import now, I created a newgetColorEnabled()
that is asynchronous.getColorEnabled()
is maintained for backwards compatibility forshowRoute()
.The cost of dynamic imports has not been investigated, so it may be necessary to optimize it with following PR:
The author should do the following, if applicable
bun run format:fix && bun run lint:fix
to format the code