feat: add options to cloudflare-adapter to allow custom _routes rules

[ajgeiss0702]

Inspired by suggestions in #8827

This PR adds an options object to adapter-cloudflare, which allows configuration of custom includes/excludes for cloudflare's _routes.json file. It also allows disabling the auto-generation of excludes.

I'm not sure whether I should document these changes on the adapter-cloudflare docs page in this PR or a separate one, so for now I am leaving that out. Please advise!

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 N/A: adapter-cloudflare has no tests

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:.

[ghostdevv]

Small nitpick 🙏

[nosovk]

It could help with: #9138

[ajgeiss0702]

It could help with: #9138

This pr won’t (unless you add it manually), but once this is merged I plan on adding more similar features to the cloudflare adapter

[Rich-Harris]

Thanks for the PR! I'm not sure this is quite right in its current form — if you're opting out of automatically generating routes, then we shouldn't always be adding /* to includes and /_app/* to excludes. Manual should mean manual, otherwise it's confusing and contradictory. In any case if /* is present then any other include is redundant.

So I'd propose getting rid of autoGenerate and just saying that if include or exclude are provided, use them, otherwise generate them automatically using the existing logic. We also need to make sure that inclusions are taken into account when determining whether we've exceeded the limits or not — at the moment this PR only considers excludes.

[ajgeiss0702]

Thanks for the PR! I'm not sure this is quite right in its current form — if you're opting out of automatically generating routes, then we shouldn't always be adding /* to includes and /_app/* to excludes. Manual should mean manual, otherwise it's confusing and contradictory. In any case if /* is present then any other include is redundant.

That makes sense. I'll make that change (probably only include /* and exclude /_app/* if autoGenerate is true)

So I'd propose getting rid of autoGenerate and just saying that if include or exclude are provided, use them, otherwise generate them automatically using the existing logic.

My reasoning for adding the autoGenerate is if a new pre-generated page or asset is added and you have custom excludes specified, if you forget to add the new page/asset it will incur unnecessary function/worker invocations. The autoGenerate option allows custom routes without the worry of constantly having to adjust it when adding/removing pre-generated pages or static assets. If one would like to have fully custom include/exclude, they can disable the autoGenerate option.

We also need to make sure that inclusions are taken into account when determining whether we've exceeded the limits or not — at the moment this PR only considers excludes.

Will do.

Thank you for the feedback on the PR, however, I am still not sure if I should make the docs changes in this PR or in a separate one.

[eltigerchino]

Adding the docs in this PR would be great 👍

[Rich-Harris]

I'm still not sure I follow — if you have /* in includes then there is no point adding anything else. You either have /* or you have manual includes.

Similarly for excludes: it already has everything that should be excluded, so adding additional entries is pointless. The exception is when the list grows too long and you need to handcraft the list making clever use of wildcards. But in that case you can't rely on the auto-generated excludes anyway, so it's very much on you to keep the configuration up to date.

So I don't see any reason to keep autoGenerate, unless there's something I'm missing?

[ajgeiss0702]

I'm still not sure I follow — if you have /* in includes then there is no point adding anything else. You either have /* or you have manual includes.

Similarly for excludes: it already has everything that should be excluded, so adding additional entries is pointless. The exception is when the list grows too long and you need to handcraft the list making clever use of wildcards. But in that case you can't rely on the auto-generated excludes anyway, so it's very much on you to keep the configuration up to date.

So I don't see any reason to keep autoGenerate, unless there's something I'm missing?

Including /* without autoGenerate was a mistake. I'm about to push a commit fixing that.

Allowing custom excludes rules in addition to the auto-generated ones would allow _redirects to work. You could exclude the paths that are in _redirects (as currently _redirects does not get priority over functions)

[Rich-Harris]

as currently _redirects does not get priority over functions

I find some of these design decisions... interesting.

Should the contents of _redirects be included in excludes for that reason? (Behind an option?) In the common case where you don't have _redirects and are instead using excludes to stay below the limit, the above point stands as far as I can see — it makes sense to auto-generate excludes or manually specify them but not both. So we need to come up with a solution that deals with redirects specifically.

[ajgeiss0702]

as currently _redirects does not get priority over functions

I find some of these design decisions... interesting.

Should the contents of _redirects be included in excludes for that reason? (Behind an option?) In the common case where you don't have _redirects and are instead using excludes to stay below the limit, the above point stands as far as I can see — it makes sense to auto-generate excludes or manually specify them but not both. So we need to come up with a solution that deals with redirects specifically.

I believe _redirects rules should be automatically excluded in excludes, but currently i'm planning on adding that in a future PR.

Another use case for adding excludes on top of the auto-generated ones (one that I am currently struggling with) is handling 404 pages that are commonly requested. For example, bots scanning for vulnerable wordpress sites, or old api routes that has been removed for years but still gets tens of thousands of requests per day.

I don't see the harm in keeping the option, unless I'm missing something.

[Rich-Harris]

The harm is this:

you can't rely on the auto-generated excludes anyway

Either the automatic excludes are below the limit, or they're unreliable. We can't have it both ways!

[ajgeiss0702]

The harm is this:

you can't rely on the auto-generated excludes anyway

Either the automatic excludes are below the limit, or they're unreliable. We can't have it both ways!

If they are below the limit, there is room to add more. If they are not below the limit, autoGenerate can be disabled. That is why it is an option, and not unconditionally always on.

I believe that the warning message and the comments around the options make it perfectly clear on the behavior of the options, so there would not be any clarity improvement by removing the option. From what I can see, removing the option would only cause pain for anyone who just wants to add a few excludes, but wants to keep the auto-generated excludes.

I don't believe removing functionality just because it might cause a warning that describes the issue clearly is the right move here. Not every sveltekit site is the same, and there are many sites that could use this option. There are also many who might not need to use this option. This is why it is an option. Take the option away and workarounds would need to be made (for instance, as a workaround to my previously mentioned 404 problem, I made empty static files of the old path. This, obviously, is not ideal)

[ajgeiss0702]

An alternative I thought of would be to have the behavior you described, but with an option called appendAutoGeneratedRules (maybe something shorter) that would do what it says, and default to false. Append the auto-generated rules to the one you provided.

[Rich-Harris]

Ok, re-reading the 404 thing I understand the motivation a bit better — this isn't just about manually defining excludes to avoid exceeding the limit.

I'm not sure I understand how appendAutoGeneratedRules would be different to autoGenerate. In both cases, the API design rings a little off for me because it's a sibling to include and exclude but is specific to exclude. I wonder if we should have a special <assets> string or something, that allows you to decide for yourself whether avoiding a function invocation for a static asset is more or less than avoiding one for bots looking for vulnerabilities:

adapter({
  exclude: ['/wp-admin', '<assets>']
})

// or
adapter({
  exclude: ['<assets>', '/wp-admin']
})

In effect, exclude would default to ['<assets>'] while include defaults to ['/*'].

Thoughts?

[ajgeiss0702]

Ok, re-reading the 404 thing I understand the motivation a bit better — this isn't just about manually defining excludes to avoid exceeding the limit.

I'm not sure I understand how appendAutoGeneratedRules would be different to autoGenerate. In both cases, the API design rings a little off for me because it's a sibling to include and exclude but is specific to exclude. I wonder if we should have a special <assets> string or something, that allows you to decide for yourself whether avoiding a function invocation for a static asset is more or less than avoiding one for bots looking for vulnerabilities:

adapter({
  exclude: ['/wp-admin', '<assets>']
})

// or
adapter({
  exclude: ['<assets>', '/wp-admin']
})

In effect, exclude would default to ['<assets>'] while include defaults to ['/*'].

Thoughts?

I believe that would be a good solution. I would assume there could also be something like <pregenerated> too (since static assets are not the only thing excluded).

Unless there are any objections, I will do this within the next few days.

[ajgeiss0702]

Ok. My current plan is to have these 4 placeholders:

• <app> - the /_app dir
• <static> - files from /static
• <prerendered> - prerendered pages
• <auto> - all of the above

Let me know if theres something I'm missing

[Rich-Harris]

Let's stick with <build>, <files> and <prerendered> for consistency with $service-worker

[ajgeiss0702]

Let's stick with <build>, <files> and <prerendered> for consistency with $service-worker

Ah ok. I couldn't find documentation on <build> and <files> at first, and since they arent named very clearly, I assumed they did something else.

Here is what I'm thinking now:

• <build> for /_app
• <files> for static files (from /static)
• <prerendered> for prerendered pages
• <all> all of the above

I will also update the documentation (at least for this adapter) to make it more clear on what <build> and <files> are

[ajgeiss0702]

Ok, it's implemented. To be safe, I will do more testing tonight, but please feel free to review.

[Rich-Harris]

Tweaked the logic a bit:

• got rid of the 'always add /* if include is empty' — this is an error on the user's part and should be treated as such. Hiding errors never helps in the end
• if include.length > 100, fail, because that will result in application errors
• if include.length + exclude.length > 100, truncate exclude, because the application will still work then (it's just less efficient). Never truncate include