perf(logger): cache ColorEnabled value

[markH5]

Maybe no need to check it in loop ?
Just check one

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.32%. Comparing base (035c2d7) to head (8a81a74).

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #4006    +/-   ##
========================================
  Coverage   91.32%   91.32%            
========================================
  Files         168      168            
  Lines       10687    10689     +2     
  Branches     3021     3174   +153     
========================================
+ Hits         9760     9762     +2     
  Misses        926      926            
  Partials        1        1            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[yusukebe]

These lines should not be changed.

[markH5]

this change doesn't align with the minor tweak mentioned in the title.
I believe modern JS engines will likely inline it after a few iterations anyway. That said, it still introduces unnecessary runtime overhead.

[usualoma]

It might be better to use a different PR, but I think it would be better to do the following because there is a lot of waste in the current humanize() and time().

/**
 * Humanize the time.
 * @param {number} time - The time to humanize.
 * @returns {string} The humanized time.
 * 
 * @example
 * ```ts
 * humanize(1000) // => 1,000
 * ```
 */
const humanize = (time: number): string =>
  time < 1000 ? String(time) : String(time).replace(/(\d)(?=(\d\d\d)+(?!\d))/g, '$1,')

const time = (start: number): string => {
  const delta = Date.now() - start
  return delta < 1000 ? `${delta}ms` : `${humanize(Math.round(delta / 1000))}s`
}

[usualoma]

Or rather, humanize() only does its job when the request is over 1000 seconds long, but since there are no such long requests normally, I feel that humanize() itself is unnecessary.

[yusukebe]

I like the following implementation. This is because the colorStatus argument can be left as one. I think it's better not to have the colorEnabled argument here.

diff --git a/src/middleware/logger/index.ts b/src/middleware/logger/index.ts
index 5bb45383..a358e4c5 100644
--- a/src/middleware/logger/index.ts
+++ b/src/middleware/logger/index.ts
@@ -26,18 +26,15 @@ const time = (start: number) => {
 }

 const colorStatus = (status: number) => {
-  const colorEnabled = getColorEnabled()
-  if (colorEnabled) {
-    switch ((status / 100) | 0) {
-      case 5: // red = error
-        return `\x1b[31m${status}\x1b[0m`
-      case 4: // yellow = warning
-        return `\x1b[33m${status}\x1b[0m`
-      case 3: // cyan = redirect
-        return `\x1b[36m${status}\x1b[0m`
-      case 2: // green = success
-        return `\x1b[32m${status}\x1b[0m`
-    }
+  switch ((status / 100) | 0) {
+    case 5: // red = error
+      return `\x1b[31m${status}\x1b[0m`
+    case 4: // yellow = warning
+      return `\x1b[33m${status}\x1b[0m`
+    case 3: // cyan = redirect
+      return `\x1b[36m${status}\x1b[0m`
+    case 2: // green = success
+      return `\x1b[32m${status}\x1b[0m`
   }
   // Fallback to unsupported status code.
   // E.g.) Bun and Deno supports new Response with 101, but Node.js does not.
@@ -52,13 +49,14 @@ function log(
   prefix: string,
   method: string,
   path: string,
+  colorEnabled: boolean,
   status: number = 0,
   elapsed?: string
 ) {
   const out =
     prefix === LogPrefix.Incoming
       ? `${prefix} ${method} ${path}`
-      : `${prefix} ${method} ${path} ${colorStatus(status)} ${elapsed}`
+      : `${prefix} ${method} ${path} ${colorEnabled ? colorStatus(status) : status} ${elapsed}`
   fn(out)
 }

[yusukebe]

@usualoma, What do you think of this implementation?

[usualoma]

${colorEnabled ? colorStatus(status) : status}

That's good, I was just thinking the same thing!

[yusukebe]

Hi @markH5

Thank you for the PR. I think this is a good improvement. This will affect the performance issue, so I labeled this PR as perf.

@usualoma What do you think of this?