fix: guard contents updated before the guard itself

[hmnd]

Fixes #16850, fixes #16775, fixes #16795, fixes #16982

#16631 introduced a bug that results in the effects within guards being evaluated before the guards themselves. I believe iterating the effects in reverse fixes the issue without any further regressions. An alternative approach could be to actually sort effects by depth before updating, but I suspect that would have a greater performance penalty.

Although all tests pass, sorry if I'm missing something obvious! I've never touched the Svelte internals until now :).

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
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
• If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: a8a5b70

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	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

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-svelte-16930-svelte.vercel.app/

[github-actions]

Playground

pnpm add https://pkg.pr.new/svelte@16930

[PatrickG]

It seems like this fixes the issue described here - but only for its first occurrence.
Clicking the button once does not log "one" anymore. Clicking it three times still logs "one".
repl with this PR

[dummdidumm]

Can you explain what the tests are supposed to test? Most (haven't checked all but the first 4 do) also pass on main, and some of them have if conditions that are never true.

I think we should reduce this only to the ones that pass with this PR but fail on main.

[hmnd]

Can you explain what the tests are supposed to test? Most (haven't checked all but the first 4 do) also pass on main, and some of them have if conditions that are never true.

I think we should reduce this only to the ones that pass with this PR but fail on main.

The tests I committed were falling (multi-nested was failing against that pr), but I didn't look too closely at the tests commit I cherry picked from the previous PR.

I'll clean those up and take a stab at @PatrickG's issue today.

[hmnd]

It seems like this fixes the issue described here - but only for its first occurrence. Clicking the button once does not log "one" anymore. Clicking it three times still logs "one". repl with this PR

It looks like sorting eager effects by depth does fix this issue, so that may be necessary after all. Perhaps more efficient to sort higher up the chain in mark_reactions... Best to sort when updating, because mark_reactions could be called multiple times for the same batch of effects.

[hmnd]

@dummdidumm all cleaned up. I've also switched to sorting the effects by depth to account for @PatrickG's repro in #16775 (see the guard-else-effect sample)

[hmnd]

It seems it still doesn't fix the main issue within #16775 though, hmm...

[PatrickG]

It doesn't seem like sorting the effects by depth fixes the issue. Clicking the button three times still logs "one" - same as with the simple reverse.

[hmnd]

@PatrickG yeah, I noticed... I'll look into it further tomorrow!

[hmnd]

@dummdidumm to be honest, as much as I'd love to fix this myself, I'm kind of at a loss on how to retain the changes in #16631 while fixing all the issues it has introduced. I'm sadly not knowledgeable enough on Svelte's internals to figure this out. I think trying to sort the effects here is just fixing a symptom downstream of the issue rather than the issue itself, as visible by the playgrounds in #16775.

But what I can tell is that #16631 introduced some really nasty side effects. It may be a good idea to either prioritize fixing it or revert it until a fix can be found. The couple of tests in this pr and the playground in #16775 are good ways to reproduce the bugs here.

If there's any other way I could assist figuring this out, do let me know!

[hmnd]

Never mind! I think I've got it. Eager effects were still executing after being destroyed, in addition to executing out of order.

[hmnd]

@PatrickG I believe I've now got the fix to your issue nailed down too. I've also ensured the guard-else-effect test based on your repl tests clicking multiple times so we don't get a false positive.

[PatrickG]

@PatrickG I believe I've now got the fix to your issue nailed down too. I've also ensured the guard-else-effect test based on your repl tests clicking multiple times so we don't get a false positive.

Very nice 👍

[gyzerok]

Great job with the fix! We are struggling from a similar issue, but unfortunately not able to reporduce it so far. Hopefully that is it :)

In our code this sometimes throws Could not @render snippet due to expression being null or undefined.

{#if someSnippet}
  <div in:scale>
    {@render someSnippet()}
  </div>
{/if}

[hmnd]

@dummdidumm Ah, dang. Good catch. I'll add a sample to cover that and fix it up

[dummdidumm]

I just pushed the test with a fix, should hopefully be all good now (don't know if anyone of you wants to test this again on their own setup with my tweak)

[hmnd]

I'll try it out

[hmnd]

Ran it through my app where I caught some of the other nuances and all looks good!

[dummdidumm]

Great work, thank you!

[webJose]

While it does fix the issue I reported, it seems to introduce a new one: If you play with the same REPL in this version and v5.41.1, you'll notice that the big logo is briefly mounted when moving from the Home route to the medium or small one.

I add the links to REPL's:

v5.41.1
this PR

I can help with explanations on how the router works, if it helps. Thanks.

[hmnd]

@webJose is this not technically more accurate behavior? I haven't looked at the internals of your @wjfe/n-savant library, but it seems even on 5.38.1, rp starts as undefined and then switches to the current routes actual route param. See the console here

[webJose]

Hello, @hmnd. First and foremost: Thank you for contributing a fix for this problem. You're the man.

Second: Maybe you're correct, but it is baffling. You see, my router works with $derived() calculations top to bottom, triggering internal effects, and being the router's code synchronous, to have an update in the parameter (first with an undefined parameter value, and then with the one) is odd for sure.

The router works like this:

• A reactive SvelteURL instance is kept in synchrony with the document's URL.
• When a navigation link is clicked, the window's history API pushState() is used.
• Svelte's reactive system realizes that anything that read the SvelteURL reactive signals need to be recalculated.
• Svelte re-runs derived's in sequence: The first derived is the one that calculates the hash routes in this case. The second one is the one that recalculates the matching status of every route on every router instance (one router in the REPL).
• The matching recalculation is a RegEx match. This matching also obtains the parameter values (the variant parameter in the REPL).
• The matching status (Boolean value) and the parameters obtained are stored in a POJO under the route's key. Basically, a dictionary of route key/matching info.
• The derived returns the value.

The code is here: RouterEngine.svelte.ts. It is a rather small code snippet.

At this point, the contents in the Route components should show or hide based on the recalculations done. To me, the parameter value comes already set. It was obtained at the same time the matching status was evaluated.

If RouterEngine.routeStatus.logo.match is true (because the medium or small logo are now becoming visible depending on the link that was clicked), how come its variant parameter is undefined at first? It was set with a value at the same time the matching status was set.

Maybe this PR is not introducing this problem, as you're noticing it happening in previous versions, but this PR seems to be evidencing a problem: A value that is clearly not undefined, is being reported as undefined. This is causing this visual glitch.

---

Let me know if I can be of assistance.

[PatrickG]

Is rp.variant still initially undefined when you use router.routeStatus[key]?.routeParams here directly instead of params?

Might be because of the breaking change introduced by the async stuff (implemented in #15844 5.36.0):

Because of how async reactivity works, there is one small and unavoidable breaking change: beforeUpdate and $effect.pre callbacks no longer run before control flow blocks are updated. If you're using those callbacks to set state that control flow logic depends on — which you absolutely shouldn't be — then you may experience breakage. As a result, we may have to wait until 6.0 for a non-experimental release of this stuff.
#15845

[webJose]

Hello, @PatrickG. Interesting. I don't know. I have had this code like this for some time. Maybe you're right and now with this "more correct" Svelte behavior the problem lies in the use of the "convenience bindable params property" instead of using the direct derived value. After all, the convenience bindable property has to be updated via an effect.

I'll push a minor version of the router and test.

EDIT: Oh, but BTW, this is why the code uses $effect.pre(), because as per the docs, it says that it happens before rendering. So OK, maybe rp.variant changing from undefined to its "correct" value is due to using params instead of the derived value from the router, but this update is clearly happening after rendering. Would this mean that $effect.pre() is being run after rendering?

[hmnd]

@webJose can confirm the undefined didn't happen prior to 5.36 when the async stuff was introduced

[webJose]

Thanks, @hmnd for double checking. You guys are solid. 👍

I'm about to roll a minor version that will use the router's derived data directly when rendering the children. Still, I think there's a bug here around $effect.pre(), again, probably not introduced in this PR: The fact that the update to params is done in an $effect.pre() after rendering (as per evidence), but $effect.pre() should be running before rendering.

[PatrickG]

Would this mean that $effect.pre() is being run after rendering?

Yes, IMO this breaking change needs to be reverted. There are other issues with it, like #16648

[webJose]

@PatrickG you mean the async change? Or this PR?

[PatrickG]

The breaking change that the async stuff introduced. Not this PR

[webJose]

@PatrickG your assertion was spot on: With @wjfe/n-savant v0.13.0 which I rolled just now, the problem goes away. I think we're 100% correct in saying that this is not a problem with this PR, and that the problem is around $effect.pre() happening after rendering.

FYI, I had to force v0.13.0 in the REPL to test, so I leave a convenience link here of my REPL with the version forcefully stated: REPL

[hmnd]

Also just fyi @webJose v5.41.1 was released before this pr was merged, so it doesn't include these changes yet :)

[Rich-Harris]

Trying to parse the last few comments but just to be clear: do we need to revert this, or is the mentioned bug unrelated? (FWIW I can't tell what's supposed to be happening in the repros in #16930 (comment), they seem to behave identically to me?). I'll block #16992 until we resolve this

[webJose]

tl;dr

This PR looks good. None of us seem to have a problem with it. The found evidence leads us to strongly believe that, at least in my example, $effect.pre() isn't running before rendering. It seems to be running after rendering. What Patrick says needs reversion is some async stuff from previous PR or PR's.

---

Rich-sama! Hello! I'm never tired to cheerfully greet the creator of Svelte, which brought joy to front-end development.

Kidding aside: This PR fixes #16775, a bug I reported about a weird behavior when using {#key}. I was very happy to see this, but I noticed that something odd was happening: When navigating (in the playground link in my reported issue) from the Home route to the Medium or Small routes, the big logo (meant to show up only while in the Big route) started to briefly appear.

Being the ignorant that I am regarding Svelte internals, I reported this new behavior but @hmnd pointed out that rp?.variant, via the use of $inspect, was indeed changing from undefined to whatever variant value the route was for. When rp?.variant is undefined, the big logo is meant to show up. At this point, @PatrickG intervened and asked me a question.

Long story short, the code for my Route component looked like this (v0.12.0):

    ... // ETC.  Previous things not important here.

	// Effect that synchronizes the params property with the calculated params.
	$effect.pre(() => {
		params = router.routeStatus[key]?.routeParams; // <----------  params is synched using $effect.pre
	});
</script>

{#if (router.routeStatus[key]?.match ?? (!and && !path))}
	{@render children?.(params, router.state, router.routeStatus)} // <!-- <-------- params is used to render children -->
{/if}

Look at the second-to-last line: params is there. This is a bindable property to allow easy access to the route's parameter values, which is updated inside an $effect.pre. Well, I said: How come the variant parameter is briefly undefined before becoming the correct value?. This is where Patrick asked: Is router.routeStatus[key]?.routeParams (the value used to sync params) also briefly undefined? Excellent question.

I rolled out v0.13.0 of my router exchanging params in the children snippet. In this new version: {@render children?.(router.routeStatus[key]?.routeParams, router.state, router.routeStatus)}. In other words, we use the source value directly. This fixed the problem.

Conclusion

The problem was not in this PR. As per the evidence, the problem is that since some version ago, $effect.pre is running after rendering, not before rendering as it should. We know this now because the big logo shows up briefly (using v0.12.0). If it shows up, the component rendered. If it rendered with an undefined value, it means $effect.pre() didn't run before rendering, as expected.

[Rich-Harris]

great — thanks for the clarification! will cut the release