withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
43.84k stars 2.28k forks source link

Rewriting doesn't use HTTP status code from rewritten response #11306

Open vchirikov opened 1 week ago

vchirikov commented 1 week ago

Astro Info

4.11.0

Describe the Bug

If we rewrite from non-existed page we always get 404 status even if middleware rewrite to the valid page.

Looks like the response object from rewriting isn't used. This is the repro:

bug

we should get http status 200 (because we rewrite to the /about page), but we see 404.

p.s. The issue was originally described here, this is separate issue for the bug with repro. p.s. looks similar to the cookie issue (fixed by @ascorbic )

What's the expected result?

rewriting should use status code from rewritten page pipeline.

Link to Minimal Reproducible Example

https://github.com/vchirikov/astro_issue_02

ematipico commented 1 week ago

@vchirikov do you experience the same issue with output: "server" (SSR)?

vchirikov commented 1 week ago

Yes, btw the repro uses output: "server":

https://github.com/vchirikov/astro_issue_02/blob/f7c9532f3e31798f6acbbae748c076fe9fe185d9/astro.config.mjs#L7

vchirikov commented 1 week ago

To be clear, the issue isn't about /404 rewriting, it's about wrong 404 status from rewritten page (/about in the gif), which should return 200, but returned 404 instead...

ematipico commented 1 week ago

To be clear, the issue isn't about /404 rewriting, it's about wrong 404 status from rewritten page (/about in the gif), which should return 200, but returned 404 instead...

That wasn't clear from your "expectation" section. Can you be more clear, please? For example why /about should return 200 instead of 404? It's important to voice and justify expectations.

Also, could you reduce the branching of the middleware in your repro? It's not very clear where's the issue.

vchirikov commented 1 week ago

could you reduce the branching of the middleware in your repro?

done in https://github.com/vchirikov/astro_issue_02/tree/issue-11306

Can you be more clear, please?

As a developer I expect that successfully rewritten page should return status 200 instead of 404:

image

ematipico commented 1 week ago

As a developer I expect that successfully rewritten page should return status 200 instead of 404:

You haven't told me why though, because in this case you're intentionally rewriting with special route 404, which always returns a status code 404. Why should it return a 200? IMHO, it should not.

vchirikov commented 1 week ago

I rewrite all routes to next('/') (see middleware.ts on the screenshot), I don't get your you're intentionally rewriting with special route 404, which always returns a status code 404

I rewrite to an existing page, this page's response.status is 200 (!), I returned the Response object from middleware with status 200 (see the console.log in terminal + blue lines on the screenshot where I show Response with status code 200) but the Astro returns 404 to the browser.

Again rewriting correctly renders index.astro page if I use next('/') but with 404 status code despite the return code from index.astro which is 200 )


upd: oh, I think I get your point, I rewrite un-existed page and this is a special 404 route which always returns 404, but:

  1. it's unexpected behavior, I explicitly return the Response object with desired status code, but framework implicitly changes the response for me
  2. I can do this for another (existed) routes, it's inconsistency
  3. there is no docs about it

Example where it is useful:

I want to rewrite /${locale}/${url} to /${url} (and use /${url} as well, without copy-pasting the same pages to [..locale]) page and extract locale to the Astro.locals, with current behavior I will always get 404 on rewritten pages.

vchirikov commented 4 days ago

@ematipico the issue is not fixed in 4.11.1

image

repro: https://github.com/vchirikov/astro_issue_02/tree/issue-11306

npm run dev
# expected status is 200
curl -I -X GET http://localhost:4321/foo