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: Cloudflare Pages _routes.json specification (#6441) #6530

Merged
merged 4 commits into from Sep 21, 2022

Conversation

jrf0110
Copy link
Contributor

@jrf0110 jrf0110 commented Sep 2, 2022

Closes #6441

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

changeset-bot bot commented Sep 2, 2022

🦋 Changeset detected

Latest commit: 9cc96c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@sveltejs/adapter-cloudflare Patch
@sveltejs/kit Patch
@sveltejs/adapter-auto Patch

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

@netlify
Copy link

netlify bot commented Sep 2, 2022

Deploy Preview for kit-demo ready!

Name Link
🔨 Latest commit c25d8e8
🔍 Latest deploy log https://app.netlify.com/sites/kit-demo/deploys/63121e05b58e7200095b7810
😎 Deploy Preview https://deploy-preview-6530--kit-demo.netlify.app/build
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link

@jahands jahands left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jacob from the Cloudflare Pages team here - I'm also working on this project.

LGTM

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! comments/questions inline

builder.log.info(
`adapter-cloudfare is writing its own _headers file. If you have your own, you should duplicate the headers contained in: ${dest}/_headers`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we automate this, and copy the contents of ${builder.config.kit.files.assets}/_headers into ${dest}/_headers if it exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I think that ought to be out-of-scope of this PR. We could optimistically concatenate the auto-generated headers to an existing ${dest}/_headers file. However, I expect most users are running their build process and then copying their src/_headers to ${dest}/_headers after the build completes. We still need to think about the best way to approach this scenario

Comment on lines 82 to 91
// We're being conservative by not excluding all assets in
// /static just yet. If there are any upstream auth rules to
// protect certain things (e.g. a PDF that requires auth),
// then we wouldn't want to prevent those requests from going
// to the user functions worker.
// We do want to show an example of a _routes.json that
// excludes more than just /_app/immutable/*, and favicons
// are a reasonable choice
.filter(({ file }) => file.includes('favicon'))
.map((asset) => asset.file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on 'upstream auth rules'? Where would those be expressed? All static files are considered to be outside SvelteKit's purview (unless we added some platform-agnostic 'middleware' concept in future), so it feels like we could just list all static assets here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would those be expressed?

This would be the case of a Cloudflare Pages user adding logic to a catch-all functions route (e.g. functions/_middleware.ts or functions/[[path.ts]] or simply having a _worker.js file). They might be adding headers to static assets with their functions or protecting a certain files to only be accessible by emails ending in @example.com. Either way, we don't want to be too prescriptive on whether or not functions runs before the asset or not.

Do note that omitting a file from excludes will never accidentally break a user's function. The reverse is not true; A misplaced exclusion rule could prevent a function from being run that the user intended to be run. For that reason, we're intentionally being conservative on which assets are explicitly being excluded.

Further, there is a size limit for these descriptions. We're maxing out the total number of rules - include or exclude - to 100. I'm sure plenty of sites would max out that rule limit with just excludes of static paths

Comment on lines 116 to 118
getBuildData() {
return build_data;
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do everything we want in this PR without introducing new API surface area. writeClient(dest) returns an array of all the written files, so we could use that in place of build_data.manifest_data.assets, and app_dir can be accessed as builder.config.kit.appDir.

BuildData isn't a public type and it would be better to keep it that way, otherwise it becomes part of the contract and is thus subject to semver

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger that. I'll remove

@@ -23,18 +23,32 @@ export default function () {
builder.rimraf(tmp);
builder.mkdirp(tmp);

builder.writeClient(dest);
const written_files = builder.writeClient(dest);
console.log('written_files', written_files);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Remove

When a SvelteKit project is deployed to Cloudflare Pages, the server-side code is supported through Pages Functions (currently in beta). Unfortunately, by using Functions, each request must go through the server-side Functions code even if it doesn't need to. For instance, an immutable asset request to https://kit.svelte.dev/_app/immutable/assets/_layout-ab34ca4f.css would first have to route through Functions.

This is problematic since the request would "count" as a request to Functions even though only a static asset was served. Further, there is a slight amount of added latency.

This change exposes a set of includes and excludes based on static files generated.
@jrf0110
Copy link
Contributor Author

jrf0110 commented Sep 16, 2022

Hey @Rich-Harris aside from the the automerging _headers file thing, I've addressed your feedback. Mind taking another gander?

@Rich-Harris Rich-Harris merged commit ae8225d into sveltejs:master Sep 21, 2022
@Rich-Harris
Copy link
Member

thank you!

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

Successfully merging this pull request may close these issues.

Upcoming change to Cloudflare Pages projects
3 participants