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] add an ssr parameter to resolve for better skipping of SSR #2804

Merged
merged 15 commits into from
Jan 11, 2022

Conversation

JeanJPNM
Copy link
Contributor

@JeanJPNM JeanJPNM commented Nov 15, 2021

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

Closes #1650 by allowing users to pass a second argument to resolve inside the handle hook. This argument currently contains an option that tells wether or not the page should be loaded on the server.

Props to @Rich-Harris for coming up with this solution.

Benefits:

  • skips loading components on the server that aren't rendered there (i.e. truer SPA mode)
  • allows mode advanced control over rendering. E.g. dynamic rendering (i.e. only do SSR for Googlebot)

@changeset-bot
Copy link

changeset-bot bot commented Nov 15, 2021

🦋 Changeset detected

Latest commit: 7f11e91

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

@JeanJPNM
Copy link
Contributor Author

The names could be a lot better, so I'm open to sugestions.

@JeanJPNM JeanJPNM mentioned this pull request Nov 15, 2021
4 tasks
@dummdidumm
Copy link
Member

One thing I'm not totally sure about: If we introduce this new API, what do we do with the export const ssr = ... inside pages? I feel like having both unnecessary bloats the API surface, on the other hand there might be cases where it's easier to define this on the page itself rather than inside handle .

@JeanJPNM
Copy link
Contributor Author

The traditional API is still very useful, but the new one is needed because the page needs to be loaded on the server. I decided to name if allowSSR so it doesn't get confused with ssr, because they server similar purposes, but have different use cases.

@benmccann
Copy link
Member

what do we do with the export const ssr = ... inside pages?

My idea would be to let ssr be either a boolean or function(opts) where opts provides the SSRNode. Then we remove config.ssr and the page-level SSR option, but the user can easily implement the page-level option themselves. If the user passes a boolean computed off the request info then we can skip loading the page. If they pass a function then we will load the page and the user can return something like opts.page.ssr ?? true to read the value from the page just as they can do today

I'd also rename it from allowSSR to ssr. I prefer the name ssr and if you make the other changes I suggested then it no longer makes sense to have allow as part of the name

@benmccann
Copy link
Member

@Rich-Harris said he preferred removing export const ssr = false and config.ssr to allowing it to be a function as I had proposed. I would be fine with that solution as well, so let's go with that

I did a code search across GitHub and it was really only being used to deal with things like embedding an editor where I think onMount would be a better solution - if you have a single component that can't be SSR'd your first could should be to just do CSR for that one and not kill SSR for the entire page

@JeanJPNM
Copy link
Contributor Author

Honestly I just want to have a way to deal with side effect browser code, if I had time I would just make an RFC for something like plugins for the framework, so that I can write my own page config loader and get rid of the problem.

@benmccann
Copy link
Member

Don't you already have that with onMount?

@JeanJPNM
Copy link
Contributor Author

No, I mean the function that loads a page's local configuration (ssr, hydrate, router and prerender).

@benmccann
Copy link
Member

A good point raised in the maintainer's channel is that routes don't always map to urls cleanly and so it might be hard for people to migrate to this new option in some cases if we don't support function values

@JeanJPNM
Copy link
Contributor Author

JeanJPNM commented Nov 20, 2021

This pr is not intended to replace nor modify the existing methods to determine the configuration of a page, that would only be a side effect of its main purpose, get rid of annoying errors caused by side effect imports.

A good example of this would be the rxfire library. Usually you can import it and use it along firebase to create stores local to the component like this:

<script>
  import { collectionData } from "rxfire/firestore";
  import { collection, where, query } from "firebase/firestore";
  import { firestore } from "$lib/firebase";
  const citiesRef = query(
    collection(firestore, 'cities'),
    where('habitants', '>=', 10000)
  );
  const cities = collectionData(citiesRef, { idField: 'id' });
</script>

In this case it is not possible to lazy load any of the libraries because the stores need to be initialized with the component, working around this issue with onMount requires creating mock stores used while the firebase and rx modules load and also requires adding checks on every single place each store is used to ensure that the stores are not the temporary ones.

@vercel
Copy link

vercel bot commented Jan 9, 2022

@benmccann is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@netlify
Copy link

