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: Fail prerendering for pages with mutative endpoints - fixes #3410 and #4252 #4812

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 4, 2022

There are a few issues up around the confusing things that happen if a page is prerendered with mutative methods. Thankfully, this already errors at buildtime. However, it can be a bit of a footgun during development, as there's no warning what you're doing isn't valid until you try to build.

Added tests to make sure that, if a page should be prerendered (both a build-time and at runtime), it fails to do so with a descriptive message.

Fixes #3410 and fixes #4252.

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented May 4, 2022

🦋 Changeset detected

Latest commit: b9aca90

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

Only downside of this that I can think of is that it'll only fail in dev when a server-side navigation occurs... but I don't think it's even possible to check for prerendering in the client. (A quick search of /packages/kit/src/runtime/client for "prerender" yields... 0 results.)

@Rich-Harris
Copy link
Member

Thank you! There was one small change I needed to make — whereas in the page respond.js module we're checking whether we're actually prerendering at the moment, in the context of this PR we need to check whether we will prerender the page. In the first case we need to check state, but in the second (in e.g. dev mode) we need to check configuration.

Though thinking about it, state.prerender.default is now redundant... one sec...

@Rich-Harris
Copy link
Member

I renamed state.prerender to state.prerendering to make the distinction clearer

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

I renamed state.prerender to state.prerendering to make the distinction clearer

Thanks for renaming this. I knew there was something not making sense -- I thought maybe the prerender configuration was assigned to state. The new name is much clearer. 🙂

@Rich-Harris
Copy link
Member

Ah whoops, looks like I broke something. Will look into it in a bit

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

Bah, I looked into it for a bit, but I'm just not sure what's causing the issue. To save you some time, what's failing is a test in adapter-static that's making sure that when config.kit.prerender.default = false, pages that do not set export const prerender = true are not prerendered. For some reason, your last commit has made the prerendering process prerender index.html even though index.svelte does not export the prerendering attribute. I might have more time to look into it later.

@sarajw
Copy link

sarajw commented May 21, 2022

In case this is useful to you, my experiences:

I tried to update my blog that uses adapter-static the day before yesterday and it didn't build right - only one of my *.svelte pages (incidentally the simplest and most recently edited) was built into a *.html file.

I could open the site via that one rendered html page and - I'm not smart enough yet to understand how it all works - I guess the rest was populating itself via JS and it sort of worked. Apart from, the blog index page which complained about the JSON it was referencing having a rogue < in it at 0.

Anyway eventually I paid attention to some warnings I was getting in the terminal, saying I had to add
prerender: { default: true }
to svelte.config.js, and
"extends": "./.svelte-kit/tsconfig.json"
to jsconfig.json.

Then after that it built fine again. Are these lines I will have to change back once any fixes happen?

Thanks!

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

Hey @sarajw! This is an unrelated issue and is actually intended behavior (unless there's something I'm not understanding about your issue). The terminal warnings you were seeing were trying to tell you this. For adapter-static, you must specify either kit.prerender.default = true (to prerender your site into static HTML) or adapter.fallback = <some_page> (to run in SPA mode). You can read more about both of these options in the adapter documentation -- and feel free to ask more questions in the SvelteKit help channel on Discord!

@sarajw
Copy link

sarajw commented May 22, 2022

Hey @sarajw! This is an unrelated issue and is actually intended behavior (unless there's something I'm not understanding about your issue). The terminal warnings you were seeing were trying to tell you this. For adapter-static, you must specify either kit.prerender.default = true (to prerender your site into static HTML) or adapter.fallback = <some_page> (to run in SPA mode). You can read more about both of these options in the adapter documentation -- and feel free to ask more questions in the SvelteKit help channel on Discord!

Absolutely fine if that's the case, it was just new and surprising. I had already posted in the Discord SvelteKit channel and not got much help or information.

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

Absolutely fine if that's the case, it was just new and surprising. I had already posted in the Discord SvelteKit channel and not got much help or information.

And to be clear, I'm not saying you're not experiencing a bug -- just that I don't see any buggy behavior described in what you said. If you still think you are, please do create a repro and file an issue. 🙂 I personally think it's kind of weird that adapter-static doesn't "just work" without changing the mentioned settings, so it may be worth creating an issue to discuss ergonomics? 🤷

@Rich-Harris Rich-Harris merged commit a72123a into sveltejs:master May 23, 2022
@Rich-Harris
Copy link
Member

thank you!

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github deleted the sejohnson-fail-prerender-on-mutative-page-endpoint branch May 23, 2022 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants