[fix] revert change to rendering options (#2008)

[Rich-Harris]

Unfortunately I don't think the changes introduced in #2008 are viable, and we need to go back to the drawing board.

Firstly, it doesn't appear to solve #1650. Even when just using false, SvelteKit still imports the module of a page, which blows up if the page imports a client-only dependency. And even if we deferred the import, we still can't use export const ssr to selectively turn off SSR, because we can't 'see' that export without importing the page.

More generally, we can't really have functions in the config that execute at runtime, because doing so presents only bad options:

1. the app has a dependency on svelte.config.js, which could depend on who-knows-what dependencies that shouldn't be included in prod
2. rather than using the actual function, we stringify it, breaking references to anything outside the function itself, which will cause confusion

Currently we're using the actual function in dev, but stringifying in the build. So something like this...

import { should_server_render } from './utils.js';

export default {
  kit: {
    ssr: ({ page }) => {
      return should_server_render(await page);
    }
  }
};

...will work in dev but fail in production. This is maximally confusing.

None of this is to deny that the issues the PR was aimed at are worth solving. Until we can come up with a more robust solution I think we should revert #2008 before people start using the new config options in their apps.

If we want to use a function to determine ssr on a per-request basis, the proper place to express that is probably in handle. We could make a case for doing the same for router and hydrate but I'm not sure there's as much of a reason to do so; export const hydrate = false seems fine.

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]

🦋 Changeset detected

Latest commit: b19c371

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

[benmccann]

Firstly, it doesn't appear to solve #1650.

Eeks! You're right. Mea culpa.

even if we deferred the import, we still can't use export const ssr to selectively turn off SSR, because we can't 'see' that export without importing the page.

That was not a goal of the original PR. Instead, the goal of the original PR was to allow you to turn off SSR full-stop by simply setting false. In that case the page wouldn't be imported. It was also desired that you could decide using the request, in which case you also wouldn't need the page imported. Only if you needed to use properties from the page would it then load the page, so you could still do quite a bit without loading the page.

Now of course this didn't go to plan. Where I screwed up was not realizing that JS promises started evaluating immediately and not when awaited. I really thought I had tested this better, but I guess that's a second lesson I learned today

the app has a dependency on svelte.config.js, which could depend on who-knows-what dependencies that shouldn't be included in prod

agree this is a terrible option (and I missed that we were doing it in dev - thanks for pointing that out)

rather than using the actual function, we stringify it, breaking references to anything outside the function itself, which will cause confusion

yeah, it certainly is a downside. I thought it was worth the tradeoff for the increased power. at the very least we'd need to document it, but it is a bit of a footgun potentially

I'd really like if there were a way to provide options at runtime. Right now they all have to be provided an built-time and get baked into the final app. But you could imagine someone wants their SRE to change an config value and restart the app. We shouldn't have to build a new version of the app for that. If you could provide config at runtime then we could provide functions, which would be really nice. It's something that the chart.js guys do and is by far my favorite part of that API because it lets us reject all types of feature requests by telling people they can just do it with the existing options. Another SvelteKit option was changed to a function in #2007 though that one's not an issue because it's an option that controls building and isn't a runtime thing. I think it was a really nice solution to the issue there as well

If we want to use a function to determine ssr on a per-request basis, the proper place to express that is probably in handle.

I'm not quite sure how handle would do that. Stick something on the request? I guess that could work, but I don't know if I love the idea of the request becoming a bag of options in the long-term. I guess we could almost make another hook. I don't want hooks to become just a loose jumble of options either, but maybe we could turn hooks.js into our runtime config and make it more structured by making it a json config object? I haven't thought through where the various bundling pieces happen and if that'd actually work

We could make a case for doing the same for router and hydrate but I'm not sure there's as much of a reason to do so; export const hydrate = false seems fine.

I don't know that we can simply ignore router and hydrate because if we don't set them then the module will be loaded to compute whether those options are on. I agree they won't be as heavily used though and I wonder if we even need the export const way of setting them or if there's a better way of setting those options (e.g. for router, people have asked for route guards, which might be a better solution)

[Rich-Harris]

But you could imagine someone wants their SRE to change an config value and restart the app.

That definitely seems unconventional! Regardless, the build currently contains stringified functions, so you would need to import the config file for that to work, which we agree is bad.

If it were expressed via handle, then you could change environment variables, restart the app, and the behaviour would update.

I'm not quite sure how handle would do that.

A couple of straw men:

export function handle({ request, resolve, shell }) {
  if (should_server_render(request)) {
    return resolve(request);
  }

  return shell();
}

export function handle({ request, resolve }) {
  return resolve(request, {
    ssr: should_server_render(request)
  });
}

I don't know that we can simply ignore router and hydrate because if we don't set them then the module will be loaded to compute whether those options are on.

If you're server-rendering, that's fine, since you're loading the code anyway. If you're not server-rendering, hydrate is meaningless (no hydrate means you're just looking at an empty shell), and you don't really need to know the value of router until you hydrate.

[benmccann]

Option 2 seems like the nicer choice to me and not a bad option

the build currently contains stringified functions, so you would need to import the config file for that to work

Could you not do process.env still? Anyway, probably doesn't matter if we're moving away from this

That definitely seems unconventional!

That happened frequently at Google and LinkedIn. Even for this option I could imagine it being used. E.g. if your Node servers are falling over from a spike in traffic, some SRE might turn off server rendering and just let the clients do it.

If you're not server-rendering, hydrate is meaningless (no hydrate means you're just looking at an empty shell)

I actually was doing ssr: false, hydrate: true in my Sapper app awhile back. The idea is that your data would all be loaded on the server and sent down with the initial page to save a roundtrip. Svelte would then render the whole app client side by hydrating the empty shell

[Rich-Harris]

I actually was doing ssr: false, hydrate: true in my Sapper app awhile back. The idea is that your data would all be loaded on the server and sent down with the initial page to save a roundtrip.

In SvelteKit, if we skip SSR then we're also skipping load, so that wouldn't happen here