netlify bot commented Jan 9, 2022

❌ Deploy Preview for kit-demo failed.

🔨 Explore the source changes: f6714ae

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61db6c97a39feb0007a86fc7

@Rich-Harris
Copy link
Member

finally! thanks for everyone's hard work on this 🍻

@DhyeyMoliya
Copy link

Guys, this seems a very harsh decision to remove config and page level ssr now we’ll need to add a hook for just disabling ssr. Or if we want to skip ssr for 50 out of 100 pages we need to implement custom logic to identify the routes.

I may be talking out of place here. As all core members have agreed on this. Apologise for that.

@kotx
Copy link

kotx commented Jan 11, 2022

I also don't like the removal of this option- if element-level hydration becomes a thing for SvelteKit in the future, it would be easier to export const ssr = false per-component instead of writing a global function to filter all components (which I gather is what's happening here per-page). I think there should be an easier way to declare that pages/components should not be rendered on the server. Not all client-side-only code is bad!

@Rich-Harris
Copy link
Member

The previous option caused a great deal of confusion, because the page still had to be imported in order to determine whether or not SSR was necessary. Given that, in 90% of cases, the desire to disable SSR selectively was driven by the fact that pages had dependencies that couldn't be imported, the option was near-useless. Having both ssr: false in handle and export const ssr = false in pages, but with those subtly different semantics, would have compounded that confusion.

It wouldn't be good design to preserve the option on the offchance that it would still be relevant if Svelte gained an as-yet hypothetical feature.

@kotx
Copy link

kotx commented Jan 11, 2022

That makes sense. However, is there any other way to disable SSR per-page without a global handler? To me, it doesn't seem like a good design pattern to handle something that is meant to be page-level using a global function. For example Remix uses an optional file-naming scheme like .client.tsx, .server.tsx, which is file-level.
Though, I fully understand if this has been decided to be the only method to be used for page-level config, I just believe something better could potentially be added.

@kamholz
Copy link

kamholz commented Jan 11, 2022

For what it's worth, I thought it would be annoying to update my code to incorporate this change but instead it turned out to be the opposite. I was able to remove the ugly dynamic imports I previously had to put in onMount -- makes a lot of sense why this change was done. I can see how it would be annoying if a lot of different URL patterns matched but that wasn't an issue in my case.

@brgrz
Copy link

brgrz commented Jan 11, 2022

Idk but this "solution" feels like it's moving SvelteKit one step further towards being serverside-only now that turning off SSR was made more complex of an endeavour than it should be. We've been hearing from @Rich-Harris that SPA is bad for months now. dc283c0

Not all SPA is bad though. As devs we know when it makes sense for our apps and when it doesn't. What's the next step? Removal of static adapter?

@kamholz
Copy link

kamholz commented Jan 11, 2022

In my case it was nothing to do with SPA. I needed to import Leaflet and that can only be done in the browser. It was significantly more annoying to import libraries like that prior to this change.

@Conduitry
Copy link
Member

What's the next step? Removal of static adapter?

Astoundingly, no.

@dummdidumm
Copy link
Member

dummdidumm commented Jan 11, 2022

Idk but this "solution" feels like it's moving SvelteKit one step further towards being serverside-only now that turning off SSR was made more complex of an endeavour than it should be. We've been hearing from @Rich-Harris that SPA is bad for months now. dc283c0

Could you explain in more detail why in your case it's become more complex? As Rich pointed out above, the end result should be simpler now. Previously if you had a client-only-library that you imported you had to do this:

<script context="module">
  export let ssr = false;
  // ..
</script>

<script>
  import { onMount } from 'svelte';
  let lib;
  onMount(() => {
    import('lib-from-somewhere').then(module => lib = module);
  });
  // ...
</script>

With this change you can now do

<script>
  import lib from 'lib-from-somewhere';
  // ...
</script>

which is much simpler compared to the previous version. I agree though that writing the handle hook might become cumbersome, it could be as simple as

export async function handle({ request, resolve }) {
	const response = await resolve(request, {
		ssr: !request.path.startsWith('/admin')
	});
	return response;
}

If you are developing an SPA (you want nothing to be SSR'd because it's an internal business app for example) then it's even as simple as

export async function handle({ request, resolve }) {
	const response = await resolve(request, {
		ssr: false
	});
	return response;
}

These few lines of code finally enable you to build true SPAs without having to do dynamic imports workarounds. If there are many different single pages that you want to disable SSR for, determining whether or not ssr should be true can become more complex, that is true. It's a trade-off due to technical limitations of how JavaScript works.

@benmccann
Copy link
Member

if we want to skip ssr for 50 out of 100 pages we need to implement custom logic to identify the routes.

We had considered this, but it seemed extremely unlikely that you'd randomly disable a random half of pages. If you wanted to disable a lot most likely they would be under a shared path like /admin in which case the API in this PR is far easier to use than the previous API. If you have a use case where you're really disabling SSR for 50 out of a 100 pages that don't share a path prefix, please share it.

Idk but this "solution" feels like it's moving SvelteKit one step further towards being serverside-only now that turning off SSR was made more complex of an endeavour than it should be. We've been hearing that SPA is bad for months now. dc283c0
Not all SPA is bad though. As devs we know when it makes sense for our apps and when it doesn't. What's the next step? Removal of static adapter?

The primary motivation for this PR was to better support SPA mode as requested in #1650, which is quite the opposite of what you're suggesting. adapter-static is not going anywhere 😆

@kotx
Copy link

kotx commented Jan 11, 2022

I think the main problem is that we're putting a handler in a different location from the rest of the code. Developers might expect the entire configuration of a page to be done within it, without having to search for a global option. It seems like it should be scoped and/or abstracted away to be page-level.

@benmccann
Copy link
Member

I'm not sure there's a case where I'd suggest disabling just a specific page(s) though. It's generally better to use browser and onMount in those cases. Is there a use case for that?

@kotx
Copy link

kotx commented Jan 11, 2022

Honestly, if there is I haven't come across it. If browser and onMount are recommended, can we clarify it in the docs more? Particularly in the SSR section. I'd be okay with opening a PR with that if it's okay.

giokara-oqton added a commit to giokara/telraam-bloemekeswijk that referenced this pull request Jan 16, 2022
@rob-balfre
Copy link

I upgraded from v215 to v232 today and was pretty confused with the SSR change. I'm developing a 'business application behind a login' (as the docs describe!) so SSR isn't needed/wanted.

If (like me) this had you stumped then do this...

REMOVE ssr: false from svelte.config.js

ADD this to a new file called src/hooks.js. (thanks @dummdidumm)

export async function handle({ request, resolve }) {
	const response = await resolve(request, {
		ssr: false,
	});
	return response;
}

Docs for hooks @ https://kit.svelte.dev/docs#hooks

❤️ kit

@samuelstroschein
Copy link
Contributor

I upgraded from v215 to v232 today and was pretty confused with the SSR change. I'm developing a 'business application behind a login' (as the docs describe!) so SSR isn't needed/wanted.

I upgraded from v215 to v232 today and was pretty confused with the SSR change. I'm developing a 'business application behind a login' (as the docs describe!) so SSR isn't needed/wanted.

If (like me) this had you stumped then do this...

REMOVE ssr: false from svelte.config.js

ADD this to a new file called src/hooks.js. (thanks @dummdidumm)

export async function handle({ request, resolve }) {
	const response = await resolve(request, {
		ssr: false,
	});
	return response;
}

Docs for hooks @ https://kit.svelte.dev/docs#hooks

❤️ kit

Another breaking change got introduced with #3384 which breaks the solution above. request got replaced by event.

Solution that works with 1.0.0-next.241:

export async function handle({ event, resolve }) {
	const response = await resolve(event, {
		ssr: false,
	});
	return response;
}

@DoisKoh
Copy link

DoisKoh commented Mar 30, 2022

I don't know what a good solution is but I'd like to chime in saying that this affects me as well (some libraries can only be imported on the browser) and the current solution doesn't seem ideal. In terms of maintainability, now you have to go to hooks to see if it has SSR. Ideally, I'd like to be able to write a component and if it does require SSR to be turned off, it can disable SSR in pages that use it.

Again, I don't know what the solution is/should be, but just adding my 2 cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tru-er SPA mode