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

Prerender routes json #7708

Closed
wants to merge 7 commits into from
Closed

Prerender routes json #7708

wants to merge 7 commits into from

Conversation

colecrouter
Copy link

Problem

Prerendered pages on adapter-cloudflare should not require a worker request. See issue for more info #7298

Solution

Prerender pages as /foo/index.html instead of /foo.html, then exclude /foo in _routes.json. This will bypass the worker entirely, and still serve the prerendered page.

I ran the test suite, and tested it on my site with vite dev and deployed to Cloudflare Pages. I got the feeling that the change to packages/kit/src/core/prerender/prerender.js would cause more friction that it seemed to.

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2022

⚠️ No Changeset found

Latest commit: 0566ee2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@colecrouter
Copy link
Author

Y'know, I suppose it would break the prerender tests, wouldn't it? haha

@colecrouter
Copy link
Author

Not sure about that last test. I can't seem to get goto() to react to anything.

@Rich-Harris
Copy link
Member

Thanks you but we can't just change about.html to about/index.html willy-nilly. One means /about, the other means /about/.

I'm also not sure this approach scales very far. There's quote a low limit to the number of entries you can have in your _routes.json (100 for include and exclude combined, IIUC) which many sites would immediately exceed.

@colecrouter
Copy link
Author

Thanks you but we can't just change about.html to about/index.html willy-nilly. One means /about, the other means /about/.

That's what I've been alluding to in #7298 . I've got it working to Cloudflare's spec using wrangler, and vite dev the behavior seems to function identically, but I don't know what the exact expected behavior to start with (if different). /about still resolves correctly, and /about/index.html still 404s (when I was testing). Although I can't speak to /about vs /about/, Chrome usually changes that automatically 😅

The one failed test says something is wrong, but I can't seem to get it to work no matter what I do (I'm not too familiar with playwright, unfortunately). Aside from that, it seems to work fine for me "in practice".

I'm also not sure this approach scales very far. There's quote a low limit to the number of entries you can have in your _routes.json

~100 prerendered pages? How about adding a cap, or a switch + warning if over 100 entries. Then the worst case would be what's currently happening.

@vercel
Copy link

vercel bot commented Dec 7, 2022

@Mexican-Man is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@colecrouter
Copy link
Author

Instead of automatically excluding prerendered pages, I've put in an adapter option that lets you manually specify if you want to bypass. That seems a lot safer. The Vite exports for prerendered pages are working properly now, so any prerendered test errors seem to be resolved (on my computer).

@Rich-Harris
Copy link
Member

I merged #8422, which adds all assets and prerendered pages (up to the 100-rule limit), so I'll close this — thanks.

In general I think the automated approach is preferable to manually specifying exclusions, though in the rare cases that it is necessary it could be done by modifying the _routes.json after it's been created.

@colecrouter
Copy link
Author

colecrouter commented Feb 8, 2023

Bit late, but just wanted to say thanks. I just took a look at last month's bill, and we're down 85% on billed requests 😊(just from prerendered pages, alone). HUGE; thanks @Rich-Harris!

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.

3 participants