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

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

Closed
konstantinblaesi opened this issue Jan 19, 2023 · 24 comments
Closed

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

konstantinblaesi opened this issue Jan 19, 2023 · 24 comments
Labels
documentation Improvements or additions to documentation ready to implement please submit PRs for these issues!
Milestone

Comments

@konstantinblaesi
Copy link

konstantinblaesi commented Jan 19, 2023

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
Copy link
Member

Rich-Harris commented Jan 20, 2023

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.

@Rich-Harris Rich-Harris added the documentation Improvements or additions to documentation label Jan 20, 2023
@Conduitry
Copy link
Member

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
Copy link
Member

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

@geoffrich
Copy link
Member

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 #8547 intersects with this at all.

@Rich-Harris
Copy link
Member

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
Copy link
Member

braebo commented Jan 20, 2023

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
Copy link
Member

braebo commented Jan 20, 2023

Considering that --

  • stores are one of Svelte's most popular features
  • Sveltekit is the recommended way to use Svelte
  • SSR is used by default

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:

  • Some sort of compiler / buildstep / vite-plugin that can align behavior with the average user's expectations somehow.
  • A new store type.
  • Warnings that fire when a global store is mutated on the server explaining that the state will persist across user sessions.
  • Prominent warning labels on the documentation / tutorial for the writable store.

@fzembow
Copy link

fzembow commented Jan 20, 2023

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
Copy link

Tommertom commented Jan 20, 2023

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..

@dummdidumm dummdidumm mentioned this issue Jan 20, 2023
5 tasks
@hshoob
Copy link

hshoob commented Jan 20, 2023

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.

Screen.Recording.2023-01-19.at.5.35.39.PM.mov

@Rich-Harris
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Could you check for a component context on initialisation?

@Rich-Harris
Copy link
Member

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
Copy link

fzembow commented Jan 21, 2023

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 :)

@Rich-Harris Rich-Harris added the ready to implement please submit PRs for these issues! label Jan 27, 2023
@Rich-Harris Rich-Harris added this to the soon milestone Jan 27, 2023
@anxpara
Copy link

anxpara commented Feb 7, 2023

"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
Copy link

nickyhajal commented Feb 23, 2023

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
Copy link
Member

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

@Hubro
Copy link

Hubro commented Jun 29, 2023

@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
Copy link
Contributor

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
Copy link

Hubro commented Jun 29, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ready to implement please submit PRs for these issues!
Projects
None yet
Development

Successfully merging a pull request may close this issue.