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

[breaking] ssr/hydrate/router/prerender.default are now configurable in +page(.server).js and +layout(.server).js #6197

Merged
merged 27 commits into from
Aug 30, 2022

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Aug 23, 2022

How to migrate

browser.hydrate / browser.router / prerender.default

hydrate and router have been replaced with csr. See #6446 for more info.

Remove these options from your svelte.config.js and export them from your root +layout.js or +layout.server.js instead:

const config = {
	kit: {
-		browser: { hydrate: false, router: false },
-		prerender: { default: true }
	}
};
// src/routes/+layout.js
export const csr = false;
export const prerender = true;

(For now, you will need a +layout.svelte alongside your +layout[.server].js — this will be fixed soon.)

You can also set these options at lower layouts or inside pages if you want.

For other changes around prerender see this PR: #6392

handle: resolve(event, { ssr: ... })

The ssr option was removed from the resolve function that is passed into the handle hook. Set it at the appropriate +layout.js or +layout.server.js or +page.js or +page.server.js. The following example shows how to turn off SSR for all admin pages:

export function handle({ event, resolve }) {
   return resolve(event, { 
-      ssr: !event.url.pathname.startsWith('/admin')
   });
// src/routes/admin/+layout.js
export const ssr = false;

PR description

Closes #6356

Can't believe how little code the actual change is 😄

During implementing this, I realized a shortcoming of the current implementation: If +page.js accesses a browser global, for example

document;
export function load() {}

..then this will fail on the server with an error, even if you did set ssr in handle. To change this behavior we would need to lazy-load shared just like we do with component already - should we make this change? If yes, should I incorporate it into this PR?

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 Aug 23, 2022

🦋 Changeset detected

Latest commit: d111a21

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

@dummdidumm
Copy link
Member Author

I created #6207 as the enhanced version of this. I'd say that PR is a worthwhile addition and I doubt that the performance overhead of an additional lazy load is noticeable.

@Rich-Harris
Copy link
Member

Replies to the conversation in #6207:

I have used router on a per-page basis, though admittedly in a very niche scenario. The covid tracker banner on https://www.nytimes.com...


image


...is a block of HTML extracted from a statically rendered SvelteKit app and placed onto the homepage. Disabling the router for the entire embed means that links will always take you to a fresh page, instead of trying to navigate within the homepage, which makes authoring the banner much easier (particularly when it uses components that are common to the whole app).

one of the use cases I'd most like to support is load shedding. If your servers are overwhelmed then you can change an environment variable to have your clients render the page instead of your servers for a period of time to give you time to figure out your server utilization

Why does that need to be a resolve option, rather than this?

// src/lib/config.js
import { env } from '$env/dynamic/public';

export const ssr = !env.PUBLIC_DISABLE_SSR;
// +page.js
export { ssr } from '$lib/config.js';

The argument against allowing layouts to set ssr is that the semantics get super fuzzy, for me at least. I see it as a flag that signals whether something can be SSR'd (and that's how we treat it when it's a resolve option), so to have a layout opt out of SSR only for a page to opt back in to SSR feels nonsensical. At the very least, it's ambiguous. Having it be a page-only option eliminates the ambiguity, and gives us the option to change the behaviour in future.

@Rich-Harris
Copy link
Member

Do we want to allow configuring page options in +page.server.js as well as +page.js? In the case of router it would mean including the option in the server data payload, but that's easy to do. In all others, the information is used on the server rather than in the browser, so it makes sense to support it there.

@benmccann
Copy link
Member

// +page.js
export { ssr } from '$lib/config.js';

is quite painful because it needs to be added to every individual page in your application and you'd need a +page.js for every page. That being said, if we were to allow it in +layout.js I would be mostly fine with that solution. I think it'd be perfectly cromulent to say that (main)/+layout.svelte is set based on an environment variable and (main)/admin/+layout.svelte is set always to false. You would lose the ability to do something like say that it's based on an environment variable, but always true for Google, but that's perhaps a niche enough usecase that I can live without that.

dummdidumm and others added 3 commits August 24, 2022 15:47
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
@dummdidumm
Copy link
Member Author

dummdidumm commented Aug 24, 2022

The ssr option can be set in layouts - which, as stated in the other PR, got me thinking, why we don't allow this for all options.

Re configuring them in page.server.js -> I'm ok with this, AFAIK this already is supported for prerender.

-> all options should be configurable in +page/layout.js and +page/layout.server.js

@Rich-Harris
Copy link
Member

The ssr option can be set in layouts

only because of this PR! I'm saying that I'm not totally convinced that's a good idea, as it leads to ambiguity. At the very least we need to talk about the semantics because I don't think 'child beats parent' makes sense for ssr (even though it probably would make sense for other options)

@Rich-Harris
Copy link
Member

You would lose the ability to do something like say that it's based on an environment variable, but always true for Google

I'm definitely okay with that — Google agree it's a bad idea

@dummdidumm
Copy link
Member Author

The ssr option can be set in layouts

only because of this PR! I'm saying that I'm not totally convinced that's a good idea, as it leads to ambiguity. At the very least we need to talk about the semantics because I don't think 'child beats parent' makes sense for ssr (even though it probably would make sense for other options)

I definitely want this to be possible to set in layouts as well. The main use case I see for this is to disable SSR for sections of your app (like the admin section), not for single pages. So what would be better semantics then? "As soon as it's explicitly disabled it cannot be enabled again, because if you disable it in a layout it means you express that the layout already could have client-only code"? I don't know, this feels more complicated to explain and you lose out on some options when you "know what you are doing". "Last one wins" has my vote because of this.

@Rich-Harris
Copy link
Member

'Last one wins' is definitely the option that's easiest to apply consistently across page options, so as long as we're not worried about a layout declaring ssr = false because it uses a browser-only component only to blow up because of a misbehaving child, then I'm okay with that. Especially if layout-level options are the price of removing the resolve option.

If we do implement layout-level options then we could presumably eliminate config.kit.browser and config.kit.prerender.enabled in favour of setting them in the root layout, which feels like a nice win. Should we try and do that (and .server.js options) in this PR, or separately? Advantage of doing it here would be that we don't need to add temporary complexity to the docs

@dummdidumm
Copy link
Member Author

dummdidumm commented Aug 24, 2022

Agree that we should do it as part of this PR then.

If we do remove those options, do people have all possibilities still to enable/disable it? In other words, do they have access to globals/variables they cannot access in their root layout?
There's also the drawback that some people might never need a root layout and they are forced to create one then - is that a big "ugh" or just a small "meh"?

@Rich-Harris
Copy link
Member

If we do remove those options, do people have all possibilities still to enable/disable it?

Apart from controlling ssr per-request, yeah, I think that covers everything

some people might never need a root layout and they are forced to create one then

I'm not too worried about this — I'd expect almost all apps to need one eventually, so I'd file it under 'meh' personally

@Rich-Harris Rich-Harris merged commit cafdf84 into master Aug 30, 2022
@Rich-Harris Rich-Harris deleted the ssr-option branch August 30, 2022 19:15
@Rich-Harris
Copy link
Member

We'll need to update the migration guide here after we implement #6436

holdenrehg added a commit to holdenrehg/portfolio that referenced this pull request Sep 3, 2022
Setting was deprecated. See error:

Error: config.kit.prerender.default has been removed. You can set it inside the top level +layout.js instead. See the PR for more information: sveltejs/kit#6197
holdenrehg added a commit to holdenrehg/portfolio that referenced this pull request Sep 3, 2022
The default option in svelte configuration is deprecated and will be
removed eventually.

Error: config.kit.prerender.default has been removed. You can set it inside the top level +layout.js instead. See the PR for more information: sveltejs/kit#6197
MDr164 added a commit to MDr164/u-bmc_ink that referenced this pull request Sep 11, 2022
As seen here sveltejs/kit#6197
the prerender option has been removed and compilation
on never sveltekit versions would fail.

Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
MDr164 added a commit to MDr164/u-bmc_ink that referenced this pull request Sep 11, 2022
As seen here sveltejs/kit#6197
the prerender option has been removed and compilation
on never sveltekit versions would fail.

Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
MDr164 added a commit to u-bmc/ink that referenced this pull request Sep 11, 2022
As seen here sveltejs/kit#6197
the prerender option has been removed and compilation
on never sveltekit versions would fail.

Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
remogatto pushed a commit to remogatto/kratos-selfservice-svelte-node that referenced this pull request Sep 19, 2022
saladbowl77 added a commit to saladbowl77/blog-v2 that referenced this pull request Oct 22, 2022
RangHo added a commit to RangHo/rangho.me that referenced this pull request Dec 1, 2022
This is due to a breaking change in SvelteKit.
See PR sveltejs/kit#6197
@sifferhans sifferhans mentioned this pull request Dec 16, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prerendering overhaul
3 participants