fix: use runes in when detecting Svelte 5

[dummdidumm]

• fixes afterUpdate in the root sveltekit component is not called after a failing action #11027 (not the underlying issue, but the problem within SvelteKit)
• fixes Svelte 5: Forcing runes mode causes issues with generated code. #11022

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. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: 931f522

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

[Rich-Harris]

looking forward to replacing the stores with state-based alternatives so that this dance becomes unnecessary

[benmccann]

Why is this necessary? I don't see anything in the Svelte 4 version that's listed on the breaking changes page: https://svelte-5-preview.vercel.app/docs/breaking-changes. If I didn't miss it, then maybe there's something we need to fix in our compatibility or list on that page?

[Rich-Harris]

actually @benmccann is right, this should work. i'm rescinding my approval

@trueadm this is what i was talking about before when i was saying afterUpdate shouldn't be bound to render effects, it should be bound to get

[paoloricciuti]

actually @benmccann is right, this should work. i'm rescinding my approval

@trueadm this is what i was talking about before when i was saying afterUpdate shouldn't be bound to render effects, it should be bound to get

I also opened an issue in the svelte repo with a much more minimal reproduction sveltejs/svelte#9420

[dummdidumm]

actually @benmccann is right, this should work. i'm rescinding my approval

What exactly should work? Are you saying that you also expect $effect.pre to run in SSR? Because we're in agreement then, I also think that should be the behavior 😄

[trueadm]

@Rich-Harris The issue with all of this (we currently do afterUpdate on writes) is that the signal you're reading/writing to has only a component context of where it was created. So if you create a signal like this outside of a component or in another component context, non of this will work. This is what I meant by the some of these problems are likely not to be possible to be fixed and thus we need to document these breaking changes.

[trueadm]

I also don't think $effect.pre should run in SSR. The compiler should simply compile $: logic into a block that always runs on the server. If we need an effect for the server, it should be $effect.server or something adequately named. It would also likely need a different scheduling/queue as you probably don't want the same mechanics involved as the current effects that are coupled to notion of render effects on the server.

[dummdidumm]

$: is already compiled into a block that always runs on the server. But $effect.pre isn't ,which is why we need the additional if-block now. And I'm questioning that decision. My thinking is that $effect.pre on the server turns into a one-time statement. running right before the render.

[trueadm]

I'd prefer:

$effect.once(fn)

This would be an effect that runs one on the server and once on init on the client. Which seems to handle this use-case?

[PuruVJ]

This is basically onMount, with added functionality of running on server. Could be handy, but necessary?

[dummdidumm]

It is not what's wanted here. What's wanted here is a function that runs once on the server and before every update on the client (basically $effect.pre)

[trueadm]

$effect.pre has a major downside here doing what you describe. If you run the logic on the server and it produces x = 5 but it runs twice on the client and produces x = 6 then you'll end up having different values before you come to entering the render effects that will hydrate the DOM – this will cause a mismatch and instantly cause hydration performance issues. This is why other frameworks are so strict to prevent this behavior from happening – it's a performance cliff like no other.

Maybe we can detect cascading updates in $effect.pre on the server and throw an error?

[benmccann]

I think my solution to that problem would instead be to detect in dev mode that the value on the client differs from the value on the server. Because it seems unusual to me that something would run twice on the client before hydration and once on the server and that seems like something to avoid

[trueadm]

@benmccann I mean the same logic would also run twice on the server if it could – but given we're saying that $effect.pre only runs once, then you get mismatches. I would just like us to understand the implications of having this and possibly causing mismatches as there's one thing detecting a mismatch in DEV vs enabling a developer to fix this problem easily. I still firmly believe we should be pushing people to the happy path.

The way other frameworks deal with this problem is to make it not possible to run effects on the server, and then making it clear there was a mismatch on the client – meaning there's a single place to look to fix it, the client code. In which case the fix is generally to make it an $effect rather than an $effect.pre – this goes to the useLayoutEffect vs useEffect case in React. Now another way of looking at that is saying the inverse – $effect.server only runs once on the server and you can't update state that affects UI in it, which is actually better than overloading $effect.pre.

