honojs / hono

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

`notFound` handling change from v3 -> v4 migration #2901

Closed Cherry closed 3 months ago

Cherry commented 3 months ago

What version of Hono are you using?

4.4.3

What runtime/platform is your app running on?

Cloudflare Workers

What steps can reproduce the bug?

Using the following code:

import { Hono } from 'hono'

const app = new Hono();

app.get('/foo', async (context) => {
    if (Math.random() <= 0.5) {
        return new Response('success');
    }

    // shouldn't this raise a typescript error?
    return;
});

app.notFound((context) => {
    return new Response('404z', {
        status: 400
    });
})

export default app;

And then npx wrangler dev:

Using hono 3, observe a mixture of success and 404z response Using hono 4, observe a mixture of success, and Internal Server Error responses, with this logged:

Context is not finalized. You may forget returning Response object or `await next()`

What is the expected behavior?

Is this the intended behaviour? I feel like this does make more sense in version 4, and feels more semantically correct, but I didn't see it called out in the migration guide - let me know if I missed it.

Additional information

I would honestly expect the following function to throw a TypeScript issue:

app.get('/foo', async (context) => {
    if (Math.random() <= 0.5) {
        return new Response('success');
    }

    // shouldn't this raise a typescript error?
    return;
});

If I remove the async declaration from the function, so it's just:

app.get('/foo', (context) => {
    if (Math.random() <= 0.5) {
        return new Response('success');
    }

    return;
});

I do then receive a error:

No overload matches this call.
  The last overload gave the following error.
    Argument of type '(context: Context<BlankEnv, "/foo", BlankInput>) => Response | undefined' is not assignable to parameter of type 'H<BlankEnv, "/foo", BlankInput, HandlerResponse<any>>'.
      Type '(context: Context<BlankEnv, "/foo", BlankInput>) => Response | undefined' is not assignable to type 'MiddlewareHandler<BlankEnv, "/foo", BlankInput>'.
        Type 'Response | undefined' is not assignable to type 'Promise<void | Response>'.
          Type 'undefined' is not assignable to type 'Promise<void | Response>'.
yusukebe commented 3 months ago

Hi @Cherry !

Using hono 3, observe a mixture of success and 404z response Using hono 4, observe a mixture of success, and Internal Server Error responses, with this logged:

I think the behavior is the same with both versions 3 and 4. It will be a "mixture of success and 404z response". Can you try it again and let me know the result?

I would honestly expect the following function to throw a TypeScript issue:

Ahh, yeah. I also think it should be a TypeSript error, whether it is an async handler or not. But it is very difficult or maybe impossible. Either way, I'll try to work on it. Thanks!

Cherry commented 3 months ago

Sorry, I missed a critical piece in my reproduction above.

In my tests, I'm only ever hitting /foo.

In version 3, I see a mixture of success and the 404 on /foo.

Whereas in version 4, I see a mixture of success and then the previously mentioned error on /foo. Very different behaviour.

Thanks for the reply!

yusukebe commented 3 months ago

@Cherry It's my pressure!

I would honestly expect the following function to throw a TypeScript issue

I've tried to make it your expected behavior, but I can't do it immediately. It maybe impossible. For now, I'll close this issue. Thanks.

Cherry commented 3 months ago

Thanks @yusukebe. Is the behaviour difference between v3 and v4 expected when you return nothing, and have a notFound handler?

If so, it would be good to call this out in the migration guide for v3 -> v4.

yusukebe commented 3 months ago

Thanks @yusukebe. Is the behaviour difference between v3 and v4 expected when you return nothing, and have a notFound handler?

Oh? I expect the behavior between v3 and v4 to be the same. Do you think there is a difference? Or are you saying that TypeScript Type matters?

Cherry commented 3 months ago

There is a difference, yes.

In v3, it seems that returning nothing in a handler triggers on the notFound handler. But in v4, it does not.


The TypeScript type would have been a way to catch this in my migration, but the behaviour changed in v3 -> v4.

yusukebe commented 3 months ago

@Cherry

Thank you for your explanation. But one more. Sorry to ask you something that I doubt you. Did you run the following code and get an error?

import { Hono } from 'hono'

const app = new Hono()

app.get('/foo', async (context) => {
  return
})

app.notFound((context) => {
  return new Response('404z', {
    status: 400
  })
})

export default app

Because the following error message only occurs when using middleware, the above code may not cause an error.

Context is not finalized. You may forget returning Response object or `await next()`
Cherry commented 3 months ago

Hmm, I'm not able to reproduce this anymore like I described in my initial issue, but I was able to do so reliably yesterday... very odd.

I will let you know and create a new issue if I find a way to reliably reproduce this again.

yusukebe commented 3 months ago

I will let you know and create a new issue if I find a way to reliably reproduce this again.

Please! Thanks!