adapter-cloudflare: allow disabling generation of fallback `404.html` #9762

ajgeiss0702 · 2023-04-25T05:17:37Z

Adds an option that will allow you to disable the automatic generation of 404.html if, for example, your site is fully dynamic and should not process any pages during build time.

The default option of auto will only generate the fallback page if there are any pre-rendered pages.

#9386 (comment)

closes #9386

This PR also edits the docs to describe the new option.

I have tested this before posting, and the behavior of the option is as intended:

generate404: undefined => generated only if there are any pre-rendered pages
generate404: 'auto' => generated only if there are any pre-rendered pages
generate404: true => generated
generate404: false => not generated

If no pre-rendered page is generated, a simple 404.html is generated in /_app/404.html to prevent a resurgence of #9011 using src/error.html if it exists, otherwise the default error page.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
~~Ideally, include a test that fails without this PR but passes with it.~~ N/A: adapter-cloudflare has no tests

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

changeset-bot · 2023-04-25T05:17:40Z

🦋 Changeset detected

Latest commit: f2413f7

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/adapter-cloudflare	Minor

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

eltigerchino · 2023-04-29T16:17:26Z

I'd go one step further and prevent generating a fallback if there are no prerendered pages in the cloudflare adapter.
The main issue was invalid requests to /_app/** incorrectly returned 200 if you had a /index.html file. Without one, they should correctly return 404 although the page will just be a blank white one.

Conduitry · 2023-05-07T12:26:28Z

Would dealing with #9802 remove the need for this? It would be nice not to need another option.

eltigerchino · 2023-05-07T12:27:39Z

Would dealing with #9802 remove the need for this? It would be nice not to need another option.

I believe it would. The only reason for this option was that the fallback generation was breaking people's builds.

ajgeiss0702 · 2023-05-07T13:31:51Z

Would dealing with #9802 remove the need for this? It would be nice not to need another option.

It would not. Completely removing the fallback page would cause any 404s to non-function routes to show the / page instead of a 404 page

In its current state, the option would only be needed if it breaks your build. It defaults to the same behavior as before.

I probably will get around to implementing @s3812497 ’s suggestion to automatically not generate it if there are no pre-rendered pages

eltigerchino · 2023-05-07T15:12:04Z

The proposed solution actually implements a static 404.html by default too. This would be without running hooks

ajgeiss0702 · 2023-05-07T15:31:09Z

The proposed solution actually implements a static 404.html by default too. This would be without running hooks

It would implement it for the appDir, not the actual app. The fallback page would still be needed for the actual app to ensure that 404s in the app will have the correct behavior.

eltigerchino · 2023-05-07T16:04:22Z

It would implement it for the appDir, not the actual app. The fallback page would still be needed for the actual app to ensure that 404s in the app will have the correct behavior.

If the user has opted-in to use wildcard matching for static requests, perhaps fallback generation should be opt-in/ handled by them as well.

ajgeiss0702 · 2023-05-07T16:14:30Z

It would implement it for the appDir, not the actual app. The fallback page would still be needed for the actual app to ensure that 404s in the app will have the correct behavior.

If the user has opted-in to use wildcard matching for static requests, perhaps fallback generation should be opt-in/ handled by them as well.

The simpler way would be, if unset, to only generate it if there are pre-rendered pages, as you suggested earlier

…f no fallback page is generated

…rated when it shouldn't

ajgeiss0702 · 2023-05-26T18:05:58Z

The latest commit should address all previous issues with this.

The default option for generate404 is now auto, which will only generate a fallback page if there are any pre-rendered pages
If no fallback page is generated, then a simple 404 page is generated in the appDir from /src/errors.html (or the default if it doesn't exist) to prevent a resurgence of invalid static requests return index.html instead of 404 #9011

…generation

eltigerchino · 2023-08-11T17:15:04Z

documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md

@@ -49,6 +51,16 @@ The `routes` option allows you to customise the [`_routes.json`](https://develop

 You can have up to 100 `include` and `exclude` rules combined. Generally you can omit the `routes` options, but if (for example) your `<prerendered>` paths exceed that limit, you may find it helpful to manually create an `exclude` list that includes `'/articles/*'` instead of the auto-generated `['/articles/foo', '/articles/bar', '/articles/baz', ...]`.

+### Generate 404
+
+The `generate404` option allows you to specify whether a fallback `404.html` page will be generated.


I wonder if we should rename this to generateFallback so it better matches with the error message users get during build.

> Using @sveltejs/adapter-cloudflare file:///Users/my-app/node_modules/@sveltejs/kit/src/core/postbuild/fallback.js:53 throw new Error(`Could not create a fallback page — failed with status ${response.status}`);

documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md

…generation # Conflicts: # packages/kit/src/core/adapt/builder.js

Co-authored-by: Tee Ming <[email protected]>

documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md

eltigerchino · 2023-08-12T14:12:49Z

packages/adapter-cloudflare/index.js

@@ -16,13 +16,27 @@ export default function (options = {}) {
 			builder.rimraf(tmp);
 			builder.mkdirp(tmp);

-			// generate 404.html first which can then be overridden by prerendering, if the user defined such a page


Should we still account for user-defined 404.html pages before generating it for them? Right now we would be overwriting them as it's generated after the prererendered pages.

Yea it might be better to move this back about builder.writePrerendered line

ghostdevv · 2023-08-12T15:24:54Z

packages/adapter-cloudflare/index.d.ts

+	 *
+	 * @default auto
+	 */
+	generate404?: boolean | 'auto';