This is important as I expect the majority of use-cases for $effect.pre to be reading the DOM before it's painted, so calculations around styling and scroll are available. These shouldn't ever run on the server, so it makes no sense to have it run there to begin with.

[benmccann]

This seems like something you'd do frequently with stores. I think another solution to this might be to replace the use of stores.page with runes? We could potentially provide a helper function which wraps this pattern to help people interact between the two types of reactivity

[trueadm]

@benmccann There was a suggestion to have a $kit rune a while back, this could just be something that lives on that rune.

[Rich-Harris]

Are you saying that you also expect $effect.pre to run in SSR?

No, absolutely not

and I'm not sure what $effect.once(fn) would mean other than fn()

[Rich-Harris]

The issue with all of this (we currently do afterUpdate on writes) is that the signal you're reading/writing to has only a component context of where it was created

Where the signal was created is irrelevant — what matters is where the signal is read. What I'm suggesting is basically this (pseudo-code):

function get(signal) {
  if (current_component) {
    signal.consumers.add(current_component);
  }

  // ...
}

[dummdidumm]

We should have the "how does afterUpdate work correctly" discussion in sveltejs/svelte#9420

Are you saying that you also expect $effect.pre to run in SSR?

No, absolutely not

I don't understand what changes you are requesting to this PR then. If anything, this fixes the afterUpdate bug in SvelteKit (by not using it anymore), without having to wait for a more general fix as part of sveltejs/svelte#9420

[trueadm]

The issue with all of this (we currently do afterUpdate on writes) is that the signal you're reading/writing to has only a component context of where it was created

Where the signal was created is irrelevant — what matters is where the signal is read. What I'm suggesting is basically this (pseudo-code):

function get(signal) {
  if (current_component) {
    signal.consumers.add(current_component);
  }

  // ...
}

I tried that and the issue is current_component is always null after the init phase, unless you're inside an effect. In which case, if you're inside an effect, then the existing logic of handling it for render effects can be expanded to be other effects too.

[Rich-Harris]

I don't understand what changes you are requesting to this PR then

What I'm saying is that we shouldn't need to remove the afterUpdate stuff. It should continue to work

[Rich-Harris]

I tried that and the issue is current_component is always null after the init phase, unless you're inside an effect. In which case, if you're inside an effect, then the existing logic of handling it for render effects can be expanded to be other effects too.

Yeah I'm not talking about what current_component is or isn't at present, or the details of the implementation. I'm just describing how we can make it work correctly at a high level

[dummdidumm]

But it's deprecated, and we're encouraging people to move away from it. It is counter-intuitive to not use the new tools we have in runes mode. And it's a quick-fix for the issue in the context of SvelteKit, and we can look at the underlying problem in sveltejs/svelte#9420, where - again - we should have the discussion about how to fix this.

[Rich-Harris]

The replacement code is just super nasty. I think it would be better to keep using afterUpdate until we can use state-based replacements for the stores (and keep the stores as a legacy interface to those replacements)

[dummdidumm]

It's not pretty, but it's a 1-for-1 replacement to get the behavior we want (we can probably remove components and constructors from that list). It's ok IMO, and allows us to fix this issue in SvelteKit. Depending on how we need to adjust afterUpdate it may also be more performant.

[benmccann]

I'm fine with this PR for the time being if we can file an issue to address or document the breaking change so that we don't forget to do so

[Rich-Harris]

ok — i think we should undo the afterUpdate change once we get it working though, unless we want to keep it to motivate us to build a state-based $page etc

[trueadm]

ok — i think we should undo the afterUpdate change once we get it working though, unless we want to keep it to motivate us to build a state-based $page etc

Yeah, let's do that. I think it makes most sense.

[Conduitry]

This changelog message appears to be missing a

[benmccann]

Someone made a related issue: sveltejs/svelte#9278