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

Server-rendered 404 pages no longer work on Cloudflare #67

Closed
1 task done
ToxiWoxi opened this issue Aug 15, 2023 · 27 comments · Fixed by #66
Closed
1 task done

Server-rendered 404 pages no longer work on Cloudflare #67

ToxiWoxi opened this issue Aug 15, 2023 · 27 comments · Fixed by #66
Labels
- P2: has workaround Bug, but has workaround (priority) pkg: cloudflare

Comments

@ToxiWoxi
Copy link
Contributor

ToxiWoxi commented Aug 15, 2023

What version of astro are you using?

2.9.7 to 3.3.2

Are you using an SSR adapter? If so, which one?

Cloudflare

What package manager are you using?

npm

What operating system are you using?

Linux & Deployment

What browser are you using?

Chrome

Describe the Bug

Issue: Changes to 404 handling have resulted in server-rendered 404 pages no longer working on Cloudflare.

Behaviors in different versions

Expected behavior:

  1. The 404 page loads when going to a non-existent route
  2. The 404 page can perform redirects to case-corrected versions of pages or aliases will redirect to correct page
    • Examples:
      • /eXaMPLe can be redirected to /example
      • /index can be redirected to /
  • These behaviours are tested to work in Astro v2.9.6 with Cloudflare adapter v6.6.2

Updated behavior with Astro v2.9.7 and higher

  1. Cloudflare sends a default 404 error with no page when accessing a non-existent route
  2. The 404 page still can be accessed from /404
  3. Case-corrections and aliases will not be redirected
  • These behaviours are tested to work in Astro v2.9.7 and higher with Cloudflare adapter v6.6.2

Updated behavior with Cloudflare adapter v6.7.0 and higher

  1. Cloudflare returns with the contents of the index route without redirecting or updating the url when accessing a non-existent route
  2. The 404 page still can be accessed from /404
  3. Case-corrections and aliases still will not be redirected
  • These behaviours are tested to work in Astro v2.10.5 and higher with Cloudflare adapter v6.7.0 and higher and is the current behavior on latest versions of both.

This issue does not happen on latest when prerendering the 404 page

However, this doesn't allow you to use the 404 page to do case-corrections and handle aliases

  • The latest-prerendered deployment in my provided example dedicated to this behavior

Deployments and their issues

Deployment Bad Routes Aliases Case-correction
astro@2.9.6 Returns 404.astro Working Working
astro@2.9.7 Returns nothing Configured redirects only Not working
astro@2.10.5-cloudflare@6.6.2 Returns nothing Configured redirects only Not working
astro@2.10.5-cloudflare@6.7.0 Returns index.astro Configured redirects only Not working
latest Returns index.astro Configured redirects only Not working
latest-prerendered Renders 404.astro Configured redirects only Not working

What's the expected result?

  1. The 404 page loads when going to a non-existent route
  2. The 404 page can perform redirects to case-corrected versions of pages or aliases will redirect to correct page
    • Examples:
      • /eXaMPLe can be redirected to /example
      • /index can be redirected to /
  • These behaviours are tested to work in Astro v2.9.6 with Cloudflare adapter v6.6.2

Link to Minimal Reproducible Example

https://github.com/ToxiWoxi/astro-404-issue

Participation

  • I am willing to submit a pull request for this issue.
@natemoo-re
Copy link
Member

Thanks for the thorough details and sorry for the frustration! We're tracking down quite a few 404 related issues (withastro/astro#8070) and hoping to fix them soon. The original changes probably shouldn't have gone out in a patch release.

@xriter
Copy link

xriter commented Aug 15, 2023

This might be the same / related to issue withastro/astro#8054

@lilnasy
Copy link
Contributor

lilnasy commented Aug 15, 2023

The issues described here are trickier, unfortunately.

@smastrom
Copy link

I'm having the same issue, I'll add more context maybe it can help. In my case, I'm using output: server to deploy a bilingual website to Cloudflare Pages. I have a 404.astro and index.astro which share the same logic:

---
const lang = Astro.request.headers.get('Accept-Language')

if (lang.startsWith('it')) return Astro.redirect('/it')

return Astro.redirect('/en')
---

On local dev, both work as expected. Once deployed, If try to access a non-existent page the cloudflare server returns a 500 errror. The index page logic works as expected instead.

@lilnasy lilnasy transferred this issue from withastro/astro Oct 27, 2023
@alexanderniebuhr
Copy link
Member

Can we confirm that this is still an issue. I do have projects deployed with 404.astro pages, and it works.

@alexanderniebuhr
Copy link
Member

Closing for now, because the Issue seems to be stale. Happy to reopen, if we get new information or confirmation.

@alexanderniebuhr alexanderniebuhr closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2023
@ToxiWoxi
Copy link
Contributor Author

ToxiWoxi commented Nov 14, 2023

Sorry, the last replies were sent to my spam.
This is still an issue. I've updated the "latest" branch in my repo to use Astro 3.5.3 and Cloudflare adapter 7.7.0 (latest versions at current time), which you can preview here and look at the code here.
Once again, this is the expected behavior, last working with Astro 2.9.6 and Cloudflare adapter version 6.6.2.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 14, 2023

Thanks for your effort, ToxiWoxi!
I'll try to take a look this week.

@lilnasy lilnasy reopened this Nov 14, 2023
@alexanderniebuhr
Copy link
Member

Ok just to clear some things about this issue. We need to different where the fallback routing for a 404 page happens.

  • Cloudflare Pages does only fallback to a 404.html file. That is the reason why it does work for a pre-rendered 404 page, the dist folder will have a 404.html file, therefore Cloudflare does handle the fallback routing for any route which is not found.
  • If the 404 page should be rendered using SSR, it would be up to Astro or the adapter to make sure the fallback works, because Cloudflare would not handle fallback routing in that case.
  • We need to find out if a change in Astro causes the redirect to a 404 if the page does not exist do not trigger, or if the adapter code changed. (I currently expect it to be a Astro issue, not an adapter one. But I'll quickly check the Adapter changes)

@alexanderniebuhr
Copy link
Member

@lilnasy this was the PR which moved the logic from the adapter to Astro. Not sure if that matches the time range, aligning with the versions in this report. https://github.com/withastro/astro/pull/4018/files

@ToxiWoxi
Copy link
Contributor Author

404/500 logic was refactored in this pr. This aligns with when the behavior was changed.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 14, 2023

You are right these two PRs are where 404/500 behavior changed.

If the 404 page should be rendered using SSR, it would be up to Astro or the adapter to make sure the fallback works, because Cloudflare would not handle fallback routing in that case.

Yep, and I'm pretty sure it is astro in this case. It would be happening somewhere around these parts.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 14, 2023

The returning index.astro in place of 404.astro bug (if it still exists) must have come from a routes.json refactor.

@alexanderniebuhr

This comment was marked as outdated.

@alexanderniebuhr
Copy link
Member

@lilnasy yeah that is correct. I was able to debug the change in _routes.json, the behavior is "expected", as how the _routes.json is generated. I just need to think about a good way to handle this, to get back to the old behavior of returning "no page".

@ToxiWoxi
Copy link
Contributor Author

@alexanderniebuhr after the changes in 2.9.7 (the prs mentioned above), SSR 404 pages stopped returning anything. The adapter was not updated until after Astro 2.10.5. You can use the astro@2.10.5-cloudflare@6.6.2 and astro@2.10.5-cloudflare@6.7.0 branches to compare the changes with the adapter.

@alexanderniebuhr
Copy link
Member

So the change is that the new _routes.json generates the following (by default, if the index page is pre-rendered):

{
  "version": 1,
  "include": [
    "/404",
    "/_image"
  ],
  "exclude": []
}

That means that for /unknown there is no html file for Cloudflare to return nor will it use SSR, cause the route is not in the _routes.json to execute the function.

There are currently two workarounds to this:
a) You can use SSR for the index page
b) you can set the following config in astro.config.mjs

    adapter: cloudflare({
        routes: {
            strategy: "exclude"
        }
    }),

