honojs / hono

Web framework built on Web Standards
https://hono.dev
MIT License
18.52k stars 522 forks source link

[Service Worker] "Failed to execute 'fetch' on 'WorkerGlobalScope': Illegal invocation" #3176

Closed yusukebe closed 1 month ago

yusukebe commented 1 month ago

What version of Hono are you using?

4.5.1

What runtime/platform is your app running on?

Service Worker

What steps can reproduce the bug?

  1. Clone the repo: https://github.com/yusukebe/service-worker-adapter-demo/
  2. Run dev server: bun run dev
  3. Access /
  4. Access /sw/not-found

What is the expected behavior?

It will fall back to index.html when accessing /sw/not-found.

What do you see instead?

The page is not loaded and it throws an error:

Failed to execute 'fetch' on 'WorkerGlobalScope': Illegal invocation`

https://github.com/user-attachments/assets/c369e8c4-77e9-45ff-b50d-c427c49bc6a4

Additional information

The following patch can fix it:

diff --git a/src/adapter/service-worker/handler.ts b/src/adapter/service-worker/handler.ts
index edc9efad..0b34c050 100644
--- a/src/adapter/service-worker/handler.ts
+++ b/src/adapter/service-worker/handler.ts
@@ -13,18 +13,17 @@ type Handler = (evt: FetchEvent) => void
  */
 export const handle = (
   app: Hono,
-  opts: {
+  opts?: {
     fetch?: typeof fetch
-  } = {
-    fetch: fetch,
   }
 ): Handler => {
   return (evt) => {
     evt.respondWith(
       (async () => {
         const res = await app.fetch(evt.request)
-        if (opts.fetch && res.status === 404) {
-          return await opts.fetch(evt.request)
+        if (opts?.fetch && res.status === 404) {
+          const fallbackFetch = opts.fetch ?? fetch
+          return await fallbackFetch(evt.request)
         }
         return res
       })()

But I don't have a good idea to test it.

yusukebe commented 1 month ago

Hey @nakasyou

Can you take a look when you are free?

nakasyou commented 1 month ago

Hi @yusukebe

I can't find any idea to test it.

And I have a more concise change idea (But I haven't test it):

 export const handle = (
   app: Hono,
   opts?: {
     fetch?: typeof fetch
   } = {
-    fetch: fetch,
+   fetch: self.fetch.bind(self)
   }

I think it is more readable.

yusukebe commented 1 month ago

Hi @nakasyou

Thank you for your idea. Seems good, but referring self is not good. Actually, the test will fail:

 FAIL  src/adapter/service-worker/handler.test.ts > handle > Success to fetch
ReferenceError: self is not defined
 ❯ Module.handle src/adapter/service-worker/handler.ts:19:12
     17|     fetch?: typeof fetch
     18|   } = {
     19|     fetch: self.fetch.bind(self),
       |            ^
     20|   }
     21| ): Handler => {
 ❯ src/adapter/service-worker/handler.test.ts:11:21

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed | 101 passed (102)
      Tests  1 failed | 2185 passed | 12 skipped (2198)
   Start at  18:37:29
   Duration  8.61s (transform 2.61s, setup 1.11s, collect 12.84s, tests 14.58s, environment 19ms, prepare 6.66s)

error: script "test" exited with code 1

I think we can go with my snippet and not add a test at this time.