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.

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(cloudflare-pages): Add Cloudflare Pages middleware handler #3028

Conversation

BarryThePenguin
Copy link
Contributor

Adds an adapter for Cloudflare Pages middleware

This provides a convenient way to use Context and helpers from hono within Cloudflare Pages middleware

Fixes #3015

TOKEN: 'HONOISCOOL',
}
const handler = handleMiddleware(async (c, next) => {
const cookie = getCookie(c, 'my_cookie')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included this as an example of my use-case, but it's not entirely necessary as part of the test

src/adapter/cloudflare-pages/handler.ts Outdated Show resolved Hide resolved
@yusukebe
Copy link
Member

@BarryThePenguin

Thank you for the PR. I'll review this later.

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.05%. Comparing base (afa44e9) to head (7bd318f).
Report is 6 commits behind head on next.

Files Patch % Lines
src/adapter/cloudflare-pages/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3028      +/-   ##
==========================================
+ Coverage   96.02%   96.05%   +0.02%     
==========================================
  Files         142      142              
  Lines       14271    14358      +87     
  Branches     2445     2580     +135     
==========================================
+ Hits        13704    13791      +87     
  Misses        567      567              

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

@yusukebe yusukebe changed the title Add Cloudflare Pages middleware handler feat(cloudflare-pages): Add Cloudflare Pages middleware handler Jun 27, 2024
@yusukebe yusukebe changed the base branch from main to next June 27, 2024 01:46
@yusukebe yusukebe added the v4.5 label Jun 27, 2024
@yusukebe
Copy link
Member

The current code can't handle an HTTPException thrown by a middleware like Basic Auth Middleware. The code below throws a 500 internal server error instead of a 401 authorization error.

import { basicAuth } from 'hono/basic-auth'

export const onRequest = handleMiddleware(
  basicAuth({
    username: 'foo',
    password: 'bar'
  })
)

To handle HTTPException correctly, you can write like the following:

diff --git a/src/adapter/cloudflare-pages/handler.ts b/src/adapter/cloudflare-pages/handler.ts
index 8d3325a..13e6916 100644
--- a/src/adapter/cloudflare-pages/handler.ts
+++ b/src/adapter/cloudflare-pages/handler.ts
@@ -1,5 +1,6 @@
 import { Context } from '../../context'
 import type { Hono } from '../../hono'
+import { HTTPException } from '../../http-exception'
 import { HonoRequest } from '../../request'
 import type { Env, Input, MiddlewareHandler } from '../../types'

@@ -53,9 +54,17 @@ export function handleMiddleware<E extends Env = any, P extends string = any, I
       executionCtx,
     })

-    const response = await middleware(context, async () => {
-      context.res = await executionCtx.next()
-    })
+    let response: Response | void = undefined
+
+    try {
+      response = await middleware(context, async () => {
+        context.res = await executionCtx.next()
+      })
+    } catch (e) {
+      if (e instanceof HTTPException) {
+        response = e.getResponse()
+      }
+    }

     return response ?? context.res
   }

@yusukebe
Copy link
Member

Hi @BarryThePenguin

This is a super cool feature! I've tried it, and it feels good. I've left some comments. Check them!

@BarryThePenguin BarryThePenguin force-pushed the feature/cloudflare-pages-middleware-handler branch 2 times, most recently from 0acd56c to cb19747 Compare June 27, 2024 10:19
Copy link
Contributor Author

@BarryThePenguin BarryThePenguin left a comment

Choose a reason for hiding this comment

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

I had a bit of trouble reconciling the changes in both the main and next branches, so I have rebased my changes onto next, assuming #3046 will also be in next soon 👀

src/adapter/cloudflare-pages/handler.ts Outdated Show resolved Hide resolved
@yusukebe
Copy link
Member

I had a bit of trouble reconciling the changes in both the main and next branches, so I have rebased my changes onto next, assuming #3046 will also be in next soon 👀

Ahh, sorry! I've merged the changes of main to next, which includes #3046.

@BarryThePenguin BarryThePenguin force-pushed the feature/cloudflare-pages-middleware-handler branch from cb19747 to 7bd318f Compare June 28, 2024 00:58
Comment on lines +58 to +91
try {
response = await middleware(context, async () => {
try {
context.res = await executionCtx.next()
} catch (error) {
if (error instanceof Error) {
context.error = error
} else {
throw error
}
}
})
} catch (error) {
if (error instanceof Error) {
context.error = error
} else {
throw error
}
}

if (response) {
return response
}

if (context.error instanceof HTTPException) {
return context.error.getResponse()
}

if (context.error) {
throw context.error
}

return context.res
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still not sure all of this is necessary.. I added all the test cases I could think of, so hopefully all the main use cases are covered

@@ -49,7 +49,7 @@ describe('getConnInfo', () => {
})
it('should return undefined when addressType is invalid string', () => {
const { server } = createRandomBunServer({ family: 'invalid' })
const c = new Context(new HonoRequest(new Request('http://localhost/')), { env: { server } })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like next is failing due to HonoRequest being removed 👀

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

@BarryThePenguin

Awesome work! It's wonderful that we'll be able to use a lot of Hono's middleware and helpers in the Cloudflare Pages function. Thank you very much. I'll merge this into the next and release an RC version now.

@yusukebe yusukebe merged commit 204e10b into honojs:next Jun 29, 2024
14 checks passed
@BarryThePenguin BarryThePenguin deleted the feature/cloudflare-pages-middleware-handler branch June 29, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloudflare Pages Middleware
2 participants