include as many static assets as possible in exclude list

[Rich-Harris]

closes #7640. need to check that prerendered pages (with/without trailing slashes) will still work with this setup.

cc @jrf0110 — this causes all static assets to bypass functions (or rather, as many as can fit in the _routes.json limit, which will be plenty for some but insufficient for others). I know you initially said you wanted people to be able to apply custom headers and so on, but a) the consensus seems to be that people want to avoid the cost of invoking functions for every request, and b) we don't currently have a way to add any custom logic when the worker serves a static file anyway, and I'm not aware of any requests for that behaviour

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.

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. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 579cee0

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

[dummdidumm]

(not merging yet as we may want to wait on the CC'd person's response)

[jrf0110]

LGTM! Please do reach out if y'all have any issues.

Follow-up - is it possible to configure sveltekit to have a single directory for all static files? e.g. /static/* Then we could consolidate the exclusion rule list to a single glob

[Rich-Harris]

Not really, no — it's common to use static to contain things like robots.txt and .well-known files. But even if we did, would that mean that if someone requested /static/non-existent-file, they'd see a platform 404 instead of the app 404?

[jrf0110]

Depends on what you mean by platform 404. In that case, the 404 behavior would be in control of the developer.

Supposing we had a _routes.json with:

{
  "exclude": ["/static/*"]
}

And someone performed:

GET /static/non-existent-file

That means the request would bypass the Cloudflare Pages Function - i.e. the SvelteKit server - And passthrough to the Cloudflare Pages Asset Server where the default 404 behavior would kick-in. If the deployment provides a 404 page, that will get served up. Otherwise, the index page will be served by default (SPA-mode rendering).

[Rich-Harris]

That would entail a new SvelteKit feature - we can create a 'fallback' page that in theory could be the 404.html but that's intended for SPAs - it doesn't include any body content. We absolutely don't want the index page to be rendered by default in any circumstances, so wildcards are a no-go for us.

In any case a user could theoretically have a src/routes/[...catchall] route, which handles requests for /static/non-existent-file.

The simplest thing by far for us - and I suspect most Cloudflare Pages users - would be a boolean that opted all static assets out of functions without messing around with _routes.json. Consider this a feature request :)

[ajgeiss0702]

Found an issue using this branch. It seems to add a lot of nulls to the _routes.json, which then causes it to reach the 100 limit and send a warning.

Here is the file it generates: https://pastebin.com/raw/4KqcrRwd

---

I'm not sure if its related, but for some reason it doesnt seem to generate a _worker.js file? (causing the build to fail)
Build log: https://pastebin.com/raw/gDhQSaC6

[Rich-Harris]

@ajgeiss0702 I can't reproduce that, and I can't figure out how it could be happening. Any chance you can whip up a repro?

[Rich-Harris]

(For the second thing, about not generating a _worker.js file, that's probably because you need to run pnpm build inside packages/adapter-cloudflare)

[ajgeiss0702]

@ajgeiss0702 I can't reproduce that, and I can't figure out how it could be happening. Any chance you can whip up a repro?

Of course. Might take me a few days as I currently cannot share the repo that it is actively happening on so I'll need to extract some parts until I can reproduce the issue with a basic create-svelte project.

My guess, glancing at the adapter's code, is that there is some condition where excess has a value, but the exclude list is not actually >98. Then exclude.length = 99; adds nulls.

(For the second thing, about not generating a _worker.js file, that's probably because you need to run pnpm build inside packages/adapter-cloudflare)

Ah, I knew I was doing something wrong. Running that allowed it to build.

[ajgeiss0702]

It turns out I was able to reproduce it faster than I thought it would take.

The issue occurs if the site has more than ~50 static assets (assets in the static folder and prerendered pages)

In this repo, I basically just generated a bunch of prerendered pages until the issue occurred. Adding more assets to the static folder lowers the number of prerendered pages required for the issue to appear.
WARNING: I set adapter-cloudflare to be downloaded from my site. If this is too scary for you, feel free to change it to your own (its not modified from this branch other than running npm run build)
https://github.com/ajgeiss0702/null-routes

[Rich-Harris]

Ah, I see what's happening — it's counting contents of _app individually towards the total, but those are all grouped together under _app/*. Fix inbound

[ajgeiss0702]

When attempting to deploy to cloudflare pages, the build succeeds, but on the last step I get this:

Error: Failed to publish your Function. Got error: Error 8000057: Rules must start with '/' in `_routes.json`. Refer to https://cfl.re/3FsE4aF.

It appears that the assets from the static folder are the ones that do not start with /

[korywka]

Seems like broken for routes that are NOT excluded (direct URL gives 404).
removing export const trailingSlash = 'always'; fixed that.

@Rich-Harris FYI.

[korywka]

If you need a sample repo to reproduce, please ping me and I will open the private repo. But looks like 101+ route with trailingSlash option works only with JS redirect, and not by direct route.

[benmccann]

@korywka @ajgeiss0702 can you file an issue? I'm afraid comments on a closed PR are likely to get lost. Thanks for tracking down the cause though as that will be a big help!

[korywka]

@benmccann done, thanks!

[ajgeiss0702]

@benmccann Rich fixed that problem right away in #8618

korywka's problem is separate from mine, so I agree that there should be an issue for that

[eweren]

Is there a way to set those include/excludes manually? We have slug-urls that are longer than 100 characters (another limit of cloudflare) so that this PR breaks the build. Would be awesome to set (or generate) some include/exclude routes by pattern, so that we won't have that many characters for each route and instead just say exclude: ["courses/[handle]"]. @Rich-Harris

[dummdidumm]

Could you open a new issue for that?

[eweren]

#8827 hope bug report was correct 🤷