Skip to content
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.

Sign up for GitHub

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

Jump to bottom

perf(middleware/logger): optimize color status #3603

Merged
merged 6 commits into from
Nov 2, 2024
Merged

perf(middleware/logger): optimize color status #3603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a744446
refactor(logger): optimize colors status
exoego Oct 31, 2024
9e9e900
refactor(logger): add failing test
exoego Oct 31, 2024
0b7121a
refactor(logger): remove unreachable cases. Response accepts 200-599
exoego Oct 31, 2024
e5adc98
refactor(logger): added fallback
exoego Oct 31, 2024
e6b8041
perf(middleware/logger): cleanup tests
exoego Nov 2, 2024
66f35bf
perf(middleware/logger): test unsupported status code forcibly
exoego Nov 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions src/middleware/logger/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,17 @@ describe('Logger by Middleware', () => {
return c.text(longRandomString)
})
app.get('/empty', (c) => c.text(''))
app.get('/redirect', (c) => {
return c.redirect('/empty', 301)
})
app.get('/server-error', (c) => {
const res = new Response('', { status: 511 })
if (c.req.query('status')) {
// test status code not yet supported by runtime `Response` object
Object.defineProperty(res, 'status', { value: parseInt(c.req.query('status') as string) })
}
return res
})
})

it('Log status 200 with empty body', async () => {
Expand Down Expand Up @@ -62,6 +73,14 @@ describe('Logger by Middleware', () => {
expect(log).toMatch(/1s/)
})

it('Log status 301 with empty body', async () => {
const res = await app.request('http://localhost/redirect')
expect(res).not.toBeNull()
expect(res.status).toBe(301)
expect(log.startsWith('--> GET /redirect \x1b[36m301\x1b[0m')).toBe(true)
expect(log).toMatch(/m?s$/)
})

it('Log status 404', async () => {
const msg = 'Default 404 Not Found'
app.all('*', (c) => {
Expand All @@ -73,6 +92,30 @@ describe('Logger by Middleware', () => {
expect(log.startsWith('--> GET /notfound \x1b[33m404\x1b[0m')).toBe(true)
expect(log).toMatch(/m?s$/)
})

it('Log status 511 with empty body', async () => {
const res = await app.request('http://localhost/server-error')
expect(res).not.toBeNull()
expect(res.status).toBe(511)
expect(log.startsWith('--> GET /server-error \x1b[31m511\x1b[0m')).toBe(true)
expect(log).toMatch(/m?s$/)
})

it('Log status 100', async () => {
const res = await app.request('http://localhost/server-error?status=100')
expect(res).not.toBeNull()
expect(res.status).toBe(100)
expect(log.startsWith('--> GET /server-error 100')).toBe(true)
expect(log).toMatch(/m?s$/)
})

it('Log status 700', async () => {
const res = await app.request('http://localhost/server-error?status=700')
expect(res).not.toBeNull()
expect(res.status).toBe(700)
expect(log.startsWith('--> GET /server-error 700')).toBe(true)
expect(log).toMatch(/m?s$/)
})
})

describe('Logger by Middleware in NO_COLOR', () => {
Expand Down
27 changes: 15 additions & 12 deletions src/middleware/logger/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,22 @@ const time = (start: number) => {

const colorStatus = (status: number) => {
const colorEnabled = getColorEnabled()
const out: { [key: string]: string } = {
7: colorEnabled ? `\x1b[35m${status}\x1b[0m` : `${status}`,
5: colorEnabled ? `\x1b[31m${status}\x1b[0m` : `${status}`,
4: colorEnabled ? `\x1b[33m${status}\x1b[0m` : `${status}`,
3: colorEnabled ? `\x1b[36m${status}\x1b[0m` : `${status}`,
2: colorEnabled ? `\x1b[32m${status}\x1b[0m` : `${status}`,
1: colorEnabled ? `\x1b[32m${status}\x1b[0m` : `${status}`,
0: colorEnabled ? `\x1b[33m${status}\x1b[0m` : `${status}`,
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`
}
}

const calculateStatus = (status / 100) | 0

return out[calculateStatus]
// Fallback to unsupported status code.
// E.g.) Bun and Deno supports new Response with 101, but Node.js does not.
// And those may evolve to accept more status.
return `${status}`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line is covered for color-disabled case.
But not covered for currently unsupported status code, because test always fail if we test 100 or 700.
This fallback will help when Response supports 100 or other in future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @exoego, thanks for creating the pull request.

It's a fine line as to whether we need to overwrite the object and test it, but it's better if there is test, so how about testing it with the following code?

diff --git a/src/middleware/logger/index.test.ts b/src/middleware/logger/index.test.ts
index 0623fc64..7c7e4689 100644
--- a/src/middleware/logger/index.test.ts
+++ b/src/middleware/logger/index.test.ts
@@ -32,7 +32,12 @@ describe('Logger by Middleware', () => {
       return c.redirect('/empty', 301)
     })
     app.get('/server-error', (c) => {
-      return c.text('', 511)
+      const res = new Response('', { status: 511 })
+      if (c.req.query('status')) {
+        // test status code not yet supported by runtime `Response` object
+        Object.defineProperty(res, 'status', { value: parseInt(c.req.query('status') as string) })
+      }
+      return res
     })
   })
 
@@ -95,6 +100,22 @@ describe('Logger by Middleware', () => {
     expect(log.startsWith('--> GET /server-error \x1b[31m511\x1b[0m')).toBe(true)
     expect(log).toMatch(/m?s$/)
   })
+
+  it('Log status 100', async () => {
+    const res = await app.request('http://localhost/server-error?status=100')
+    expect(res).not.toBeNull()
+    expect(res.status).toBe(100)
+    expect(log.startsWith('--> GET /server-error 100')).toBe(true)
+    expect(log).toMatch(/m?s$/)
+  })
+
+  it('Log status 700', async () => {
+    const res = await app.request('http://localhost/server-error?status=700')
+    expect(res).not.toBeNull()
+    expect(res.status).toBe(700)
+    expect(log.startsWith('--> GET /server-error 700')).toBe(true)
+    expect(log).toMatch(/m?s$/)
+  })
 })
 
 describe('Logger by Middleware in NO_COLOR', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's hacky but nice.
Done in 66f35bf

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

}

type PrintFunc = (str: string, ...rest: string[]) => void
Expand Down
Loading