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

Inconsistent 404 handling in onRequest middleware for SSR mode with nodejs integration #8273

Closed
1 task
artmsv opened this issue Aug 29, 2023 · 3 comments
Closed
1 task
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: error pages Related to 404 and 500 handling (scope) feat: middleware Related to middleware (scope) pkg: node Related to Node adapter (scope)

Comments

@artmsv
Copy link

artmsv commented Aug 29, 2023

Astro info

❯ astro info

Astro version            v2.10.14
Package manager          npm
Platform                 linux
Architecture             x64
Adapter                  @astrojs/node
Integrations             None or couldn't determine.

What browser are you using?

Chrome

Describe the Bug

I'm using Astro in SSR mode with nodejs integration.
I have middleware with onRequest handler where I'm trying to get html markup from the response:
const html = await response.text();.
If a page does exist, it will return HTML markup, but if it does not exist, this middleware will not be executed, and 404 page will be shown. If somepage.astro returns new Response(null, {status: 404}), the middleware will get executed, but the response.text() will return an empty string.

Moreover, if I reconstruct the response in the middleware in this way:

const response = await next();
const html = await response.text();
return new Response(html, response);

200 pages will be displayed correctly, but the somepage.astro will return 404 page with text/plain to the browser.

In the provided stackblitz you can try to visit:

  • /awd - this page does not exists and you will get 404
  • /somepage - returns text/plain

What's the expected result?

  • The middleware should get executed if a page is not found
  • the response from next() should return the 404.astro content with a header Content-Type: text/html.

Also, I got into my thought, what if I want to return 404 response from middleware before it gets to next(); for some "unknown" reason? It contradicts the behavior I described above.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-oiqdwp?file=astro.config.mjs,src%2Fpages%2F404.astro,src%2Fpages%2Fnotfound.astro,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 29, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Aug 29, 2023

Is there a difference between dev and build?

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

artmsv commented Aug 29, 2023

Is there a difference between dev and build?

Looks like there is a difference. In dev, for both pages /awd and /somepage middleware gets executed and it does return html markup and 404 page displays correctly when reconstructed.

@lilnasy lilnasy added - P3: minor bug An edge case that only affects very specific usage (priority) pkg: node Related to Node adapter (scope) feat: middleware Related to middleware (scope) feat: error pages Related to 404 and 500 handling (scope) and removed needs response Issue needs response from OP labels Aug 29, 2023
@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!

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: error pages Related to 404 and 500 handling (scope) feat: middleware Related to middleware (scope) pkg: node Related to Node adapter (scope)
Projects
None yet
Development

No branches or pull requests

3 participants