Both of those workarounds should give you the "known" behavior back of getting a 404 with no content. That issue however is probably an Astro topic.

@lilnasy do you think we should transfer this issue back to the main repo, and create another issue for the _routes.json issue, which I can tackle here?

@lilnasy
Copy link
Contributor

lilnasy commented Nov 14, 2023

Yeah, that makes sense.

@alexanderniebuhr alexanderniebuhr transferred this issue from withastro/adapters Nov 14, 2023
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Nov 14, 2023
@alexanderniebuhr
Copy link
Member

This is only relevant for the adapter, and I commented it on that issue, just cross-posting here for visibility: https://discord.com/channels/595317990191398933/1174021841023664288

@ToxiWoxi
Copy link
Contributor Author

It turns out that this was in fact an adapter issue. If this could be moved back, that'd be great. Otherwise, there is a pending pull request in the adapters repo that will close it if merged.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 15, 2023

My understanding is that there two issues. The PR in the adapters repo fixes index.astro being returned in place of 404.astro.
image

Can you confirm that the other issues in the table no longer exist?

@ToxiWoxi
Copy link
Contributor Author

I guess not, technically.
My understanding is that the two issues are as follows:

  1. In the "include" routing strategy, server rendered 404 pages route to index.
  2. In the "exclude" routing strategy, all 404 pages route to an empty 404.

The pr solves issue 2. My understanding is that issue 1 appears to be caused by #64, so I think this issue is now technically about issue 2. I can edit the original comment on this issue and make changes to the test repo I have to reflect this later if we agree on that so there is no further confusion. (I will be out of the house for the next 2 hours)

@lilnasy
Copy link
Contributor

lilnasy commented Nov 15, 2023

After looking at the PR, I see what you mean. The adapter did not give astro's SSR functions the opportunity to render 404.astro. Great work! This issue can be transferred back.

@lilnasy lilnasy transferred this issue from withastro/astro Nov 15, 2023
@alexanderniebuhr
Copy link
Member

I don't think this is true, and only one of the issues can be fixed in the adapter.
However we can keep it in this repo until we know better.

@ToxiWoxi
Copy link
Contributor Author

ToxiWoxi commented Nov 16, 2023

only one of the issues can be fixed in the adapter.

Both issues are technically adapter, though you are correct that only one can be fixed.

The issues with the exclude routing strategy are an adapter issue. #66 solves it.

The issues with the include routing strategy seems to be that Cloudflare treats apps without 404.html as SPAs and redirects 404s to index.html. Since all non-defined routes get sent through Cloudflare's static method under this strategy, I don't think we are able to fix this behavior.

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Nov 16, 2023

The issues with the exclude routing strategy are an adapter issue. #66 solves it.

I'm still not convinced, I know that the adapter code returns an empty 404 response, however routeData should not be undefined. I'll have to double check if that was always the case, but the adapter always had the empty 404 response and it was not an issue before.

The issues with the include routing strategy seems to be that Cloudflare treats apps without 404.html as SPAs and redirects 404s to index.html. Since all non-defined routes get sent through Cloudflare's static method under this strategy, I don't think we are able to fix this behavior.

We are, we need to change the way we generate _routes.json file.

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Nov 16, 2023

I still think this is a Astro issue. Unless the following change in behavior is known and expected, but I don't think so nor do I have confirmation:

astro@2.9.6 returns a route object for a page which is not found

routeData Object {
  route: /404,
  type: page,
  pattern: /^\/404\/?$/,
  params: Array(0),
  component: src/pages/404.astro
  ...
}

astro@>2.9.6 does not return it anymore

routeData undefined

@alexanderniebuhr alexanderniebuhr added pkg: cloudflare - P2: has workaround Bug, but has workaround (priority) and removed needs triage Issue needs to be triaged labels Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: has workaround Bug, but has workaround (priority) pkg: cloudflare
Projects
None yet
6 participants