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

Returning Response(null, { status: 404 }) from middleware makes astro dev hang #8257

Closed
1 task
itsmatteomanf opened this issue Aug 28, 2023 · 10 comments
Closed
1 task
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: dev Related to `astro dev` CLI command (scope) feat: error pages Related to 404 and 500 handling (scope)

Comments

@itsmatteomanf
Copy link
Contributor

itsmatteomanf commented Aug 28, 2023

Astro info

Astro version            v2.10.14
Package manager          pnpm
Platform                 darwin
Architecture             arm64
Adapter                  @astrojs/cloudflare
Integrations             @astrojs/tailwind

What browser are you using?

Safari, Chrome

Describe the Bug

If I try and return new Response(null, { status: 404 }) from the middleware (works in a page), the process astro dev hangs indefinitely, forcing a manual restart.

What's the expected result?

Returning the 404.astro page.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/withastro-astro-issue-8257?file=src%2Fmiddleware.js

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Aug 28, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Aug 29, 2023

Couldn't reproduce, the dev server continues to serve requests and handle module updates as expected.

Could you check again?

@lilnasy lilnasy added needs response Issue needs response from OP and removed needs triage Issue needs to be triaged labels Aug 29, 2023
@itsmatteomanf
Copy link
Contributor Author

Stackblitz's example fails, too... Other than telling you that's the case, I have no idea how to test again, given it behaves like that to me (and even in the minimal example above). The page shows blank, while it should return the 404 page given what the middleware tells it to return.

@ematipico
Copy link
Member

Is it possible that the middleware doesn't run?

@itsmatteomanf
Copy link
Contributor Author

itsmatteomanf commented Aug 29, 2023

Is it possible that the middleware doesn't run?

I doubt that, given that if I put anything else in there it works just as well. Even returning new Response("hello", { status: 200 }) works. As soon as the status is set to 404 it hangs and never returns anything.

As I said above, putting the same in a page, not the middleware, works as expected when @natemoo-re suggested this solution on Discord.

@itsmatteomanf
Copy link
Contributor Author

Adding that I am expecting to return the 404 page on a page that actually exists, so middleware should definitely run.
I'm pretty sure middleware runs even on routes that return 404, though.

@lilnasy
Copy link
Contributor

lilnasy commented Aug 29, 2023

404.astro wasn't there when I tested yesterday. I can confirm the issue now.

@lilnasy lilnasy added - P3: minor bug An edge case that only affects very specific usage (priority) feat: dev Related to `astro dev` CLI command (scope) feat: error pages Related to 404 and 500 handling (scope) and removed needs response Issue needs response from OP labels Aug 29, 2023
@itsmatteomanf
Copy link
Contributor Author

itsmatteomanf commented Aug 29, 2023

Oh, I'm sure I added it... it might not have saved it and it did when I switched to that tab again after you posted.

I think it's not just in dev, it's in prod, too, @lilnasy.

@lilnasy
Copy link
Contributor

lilnasy commented Aug 29, 2023

Just tested by adding the node adapter to the repro, it works fine.

@ematipico
Copy link
Member

I am going ahead and close the issue. This is a feature, and it requires some internal changes, which should also be reflected in the respective adapters.

One of our maintainers - @lilnasy - created a proposal that would solve this issue: withastro/roadmap#681

If you need it, please upvote it!

@itsmatteomanf
Copy link
Contributor Author

This is a feature

I'm not sure it is, it's a bug for me, if that's the way it's recommended to do 404 from middleware now.
I worked around it, plus it's not a massive issue, but it's still a problem.

If you need it, please upvote it!

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: dev Related to `astro dev` CLI command (scope) feat: error pages Related to 404 and 500 handling (scope)
Projects
None yet
Development

No branches or pull requests

3 participants