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: Introduce Service Worker Adapter #3062

Merged
merged 8 commits into from
Jul 11, 2024

Conversation

nakasyou
Copy link
Contributor

I added adapter for Service Worker on browser.

I think that Hono is good for creating Service Worker.

In the PR, you can write code like that:

import { Hono } from 'hono'
import { handle } from 'hono/service-worker'

const app = new Hono()
app.get('/', c => c.text('Hello World'))

self.addEventListener('fetch', handle(app))

When app responses 404, adapter fetches the original server.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

Copy link

codecov bot commented Jun 30, 2024

Codecov Report

Attention: Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.

Project coverage is 96.12%. Comparing base (c095dfa) to head (a0d5039).
Report is 42 commits behind head on next.

Files Patch % Lines
src/adapter/service-worker/index.ts 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3062      +/-   ##
==========================================
+ Coverage   95.87%   96.12%   +0.25%     
==========================================
  Files         137      147      +10     
  Lines       13460    14795    +1335     
  Branches     2295     2602     +307     
==========================================
+ Hits        12905    14222    +1317     
- Misses        555      573      +18     

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

@yusukebe
Copy link
Member

yusukebe commented Jul 1, 2024

Sounds good! I'll try it later.

@yusukebe
Copy link
Member

yusukebe commented Jul 6, 2024

Hi @nakasyou

I've noticed that this adapter is so important! If we have this adapter, we can obsolete the app.fire() method in the next major release. The app.fire() is this:

// src/hono-base.ts
fire = (): void => {
  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
  // @ts-ignore
  addEventListener('fetch', (event: FetchEventLike): void => {
    event.respondWith(this.dispatch(event.request, event, undefined, event.request.method))
  })
}

I think it's okay to delete it completely because this service worker style is now used only for Fastly Compute and Service Worker.

@yusukebe yusukebe added the v4.5 label Jul 6, 2024
@nakasyou
Copy link
Contributor Author

nakasyou commented Jul 6, 2024

@yusukebe It sounds good!
However, the adapter is for browsers. So if app returns 404, adapter fetches the original server. Currently replacing app.fire maybe not good.

@yusukebe
Copy link
Member

yusukebe commented Jul 6, 2024

However, the adapter is for browsers. So if app returns 404, adapter fetches the original server.

Yeah. I'll consider it.

@yusukebe
Copy link
Member

yusukebe commented Jul 8, 2024

Hi @nakasyou !

I've created the demo project to try this adapter: https://github.com/yusukebe/service-worker-adapter-demo

And I'll suggest the code, though I'm not sure this is 100% good:

diff --git a/src/adapter/service-worker/handler.ts b/src/adapter/service-worker/handler.ts
index 2c85343..b8801e3 100644
--- a/src/adapter/service-worker/handler.ts
+++ b/src/adapter/service-worker/handler.ts
@@ -1,12 +1,12 @@
+/// <reference lib="webworker" />
 /**
  * Handler for Service Worker
  * @module
  */

 import type { Hono } from '../../hono'
-import type { FetchEventLike } from '../../types'

-type Handler = (evt: FetchEventLike) => void
+type Handler = (evt: FetchEvent) => void

 /**
  * Adapter for Service Worker
@@ -14,24 +14,16 @@ type Handler = (evt: FetchEventLike) => void
 export const handle = (
   app: Hono,
   opts: {
-    fetch: (req: Request) => Promise<Response>
+    fetch: typeof fetch | undefined
   } = {
     fetch: fetch,
   }
 ): Handler => {
-  return (evt) => {
-    const fetched = app.fetch(evt.request)
-    if (fetched instanceof Response && fetched.status === 404) {
-      return
+  return async (evt) => {
+    let res = await app.fetch(evt.request)
+    if (res.status === 404 && opts.fetch) {
+      res = await opts.fetch(evt.request)
     }
-    evt.respondWith(
-      (async () => {
-        const res = await fetched
-        if (res.status === 404) {
-          return await opts.fetch(evt.request)
-        }
-        return res
-      })()
-    )
+    evt.respondWith(res)
   }
 }

There are two points:

  1. To correct types on the user side, this uses FetchEvent in the WebWorker for evt.
  2. Simplify the logic. I think this is enough. The current code can't handle the 404 in the above demo project well, but the error will be removed with this code.

Plus, with this code, you can pass through the handling 404 by assigning undefined to the fetch option. This means it can support Fastly Compute.

addEventListener(
  'fetch',
  handle(app, {
    fetch: undefined
  })
)

What do you think of them?

@nakasyou
Copy link
Contributor Author

nakasyou commented Jul 8, 2024

Hi @yusukebe, I agree for your comment.
I'll change it.

@yusukebe yusukebe changed the base branch from main to next July 8, 2024 14:30
@nakasyou
Copy link
Contributor Author

nakasyou commented Jul 8, 2024

Hi @yusukebe, I have an issue at TypeScript. It is made by webworker types.
Do you know how to resolve?

@yusukebe
Copy link
Member

yusukebe commented Jul 9, 2024

@nakasyou

How about defining types ourselves instead of referring to lib WebWorker?

interface ExtendableEvent extends Event {
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  waitUntil(f: Promise<any>): void
}

export interface FetchEvent extends ExtendableEvent {
  readonly clientId: string
  readonly handled: Promise<undefined>
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  readonly preloadResponse: Promise<any>
  readonly request: Request
  readonly resultingClientId: string
  respondWith(r: Response | PromiseLike<Response>): void
}

@yusukebe yusukebe changed the title feat: Introduce service worker adapter feat: Introduce Service Worker Adapter Jul 11, 2024
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

@nakasyou

Awesome work! I'll merge this into the next now. Thanks!

@yusukebe yusukebe merged commit c2698fa into honojs:next Jul 11, 2024
14 checks passed
@nakasyou nakasyou deleted the feat/service-worker branch July 11, 2024 11:06
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.

2 participants