Should this be named something like fallback?

Co-authored-by: Tee Ming <[email protected]>

ghostdevv · 2023-08-12T15:46:05Z

documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md

+The default value of `auto` will only generate a fallback `404.html` page if there are *any* prerendered pages.
+
+You may want to set this to `false` if you have a fully dynamic app that does not prerender pages during build time.


Suggested change

The default value of `auto` will only generate a fallback `404.html` page if there are *any* prerendered pages.

You may want to set this to `false` if you have a fully dynamic app that does not prerender pages during build time.

The default value of `auto` or `true` will generate a fallback page if prerendering occurs. This will be generated from your [error page](https://kit.svelte.dev/docs/routing#error).

You may want to set this to false if you have a fully dynamic app that does not prerender pages during build time.

I don't think we need to say this, as auto would prevent this anyway? If we want to mention false then we could say something like Set this option to false to prevent generating a fallback.

Rich-Harris · 2024-01-10T22:34:13Z

I've gone round in circles trying to understand this PR, #9386, #9011 and #9802, and here's where I'm at:

As far as I can tell, there's no real reason to ever generate an SPA-style fallback 404.html page, because there's always a _worker.js to render a 404 page on demand. (If you don't want a _worker.js, you should be using adapter-static.)

The only time we need a 404.html is to handle requests for non-existent assets that are covered by exclude in _routes.json — i.e. /_app/immutable/some-file-that-no-longer-exists.js. In that case, it doesn't need to be a fancy SPA-style fallback, it can literally just be a plaintext Not found response.

Am I missing anything?

ajgeiss0702 · 2024-01-10T22:46:18Z

I've gone round in circles trying to understand this PR, #9386, #9011 and #9802, and here's where I'm at:

As far as I can tell, there's no real reason to ever generate an SPA-style fallback 404.html page, because there's always a _worker.js to render a 404 page on demand. (If you don't want a _worker.js, you should be using adapter-static.)

The only time we need a 404.html is to handle requests for non-existent assets that are covered by exclude in _routes.json — i.e. /_app/immutable/some-file-that-no-longer-exists.js. In that case, it doesn't need to be a fancy SPA-style fallback, it can literally just be a plaintext Not found response.

Am I missing anything?

Specifically my use case for it is that I generally exclude some routes that are usually checked by vulnerability scanners (to save on a few invocations). For example:

"/*/wp-includes",
"/*/wp-includes/*",
"/wp-includes",
"/wp-includes/*",
...etc

I would like for these to have the same 404s as the rest of my site.

Another use case I can think for it is prerendered pages with rest parameters.

For example, if you have a personal site that has some stuff that requires server logic, aswell as a static blog. You can exclude /blog/* to avoid risking going over the routes.json limit. If you just had a plaintext 404 page in 404.html, then users would get that when trying to go to a blog post that doesn't exist, or was removed.

Adding the option would allow devs to manually control whether the SPA is used or the static page is used, if there is some circumstance that was not thought of in this PR (like what happened in #9386 )

Rich-Harris · 2024-01-10T23:28:01Z

Yeah, I did consider the latter option, but thought it was probably overkill. But it would be easy enough to implement. If we were to do that I think we could ditch the 'is something prerendered or not?' logic because it seems a bit complicated and I'm not totally sure why it's relevant — it could just be an option like this:

import adapter from '@sveltejs/adapter-cloudflare';

export default {
  kit: {
    adapter: adapter({
      fallback: 'spa' // 'plaintext' (default) or 'spa'
    })
  }
};

Thoughts?

Rich-Harris · 2024-01-10T23:29:44Z

(Honestly, this would all be so much easier if Cloudflare just let you exclude all static assets, or did away with the arbitrary 100 route restriction, so that hacks like /blog/* weren't necessary in the first place. Weird design choice.)

eltigerchino · 2024-01-18T15:23:44Z

closed by #11596 which now does not generate a fallback by default and provides the ability to do so through the adapter config.

feat: allow disabling generation of 404.html

93c4045

vercel bot deployed to Preview – kit April 25, 2023 05:18 View deployment

Add auto option to generate404 & generate simple 404.html in appDir i…

318c674

…f no fallback page is generated

vercel bot deployed to Preview – kit May 26, 2023 17:56 View deployment

Fixed truthy values in generate404 causing a fallback page to be gene…

abb81ec

…rated when it shouldn't

vercel bot deployed to Preview – kit May 26, 2023 18:03 View deployment

Update docs to mention auto for generate404

b9e8d35

vercel bot deployed to Preview – kit May 26, 2023 18:14 View deployment

Merge remote-tracking branch 'upstream/master' into disable-fallback-…

d3038d7

…generation

vercel bot deployed to Preview – kit May 26, 2023 18:21 View deployment

Run prettier format

483b580

vercel bot deployed to Preview – kit May 26, 2023 18:23 View deployment

ajgeiss0702 changed the title ~~adapter-cloudflare: allow disabling generation of 404.html~~ adapter-cloudflare: allow disabling generation of fallback 404.html May 30, 2023

benmccann added the pkg:adapter-cloudflare label Aug 11, 2023

benmccann requested a review from eltigerchino August 11, 2023 16:54

vercel bot deployed to Preview – kit-svelte-dev August 11, 2023 16:58 View deployment

eltigerchino reviewed Aug 11, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into disable-fallback-…

31e29e0

…generation # Conflicts: # packages/kit/src/core/adapt/builder.js

vercel bot deployed to Preview – kit-default-template August 11, 2023 18:31 View deployment

Update documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md

714adc6

Co-authored-by: Tee Ming <[email protected]>

vercel bot deployed to Preview – kit-default-template August 11, 2023 18:43 View deployment

eltigerchino reviewed Aug 12, 2023

View reviewed changes

ghostdevv reviewed Aug 12, 2023

View reviewed changes

Update documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md

71f8465

Co-authored-by: Tee Ming <[email protected]>

vercel bot deployed to Preview – kit-default-template August 12, 2023 15:36 View deployment

ghostdevv reviewed Aug 12, 2023

View reviewed changes

merge main

f2413f7

vercel bot deployed to Preview – kit-svelte-dev January 10, 2024 22:12 View deployment

Rich-Harris mentioned this pull request Jan 10, 2024

breaking: generate plaintext 404.html for Cloudflare Pages, instead of SPA-style fallback #11596

Merged

6 tasks

eltigerchino closed this Jan 18, 2024

		The default value of `auto` will only generate a fallback `404.html` page if there are any prerendered pages.

		You may want to set this to `false` if you have a fully dynamic app that does not prerender pages during build time.

Uh oh!

adapter-cloudflare: allow disabling generation of fallback 404.html #9762

adapter-cloudflare: allow disabling generation of fallback 404.html #9762

Uh oh!

Conversation

ajgeiss0702 commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Changesets

Uh oh!

changeset-bot bot commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

eltigerchino commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Conduitry commented May 7, 2023

Uh oh!

eltigerchino commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajgeiss0702 commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eltigerchino commented May 7, 2023

Uh oh!

ajgeiss0702 commented May 7, 2023

Uh oh!

eltigerchino commented May 7, 2023

Uh oh!

ajgeiss0702 commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajgeiss0702 commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eltigerchino Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eltigerchino Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghostdevv Aug 12, 2023

Choose a reason for hiding this comment

Uh oh!

ghostdevv Aug 12, 2023

Choose a reason for hiding this comment

Uh oh!

ghostdevv Aug 12, 2023

Choose a reason for hiding this comment

Uh oh!

Rich-Harris commented Jan 10, 2024

Uh oh!

ajgeiss0702 commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rich-Harris commented Jan 10, 2024

Uh oh!

Rich-Harris commented Jan 10, 2024

Uh oh!

eltigerchino commented Jan 18, 2024

Uh oh!

Uh oh!

adapter-cloudflare: allow disabling generation of fallback `404.html` #9762

adapter-cloudflare: allow disabling generation of fallback `404.html` #9762

ajgeiss0702 commented Apr 25, 2023 •

edited

Loading

changeset-bot bot commented Apr 25, 2023 •

edited

Loading

eltigerchino commented Apr 29, 2023 •

edited

Loading

eltigerchino commented May 7, 2023 •

edited

Loading

ajgeiss0702 commented May 7, 2023 •

edited

Loading

ajgeiss0702 commented May 7, 2023 •

edited

Loading

ajgeiss0702 commented May 26, 2023 •

edited

Loading

eltigerchino Aug 12, 2023 •

edited

Loading

ajgeiss0702 commented Jan 10, 2024 •

edited

Loading