Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adapter-cloudflare ignores _redirects config for cloudflare pages #9138

Open
nosovk opened this issue Feb 20, 2023 · 9 comments · May be fixed by #12821
Open

adapter-cloudflare ignores _redirects config for cloudflare pages #9138

nosovk opened this issue Feb 20, 2023 · 9 comments · May be fixed by #12821

Comments

@nosovk
Copy link
Contributor

nosovk commented Feb 20, 2023

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
Copy link
Member

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
Copy link
Member

eltigerchino commented Feb 21, 2023

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
Copy link
Contributor Author

nosovk commented Feb 21, 2023

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
Copy link
Member

eltigerchino commented Feb 22, 2023

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
Copy link
Contributor

ajgeiss0702 commented Feb 22, 2023

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
Copy link
Contributor Author

nosovk commented Feb 22, 2023

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
Copy link
Member

eltigerchino commented Feb 28, 2023

#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
Copy link
Contributor

#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

@eltigerchino
Copy link
Member

Here's a workaround in the meantime that reads the _redirects file and excludes the routes.

// svelte.config.js
const redirects = readFileSync('./static/_redirects', 'utf8')
	.split('\n')
	.filter(Boolean)
	.map((line) => {
		const [from] = line.split(' ');
		if (!from) {
			throw new Error(`Invalid _redirects entry: ${line}`);
		}
		return from;
	});
const config = {
	kit: {
		adapter: adapter({
			routes: {
				include: ['/*'],
				exclude: ['<all>', ...redirects]
			}
		}),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants