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

fix: skip calling respond for server-side fetch on prerendered pages #13377

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aloisklink
Copy link
Contributor

Server-side fetch() (e.g. when isSubRequest: true) currently has a few bugs when called on a prerendered route.

  1. When a non-prerendered route also matches the same URL as the prerendered route, the non-prerendered routes are taking precedence (e.g. if export const prerender = 'auto'). Fixes Global slug overrides prerendered route when accessed from another route with event.fetch, but ok from client? #12739.
  2. Prerendered routes still have hooks.server.js run on them, but only when isSubRequest: true

I've fixed both of these issues by adding a prerendered_routes: Set<string> to the manifest. Then, we instantly call a real HTTP fetch() if the path matches, just like when calling server-side fetch() on assets (when the adapter doesn't support reading assets from the file-system).

During dev/build, there are no prerendered_routes, so it's left empty in the manifest (although that does mean the manifest-full.js also excludes it!)

Potential issues

  • According to the docs for prerendered.paths (which I'm using to set prerendered_routes), it only has the routes without a trailing slash. Would this be an issue? If so, the code might get a lot more complicated!
    /** An array of prerendered paths (without trailing slashes, regardless of the trailingSlash config) */
    paths: string[];
  • In the future, we might want to make a new entry like prerendered_redirects, so that we can follow the redirect chain without having to make a real HTTP request, but I think that should be fine if this is under the private _ field in the manifest.js file.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check
    • pnpm test fails on main locally for me, so let's see if the CI passes on it!

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:.
    • I've made two separate changesets for both the bugs I've fixed! Let me know if you want me to combine them (or if I should add more info to them, since I've just copied my commit message title, which are 50 chars long)

Edits

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

When using server-side fetch for internal requests, if a server route
is matched from the server `manifest.js`, it gets called without making
a real HTTP request.

However, prerendered routes are not included on this list! This is fine
when routes are prerendered with `export const prerender = true;`, but
will cause issues with a non-prerendered route also matches the same
URL as any prerendered routes.

This commit adds `prerendered_routes: Set<string>` to the `manifest.js`,
which skips calling the non-prerendered route.
Doing a server-side `fetch()` (e.g. `isSubRequest: true`) to a route
that is prerendered runs `hooks.server.js`,
which is different behaviour from a normal `fetch()`.
Copy link

changeset-bot bot commented Jan 24, 2025

🦋 Changeset detected

Latest commit: 3ed5527

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit 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

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.

Global slug overrides prerendered route when accessed from another route with event.fetch, but ok from client?
1 participant