sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.4k stars 1.88k forks source link

adapter-cloudflare ignores `_redirects` config for cloudflare pages #9138

Open nosovk opened 1 year ago

nosovk commented 1 year ago

Describe the bug

Cloudflare provides functionality to handle redirects using _redirects config file (doc) When you try to use Cloudflare pages redirects with svelte kit project, then Cloudflare redirects are ignored. It seems that __routes config, generated by sveltekit covers all routes with *

image
  "include": [
    "/*"
  ],

It means that all requests handled by worker script, before being passed to redirect.

Proposed solution: add contents of _redirects to exclude section, like static assets.

Reproduction

Sorry, but I can't provide public Cloudflare account as reproduction. I can only say that problem exists in any svelte kit project deployed to cloudflare pages.

Logs

No response

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (8) x64 Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz
    Memory: 7.43 GB / 31.88 GB
  Binaries:
    Node: 18.12.1 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.15 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 8.19.3 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (110.0.1587.50)
    Internet Explorer: 11.0.19041.1566
  npmPackages:
    @sveltejs/adapter-cloudflare: 2.0.2 => 2.0.2
    @sveltejs/kit: ^1.7.2 => 1.7.2
    svelte: ^3.55.1 => 3.55.1
    vite: ^4.1.1 => 4.1.2

Severity

blocking an upgrade

Additional Information

No response

ghostdevv commented 1 year ago

Are the docs wrong when they say to place this file in static and it will work? https://kit.svelte.dev/docs/adapter-cloudflare#notes

eltigerchino commented 1 year ago

Are the docs wrong when they say to place this file in static and it will work?

https://kit.svelte.dev/docs/adapter-cloudflare#notes

No, the docs are correct.

@nosovk may I know what your use case is for this? If you are attempting to add redirects for a prerendered page , this should work as expected (I was wrong). Otherwise, SSR routes should use Cloudflare redirect rules / page rules, or implement the redirect logic to the sveltekit app itself.

nosovk commented 1 year ago

We have site, which is hosted on Cloudflare pages. Currently, we need to change URL structure (rename categories, move articles over the categories etc.). When we move pages we should leave 301 redirects on the previous address, and point it to the new destination. We have nearly a hundred redirects... It's much easier to hold them all in __redirects config, that will serve those redirects without launching workers.

Currently we do something like that:

[...rest]/+page.sever.ts


import { error, redirect } from '@sveltejs/kit';
import type { PageServerLoad } from './$types';

const redirects = { '/actions': '/what-we-do', '/actions/200-days-into-russias-war-on-ukraine': '/whats-new/200-days-into-russias-war-on-ukraine', // ... /// ... };

export const load: PageServerLoad = async ({ url }) => { if (redirects[url.pathname]) { throw redirect(301, redirects[url.pathname]); } throw error(404, 'Not found'); };


and it actually works when we have our categories renamed, like actions => whats-new

But it's really good idea to manage all those redirects in one place, especially if there is some solution from platform.
eltigerchino commented 1 year ago

We have site, which is hosted on Cloudflare pages. Currently, we need to change URL structure (rename categories, move articles over the categories etc.). When we move pages we should leave 301 redirects on the previous address, and point it to the new destination. We have nearly a hundred redirects... It's much easier to hold them all in __redirects config, that will serve those redirects without launching workers.

Thank you for sharing. You've actually helped correct my understanding about Cloudflare Pages redirects (previously I thought the redirect would bypass the worker, thus fail when arriving at the destination).

I'd love to try my hand at implementing this. Although, one thing to be wary of is the 100 rule limit for _routes.json. A number of those rules would already be used up by assets and prerendered pages, leaving very little to no room left for those redirects if your site is very large.

Should the build warn or fail when the _routes.json rules have been exceeded? Should _redirects take priority over static assets and prerendered pages? (which will still be served by the worker if not listed in _routes.json).

ajgeiss0702 commented 1 year ago

We have nearly a hundred redirects...

I would like to add that you cannot add all of your redirects to _routes.json. _routes.json has a limit of 100 rules. Unless many of those redirects could be handled under wildcards

nosovk commented 1 year ago

I think that prerendered pages and static assets should be level 1 citizen in routes, and warning user about that limit o space for redirect reached is Ok. I actually didn't knew about limit of 100 elements in _routes.json. Limit for _redirects is 500 elements, which makes the wholl situation a bit broken... Adding wildcards is a manual approach, which might be not simple at all for most users.

eltigerchino commented 1 year ago

https://github.com/sveltejs/kit/pull/9111 allows manually excluding routes.

Should the adapter warn the developer when a _redirects file is found and certain routes aren't excluded?

Or should the adapter try to automatically include the routes found in _redirects?

ajgeiss0702 commented 1 year ago

9111 allows manually excluding routes.

Should the adapter warn the developer when a _redirects file is found and certain routes aren't excluded?

Or should the adapter try to automatically include the routes found in _redirects?

9111 adds placeholders that allow automatic generation of exclude rules for things like _app, static files, and pre generated pages. I’m planning on adding another placeholder that would excludes redirects