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] allow ssr to be configurable in +page.js and +layout.js (enhanced version) #6207

Closed
wants to merge 3 commits into from

Conversation

dummdidumm
Copy link
Member

Superseedes #6197. This is the same PR as #6179 with the addition of lazy-loading the shared node, which means you can use browser-only globals in +page.js, too.

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: 6a7b8c5

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

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

I prefer the simpler version if I'm honest. This adds a bit more complexity than I think is warranted by the feature — if page.js references a browser-only global then we already can't import it to see if ssr is true or false, which means AFAICT this only helps when ssr is false in handle and the page needs to fetch some data from the server when it hydrates, which feels pretty niche.

Given that you can always work around including browser globals (worst case scenario, import() inside your load), it feels like overkill to me. #6197 by contrast is a thing of beauty! (though the inline comments apply there too — not sure why we'd treat ssr differently to the other page options?)

@@ -13,23 +11,27 @@ SvelteKit includes a [client-side router](/docs/appendix#routing) that intercept
In certain circumstances you might need to disable [client-side routing](/docs/appendix#routing) with the app-wide [`browser.router` config option](/docs/configuration#browser) or the page-level `router` export:

```js
/// file: +page.js/+page.server.js
/// file: +page.js/+page.js
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// file: +page.js/+page.js
/// file: +page.js

### hydrate

Ordinarily, SvelteKit [hydrates](/docs/appendix#hydration) your server-rendered HTML into an interactive page. Some pages don't require JavaScript at all — many blog posts and 'about' pages fall into this category. In these cases you can skip hydration when the app boots up with the app-wide [`browser.hydrate` config option](/docs/configuration#browser) or the page-level `hydrate` export:

```js
/// file: +page.js/+page.server.js
/// file: +page.js/+page.js
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// file: +page.js/+page.js
/// file: +page.js

export const ssr = false;
```

In contrast to the other options, you can set this option in both `+page.js` and `+layout.js`. `ssr` options in subsequent layouts or the page overwrite earlier options. You cannot set this option in `+page.server.js` or `+layout.server.js`. This option does not take effect if the `ssr` option [in the handle hook](/docs/hooks#handle) is evaluated to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for treating ssr differently? This feels a bit confusing to me. If we made it a page option like all the others then we could move the 'You can control this setting...' paragraph back to the top instead of repeating it

@Rich-Harris
Copy link
Member

To add to that thought: the whole value of export const ssr is that it gives us granular control over which pages can be SSR'd that we didn't have before. Telling users that the proper response to a +page.js that can't load on the server is to use the resolve option (instead of teaching them how to fix their +page.js) therefore defeats the object. The message it sends is 'getting SSR to work is hard, so we've made it easy for you to opt out' which shirks our responsibility to end users.

The resolve option was added because we didn't have a way to do this before. Now that we do, I think we could consider getting rid of the resolve option in favour of a config option for people who absolutely definitely need an SPA (not sure what it should be called — ideally it'd be a sibling to hydrate and router like it is inside +page.js, though config.kit.browser.ssr is obviously a non-starter). I know @benmccann wants to keep it in order to generate different responses for search engines (I think?) but that's a bad practice that is liable to get your site downranked.

But we can only realistically get rid of the resolve option if we don't treat it as a safety net for people who can't get their +page.js modules to load on the server.

@benmccann
Copy link
Member

There are a lot of reasons why being able to set the ssr flag dynamically is valuable. E.g. 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. This is something I've seen in practice being used to stop a production outage. Having it be a build-time option when it's not related to building would be really frustrating.

I don't really understand the argument that a resolve option lets people opt-out of SSR too easily. Wouldn't a global flag let them opt-out even more easily and on a much wider basis? I saw the page-level option being used as an opt-out far more than I've ever seen the resolve option being used.

If we're really concerned about the consistency we could make hydrate and client be resolve options as well, but really I think we should move more in the direction of getting rid of those options. Hydrate should go away longer-term after Svelte supports partial hydration. client makes sense on an app-level basis, but I've never seen anyone use it on a per-page basis and am not sure it needs to be a page option.

@dummdidumm
Copy link
Member Author

I'm ok with closing this PR for now and wait for an actual use case someone has where it doesn't work anymore - given the fact, that someone opened an issue right away that the ssr option wasn't working anymore, and we changed the component to be lazy-loaded, and didn't hear anything from there on, it should be ok.

On the handle option: Ben has given some good arguments for making ssr a runtime, option. In contrast to the other options, it's possible to change it at runtime, whereas you can't change hydrate on the fly, because it affects the Svelte compiler output. router I'm not sure about.

Regardless how we decide, we would still need to duplicate that option, because that "also applies to layouts" sentence is still true - except we change the logic so that other options can also be set in layouts and the same behavior applies (last one wins). I don't see a drawback for that and it would make things more consistent.

@Rich-Harris
Copy link
Member

I'll close this PR in favour of #6197 and reply to the above over there instead

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.

3 participants