fix(cloudflare): Fix 404.astro not being used when routes.strategy is set to exclude

[ToxiWoxi]

Changes

Turns out it was an adapter issue after all! Fixes #67

Currently, missing routes in the Cloudflare adapter are responded to with blank 404 responses:

return new Response(null, {
	status: 404,
	statusText: 'Not found',
});

Astro has its own error handler! We can use that by returning undefined for our routeData in our render function. This pr does that.

Testing

By using pnpm link in the latest branch of my 404 issue repo and going to http://localhost:8788/thispagedoesnotexist in my browser, I can confirm that these changes result in the #renderError method being invoked which allows 404 pages being rendered properly whether they're prerendered or server-rendered.

I've also set up 3 previews:
One that uses a server-rendered 404 page, another that uses a prerendered 404 page, and a third that shows the current behavior.

Docs

I don't recommend any changes to the docs.
This PR should not change the behavior of any users as it returns 404 behavior back to normal, matching other adapters.

Issues

The current #renderError method in the main repo fetches the 404 page when the 404 page is prerendered. This is seen in a warning in Wrangler. It does not effect website visitors, but it does result in 2 subrequests per page load, which you can see in the Functions metrics on the Cloudflare dashboard. To avoid this, you can use the "include" routing strategy or use SSR on the 404 page.
I may open an issue for this in the main Astro repo later.

[changeset-bot]

🦋 Changeset detected

Latest commit: 9b80e90

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages

Name	Type
@astrojs/cloudflare	Patch
@test/astro-cloudflare-dev-runtime	Patch
@test/astro-cloudflare-directory-mode	Patch
@test/astro-cloudflare-external-image-service	Patch
@test/astro-cloudflare-function-per-route	Patch
@test/astro-cloudflare-hybrid	Patch
@test/astro-cloudflare-no-output	Patch
@test/astro-cloudflare-prerender	Patch
@test/astro-cloudflare-routes-json	Patch
@test/astro-cloudflare-wasm-function-per-route	Patch
@test/astro-cloudflare-wasm-directory	Patch
@test/astro-cloudflare-wasm	Patch
@test/astro-cloudflare-with-solid-js	Patch
@test/astro-cloudflare-wrangler-runtime	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[lilnasy]

Adding some indentation back so that the diff is easier to review.

[lilnasy]

Ah! So the adapter is the one returning the empty 404 responses. It is confusing because that's what astro does when there isn't a 404.astro as well.

[lilnasy]

LGTM, thanks for looking into it!

A couple of suggestion on the changeset but not blocking.

[ToxiWoxi]

Ah! So the adapter is the one returning the empty 404 responses. It is confusing because that's what astro does when there isn't a 404.astro as well.

Yeah! I was playing around with Astro's render function in the main repo and noticed that nothing changed when I modified the renderError method. Sure enough, it never got called.

[lilnasy]

That's incredible! The monorepo and the codebase can be hard to work with but you got to the bottom of it.

[alexanderniebuhr]

@ToxiWoxi thanks for the PR! 🚀
I think I understood the change, but I'd like to test it a little bit myself (therefore going to block this), because I don't think this should be the right approach. But let me get back once I know more.

[alexanderniebuhr]

Generally this can be a good change, because if Astro.render() handles cases where routeData is undefined we don't need to have the custom logic.

However, I still want to wait for information about the fact that routeData started to return undefined instead of matching the 404, which in fact seems to be a regression, because the option matchNotFound should match it.

[ToxiWoxi]

In the 404/500 refactor in withastro/astro#7754, matchNotFound was removed (though its definition through an interface was not removed).
They moved 404 handling from app.match to app.render.
To use the new error handler, routeData must be undefined.
This is done on line 150 of astro/packages/astro/src/core/app/index.ts (which you can look at here)

[alexanderniebuhr]

Interesting! Thanks for pointing this out. Wondering why other adapters where updated but not the Cloudflare one during that refactor.
I personally still think that is not the best call, but if it was changed in Astro, we do have to change it here too. I'll propose another changeset, because I think we need to make it more clear. I'll also let Docs check it, so we are safe.

@ToxiWoxi thanks for the contribution, and my apology for asking many questions, it is important to get down to the root and find out why the behavior changed without making sure the Cloudflare adapter stays compatible. I'll make sure this get's merged soon.

[ToxiWoxi]

my apology for asking many questions

Completely understand! There's a lot of jumping around you have to do to figure it all out and the diffs that caused the changes are fairly large. Thanks for everything!

[alexanderniebuhr]

We've discussed this on Discord and we'll go ahead with this solution.
Just giving docs some time to check the changeset, I'll merge it tomorrow anyways.

[sarah11918]

Really nice changeset @alexanderniebuhr ! 🥳