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

feat: append prerendered redirects to _redirects file #12199

Merged
merged 6 commits into from
Jul 23, 2024

Conversation

Zhincore
Copy link
Contributor

@Zhincore Zhincore commented May 8, 2024

Makes Cloudflare Pages adapter generate a static _redirects file in build output.
This way the redirects happen on server and don't have to be a client-side HTML file as it is now.

It simply collects redirects from builder.prerendered.redirects, so I am not sure how reliable that is?

My example purpose is that I have following +server.ts file:

import { redirect } from '@sveltejs/kit';
import path from "$data/file.json?url"
import type { RequestHandler } from './$types';

export const prerender = true;

export const GET: RequestHandler = async () => {
    redirect(302, path);
};

Normally the adapter generates a __data.json and an .html file that redirects the user there (which still happens, I didn't change that), but I need that redirect to happen on server, so that the file.json file is served.


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. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented May 8, 2024

🦋 Changeset detected

Latest commit: be8b4bb

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 eltigerchino changed the title feat: Generate static _routes in Cloudflare adapter feat: append prerendered redirects to _redirects file May 10, 2024
@benmccann benmccann requested a review from eltigerchino June 21, 2024 03:13
packages/adapter-cloudflare/index.js Outdated Show resolved Hide resolved
packages/adapter-cloudflare/index.js Show resolved Hide resolved
@Zhincore
Copy link
Contributor Author

Zhincore commented Jul 1, 2024

If the target of redirect is a small file, it gets included in the redirects as data:base64 URL (not valid in Cloudflare's eyes), is there a way to prevent it, apart form changing Vite's inline limit?

Due to the latest change, client gets served the HTML/JS redirect instead of being redirect by the server.

@eltigerchino
Copy link
Member

If the target of redirect is a small file, it gets included in the redirects as data:base64 URL (not valid in Cloudflare's eyes), is there a way to prevent it, apart form changing Vite's inline limit?

Not that I know of. Your best bet is to add a predicate to the build.assetsInlineLimit option so it returns false when it matches the target filepath https://vitejs.dev/config/build-options.html#build-assetsinlinelimit

Due to the latest change, client gets served the HTML/JS redirect instead of being redirect by the server.

Which change are you referring to? Can you elaborate?

@Zhincore
Copy link
Contributor Author

Zhincore commented Jul 1, 2024

The predicate might be a good thing to be mentioned in the docs if this PR gets merged.

Which change are you referring to? Can you elaborate?

I'm referring to my own commit which excludes the redirected path from being processed by the worker

@eltigerchino
Copy link
Member

Which change are you referring to? Can you elaborate?

I'm referring to my own commit which excludes the redirected path from being processed by the worker

I'm not sure if I understand. Isn't the _redirects rule serving the redirect instead of the HTML/JS file?

@Zhincore
Copy link
Contributor Author

Zhincore commented Jul 1, 2024

I'm not sure if I understand. Isn't the _redirects rule serving the redirect instead of the HTML/JS file?

Yes, exactly, but when the files gets inlined, it gets inlined inside the _redirects as well (it's a data: URL instead of a path to file). Cloudflare ignores that rule as invalid and serves the static HTML instead.

@eltigerchino
Copy link
Member

eltigerchino commented Jul 1, 2024

I'm not sure if I understand. Isn't the _redirects rule serving the redirect instead of the HTML/JS file?

Yes, exactly, but when the files gets inlined, it gets inlined inside the _redirects as well (it's a data: URL instead of a path to file). Cloudflare ignores that rule as invalid and serves the static HTML instead.

Weird, I wasn't able to reproduce that. When I followed the +server.js example from the PR description, there simply wasn't any HTML file reproduced and the _redirects file was empty. Maybe we can throw an error with instructions on how to fix this if one of the redirect locations is a base64 string (I'd open a new issue and PR for this if the inlined base64 string causes the prerendered redirect to disappear because it sounds different from the scope of this PR).

Side note: we should probably add a check to see if there are any prerendered redirects before writing to _redirects:

+ if (builder.prerendered.redirects.size > 0) {
	writeFileSync(`${dest}/_redirects`, generate_redirects(builder), { flag: 'a' });
+ }

@eltigerchino eltigerchino merged commit b2b7e34 into sveltejs:main Jul 23, 2024
12 checks passed
@eltigerchino
Copy link
Member

Thank you!

@github-actions github-actions bot mentioned this pull request Jul 23, 2024
paoloricciuti pushed a commit to paoloricciuti/kit that referenced this pull request Jul 31, 2024
* feat: Generate static _routes in Cloudflare adapter

* Update .changeset/ten-shirts-add.md

* fix: add comements and newline in generated _redirects file

* fix: exclude redirect paths from cloudflare

* fix: check if there are redirects before writing

* Fix formatting

---------

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants