sveltejs / kit

web development, streamlined
https://svelte.dev/docs/kit
MIT License
18.76k stars 1.95k forks source link

SSR rendered page retains old store value in dev mode #8614

Closed konstantinblaesi closed 1 year ago

konstantinblaesi commented 1 year ago

Describe the bug

Steps:

  1. Start dev server
  2. Initialize and export a store with value 0
  3. Import the store in a page
  4. Set store value in the page to 1
  5. Open the page for the first time
  6. As expected the page renders store value 1, not 0
  7. Deactivate/remove code from step 4
  8. Refresh the page
  9. Page now renders store value 1 (shortly, SSR cached?) which is then hydrated/updated client side to value 0

Workaround:

Restart the dev server, the SSR rendered page no longer has value 1 but value 0

Question:

Is this intended/expected behavior, is it configurable or a bug in vite or svelte?

Reproduction

https://github.com/konstantinblaesi/store-issue-repro

Logs

No response

System Info

System:
    OS: Linux 6.1 Fedora Linux 37 (Workstation Edition)
    CPU: (8) x64 Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
    Memory: 15.16 GB / 31.03 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 18.7.0 - /usr/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.18.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 109.0.5414.74
    Firefox: 108.0.1
  npmPackages:
    @sveltejs/adapter-auto: ^1.0.0 => 1.0.2 
    @sveltejs/kit: ^1.0.0 => 1.1.4 
    svelte: ^3.54.0 => 3.55.1 
    vite: ^4.0.0 => 4.0.4

Severity

annoyance

Additional Information

Original discord discussion

Rich-Harris commented 1 year ago

This is expected behaviour. Your store is created once on the server, when it starts up; if two users load pages that reference that store, they will both read the same value. In the browser, the store is created again when the app boots up, but with the initial value.

It's not a SvelteKit thing so much as an inevitable consequence of the fact that servers serve multiple users. If you want a store that is only used by a single user, your best bet is to return it from a universal load function:

// src/routes/+layout.js
export function load() {
  return {
    value: writable(0)
  };
}
<!-- inside a component -->
<script>
  import { page } from '$app/stores';

  $: ({ value } = $page.data); //  or `$: value = $page.data.value`, if you prefer
</script>

<h1>value is {$value}</h1>

For good measure, define App.PageData so that you get type safety:

// src/app.d.ts
import { Writable } from 'svelte/store';

declare global {
  namespace App {
    interface PageData {
      value: Writable<number>;
    }
  }
}

export {};

Will leave this open with a documentation label.

Conduitry commented 1 year ago

Would this go under https://kit.svelte.dev/docs/load#shared-state ? Is there anything else there we need to say, or do we just need to make it stand out more?

ghostdevv commented 1 year ago

Stuff like this makes it far too easy to create security holes, ideally the docs should have it in big flashing lights :rofl:

geoffrich commented 1 year ago

I think with global stores being such a common pattern in vanilla Svelte (even being used in the tutorial extensively) we need to make this behavior (and suggested solutions) more prominent -- it's a real paradigm shift for people. I bet many people who read that docs section don't realize that "shared variables outside your load function" includes stores.

Not sure if https://github.com/sveltejs/kit/pull/8547 intersects with this at all.

Rich-Harris commented 1 year ago

I'm trying to write some docs for this now, but I'm struggling to come up with a plausible use case — I can think of examples of sensitive non-store information that would be returned from a load (username etc), and examples of stores that you'd set outside the context of a load (albeit only in the browser, e.g. a volume setting for media elements) but nothing that falls into both categories. What's an example that people have come upon in the wild?

braebo commented 1 year ago

I think the count example is actually a good one because it's usually the first store everyone makes when learning, and it's ambiguous enough to represent "any store with global state".

braebo commented 1 year ago

Considering that --

I imagine this is going to be a widespread and dangerous footgun for many users. I've been using Svelte daily for years and I still forget about this sometimes (albiet my habits likely contribute to my forgetfulness).

Are we certain that documentation warning people is the best/only solution? Just spitballing some (mostly unrealistic) thoughts I've had:

fzembow commented 1 year ago

Chiming in here, I have shot myself in the feet with this on a current project

come up with a plausible use case

In my use case, I have a glorified todo list app. The current user's particular todos are loaded into a store so they can be modified post-load. I realize that the store should be initialized in the load() now, but previously I was just loading the data itself in load() and then passing it to a global store's initializer and then using it directly in a bunch of child components... because I wasn't aware of the behavior.

I too am a bit worried about accidentally re-introducing other shared stores accidentally in the future though.

I have a hard time even thinking of a good use case for having a globally shared server-side store... the power of stores IMO is their connection to the svelte components, reactivity, etc, which isn't the case in SSR. If I wanted to notify something server side of changes to a value I could just use observable / EventEmitter / queues etc ... and that feels like a less common need than "ship some reactive data to the frontend after rendering it on the backend for a request"

Tommertom commented 1 year ago

I'd say the OP's case is an excellent example on how HMR works and should work - but maybe I am overlooking somnething given the comments by others..

hshoob commented 1 year ago

I'm trying to write some docs for this now, but I'm struggling to come up with a plausible use case — I can think of examples of sensitive non-store information that would be returned from a load (username etc), and examples of stores that you'd set outside the context of a load (albeit only in the browser, e.g. a volume setting for media elements) but nothing that falls into both categories. What's an example that people have come upon in the wild?

FYI. This is what I was dealing with originally.

https://user-images.githubusercontent.com/18516326/213664713-7d4d7e4f-42db-40fb-967c-6aabe4a239b2.mov

Rich-Harris commented 1 year ago

Thanks for the examples @fzembow and @hshoob. This is what I suspected — the problem isn't the stores, it's the way they're being used. (This isn't a criticism! We need to find a way to make what I'm about to say more obvious.)

Your load functions should be pure — no side-effects. That means no setting store values, no writing to databases, no modifying any variables outside the load function. You can bend the rules for the sake of observability (e.g. console.log(stuff) or updating a last_viewed field in a database are harmless), but beyond that it's a recipe for confusion. Does the user object need to be a store, or could it be accessed via $page.data.user instead?

The same goes for the top level of a component's <script> block — writing to $strokeWidth is a side-effect that has impact beyond the scope of the component itself (a store that doesn't belong to the component has a value that depends on whether or not the component happens to be rendered which is a bad situation to be in).

A page I link to a lot is CommandQuerySeparation. In essence, the idea is that a function should return something or it should do something, but never both. That very much applies here. (If I were designing a language, I'd enforce this at the syntax level!) There are occasional exceptions (in the linked page, Fowler mentions popping a stack; logging is another obvious example) but they are exactly that — exceptions.

So while there are legitimate reasons to return a store from a load (e.g. realtime subscriptions, where something like stock price data can change without a navigation), I don't know that documenting this pattern as a way of dealing with global state is the right approach. It feels like we need to do a better job of communicating that your load and <script> blocks shouldn't have side-effects. Global stores are totally fine, as long as you don't write to them during data loading or render.

Rich-Harris commented 1 year ago

One possibility that springs to mind, inspired by @FractalHQ's suggestions — perhaps we could alias svelte/store to a wrapper module that does something like this:

import { writable, readable, derived, get } from 'svelte/store'; // not sure how to prevent this from being circular
import { BROWSER } from 'esm-env';

function safe_writable(value, start) {
  const store = writable(value, start);
  if (!BROWSER) {
    store.set = store.update = () => {
      throw new Error('Cannot write to a store on the server');
    };
  }
  return store;
}

export { readable, derived, get, safe_writable as writable };
Rich-Harris commented 1 year ago

@dummdidumm pointed out that that's not really a workable solution, since valid things like this would be prevented:

<script>
  import { spring } from 'svelte/motion';

  export let count = 0;

  const value = spring();
  $: $value = count;
</script>

Unless we tried something sneakier, like allowing a .set(value) if the current value is undefined, or allowing the first .set(...) but not the second?

Rich-Harris commented 1 year ago

wait... this is easy. we just need to differentiate between stores that were created during render and those that weren't:

import { writable, readable, derived, get } from 'svelte/store'; // not sure how to prevent this from being circular
import { BROWSER } from 'esm-env';

function safe_writable(value, start) {
  const store = writable(value, start);
+  if (!BROWSER && !IS_LOADING_OR_RENDERING) {
    store.set = store.update = () => {
      throw new Error('Cannot write to a store on the server');
    };
  }
  return store;
}

export { readable, derived, get, safe_writable as writable };
geoffrich commented 1 year ago

Won't that also throw for your spring example above, since it's calling set during rendering in the reactive statement? AFAIK we would still want that to be allowed since the store isn't global, it's created in the component.

ghostdevv commented 1 year ago

Could you check for a component context on initialisation?

Rich-Harris commented 1 year ago

Won't that also throw for your spring example above, since it's calling set during rendering in the reactive statement?

In that case, IS_LOADING_OR_RENDERING would be true, so the monkey-patching wouldn't happen. Pseudo-code:

// src/runtime/server/page/load_data.js
IS_LOADING_OR_RENDERING = true;
const result = await node.universal.load.call(null, {...});
IS_LOADING_OR_RENDERING = false;
// src/runtime/server/page/render.js
IS_LOADING_OR_RENDERING = true;
rendered = options.root.render(props);
IS_LOADING_OR_RENDERING = false;

(Though now that I look at that code, I'm not sure how to deal with the possibility that an illegal set happens while an unrelated load is happening... though those cases would be extremely rare since this would only happen in dev, where the server only has one user to worry about. We really need AsyncLocalStorage to get standardised.)

Checking for a component context would work for the rendering bit, yeah, just not the loading bit.

fzembow commented 1 year ago

On some reflection, I think I came to a conclusion as to why I keep getting tripped up on this, even though rationally I totally understand the current implementation's behavior. I think it's somewhat due to my mental positioning and primary usage of svelte as a "powerful prototyping tool": I usually start with a frontend-only perspective, with all state lost on a reload, and only later add meaningful server-side persistence to a project.

With this progression, it's really tempting to just take some existing global store and just stick the result of a load into it -- somewhat of a path-dependent developer experience :)

anxpara commented 1 year ago

"If you want a store that is only used by a single user"

Does a store "being used by a single user" equate to a store that's client-side only?

Separately, i like the idea of making another type of store with a more specific/descriptive name.

Installing 3rd party store libraries to change the behavior of stores i had already made has been a very nice experience. I feel like some of those store types (e.g. local-storage-based and cookie-based) should be built in to sveltekit

nickyhajal commented 1 year ago

Before using SvelteKit, I would have a store called me that made the logged-in user's information available throughout the client-side app.

Now using SvelteKit, my instinct was to do the same so I have this:

// layout.server.ts
import { trpc } from '../lib/trpc/client'
import type { LayoutServerLoad } from './$types'

export const load = (async (req) => {
  const me = trpc().me.get.query()
  return {
    me
  }
}) satisfies LayoutServerLoad
// layout.svelte
<script lang="ts">
  import type { LayoutData } from './$types'
  import {  me } from '$lib/core/stores'

  export let data: LayoutData

  if (data.me) {
    me.set(data.me)
  }
</script>

<div>
  <slot />
</div>

That seems to be creating a global store with the last logged in user's information which is definitely not what I intended!

Does that mean that for any data that I want server-side rendered, I can't have it in a store because it will end up as a server global?

Rich-Harris commented 1 year ago

Closing as we now have documentation which explains how to do this stuff safely

Hubro commented 1 year ago

@Rich-Harris I don't feel like the documentation is quite approachable enough for noobs like me. It's not clear to me how to convert SPA patterns into equivalent SSR patterns. For example, I like to create stores like this:

const messageStore = writable(await loadMessages());

export default {
  subscribe: messageStore.subscribe,

  postMessage: (tag: string, text: string) => {
    // Code for posting a new message with optimistic updates
  },
};

This feels nice and elegant for an SPA, but of course once I enable SSR, loadMessages() is run on the server and cached for all users, which is terrible.

What's the best alternative pattern for achieving the same result with SSR? My first instinct would be to load the data in load and inject it into the store, but the documentation explicitly says to not do that: https://kit.svelte.dev/docs/state-management#no-side-effects-in-load

I want my data in a store so I can subscribe to it and attach related functions to it, but the documentation doesn't mention how to do that.

coryvirok commented 1 year ago

FWIW, I usually follow the docs that you linked to load the data in +page.ts, pass it to the template via page.data, then initialize the store in the template and store it in the context.

// +page.js
export async function load({ fetch }) {
    const response = await fetch('/api/user');

    return {
        user: await response.json()
    }
}
// +page.svelte

<script>
  export let data
  const store = writable(data.user)
  setContext('user', store)
</script>

Or if you need this in all sibling routes, move the load() to +layout.js and the setContext() to +layout.svelte.

Hubro commented 1 year ago

@coryvirok Got it, that makes sense, thanks! Sounds like load() in +layout.svelte plus setContext() is the SSR-compatible equivalent for global stores.

But I am a little surprised that the canonical way to load data into a store is so roundabout. Feels like there's a lot of room for an improved developer experience here.