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: Throw when prerendered routes have unresolvable content negotiation #9994

Conversation

elliott-with-the-longest-name-on-github
Copy link
Contributor

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github commented May 20, 2023

Closes #9929
Closes #10102

I'm not positive this is the right solution here, but it definitely isn't breaking any tests and I think it's conceptually sound. The issue is that when prerendering, we don't set the Accept header, meaning routes with both a +page.svelte and +server.ts file with a GET handler will negotiate to the GET handler rather than the page, causing incorrect prerender output. Given that prerendering is supposed to emulate browser behavior (by "visiting" each page), it seems logical to set this header. It certainly fixes the problem in the linked issue.

If a prerender request is made to a route that has unresolvable content negotiation, throws. What this means practically is that you can't prerender a route with both a GET method exported from +server.js and also have a page at that route -- static file servers can't negotiate this request after prerendering, so the behavior is undefined.

This also fixes a minor bug: With the other route options, we used the precedence of +page => +page.server => +endpoint for option resolution. For entries, we were incorrectly using a precedence of +endpoint => +page => +page.server, which is all kinds of weird. I think that might technically be breaking, but it's a very new feature and I don't know anyone who'd argue that it's the correct order of precedence, so I'm in favor of treating it as a fix.

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:.

@changeset-bot
Copy link

changeset-bot bot commented May 20, 2023

🦋 Changeset detected

Latest commit: 9999c99

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

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github changed the title Elliott/9929 fix entry exports fix: Make routes with a +server.ts and +page.svelte prerender +page.svelte May 20, 2023
@Rich-Harris
Copy link
Member

Thinking further on this, it strikes me that if a route is being prerendered then it's a straight-up error to have both a +page.svelte and a +server.js, and we should treat it as such rather than trying to figure out which one 'wins'. Thoughts?

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

@Rich-Harris

This should be ready to go, with the caveat that I didn't add any dev-time errors. I have no idea where to put them. If you point me at the right spot, I'll add 'em.

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github changed the title fix: Make routes with a +server.ts and +page.svelte prerender +page.svelte fix: Throw when prerendered routes have unresolvable content negotiation May 30, 2023
Comment on lines +12 to +17
"@sveltejs/adapter-auto": "workspace:^",
"@sveltejs/kit": "workspace:^",
"svelte": "^3.56.0",
"svelte-check": "^3.0.2",
"typescript": "^4.9.4",
"vite": "^4.3.6"
Copy link
Member

Choose a reason for hiding this comment

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

we should keep these in sync with the other test projects

Suggested change
"@sveltejs/adapter-auto": "workspace:^",
"@sveltejs/kit": "workspace:^",
"svelte": "^3.56.0",
"svelte-check": "^3.0.2",
"typescript": "^4.9.4",
"vite": "^4.3.6"
"@sveltejs/adapter-auto": "workspace:^",
"@sveltejs/kit": "workspace:^",
"svelte": "^4.0.5",
"svelte-check": "^3.4.4",
"typescript": "^4.9.4",
"vite": "^4.4.9"

@benmccann
Copy link
Member

I'm not sure this is the direction I would have gone in. If you could get either HTML or JSON from the same URL depending on the request header, I would assume we should default to the HTML version. I'm not sure I like the idea of forcing the user to restructure their URLs because they turned on pre-rendering. If there's no HTML version then throwing an error makes sense to me. But I do understand the opposite point of view as well and see that Rich had suggested making this an error above (#9994 (comment)).

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

But the point is, you can't get both if you're pre-rendering. The request to the route is made by the SvelteKit crawler -- if you have both a GET endpoint and a page, we have no way of defining what should happen, other than silently just preferring one over the other.

@benmccann
Copy link
Member

I imagine 95% of the time that users hit this, you've got an HTML page which gets its JSON data from an endpoint at the same URL. It seems like it'd be nice to be able to prerender that HTML page. I don't think the JSON API is something that's really user visible and so if users don't have the ability to access that when prerendering is turned on that seems fine to me.

It's been awhile since I've looked at the prerendering code though. I don't remember how the fetch responses are handled during prerendering and whether this is merely a decision of choosing whether we want to do that or whether it would not be technically feasible to prerender in that scenario.

@eltigerchino
Copy link
Member

eltigerchino commented Oct 11, 2023

We could set the accept header to negotiate content while prerendering. Using the metadata to know which routes are endpoints / pages and send a prerender request them with the correct accept header. Maybe only throw an error if the prerendered endpoint is a HTML file? A static server could still serve /about.html, /about and /about.json.

for (const [id, { prerender }] of metadata.routes) {
if (prerender !== undefined) {
prerender_map.set(id, prerender);
}
}

@Rich-Harris
Copy link
Member

this was superseded by #11256, so i'll close it.

while it's true that some static webservers can be configured to do content negotiation, i really don't think that's something we want to get into. the point of static prerendering is to have an output that works more or less everywhere

@dummdidumm dummdidumm deleted the elliott/9929-fix-entry-exports branch December 13, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants