fix: memoize repeated state reads in guard expressions #16881
Conversation
🦋 Changeset detectedLatest commit: a758384 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
| walk(value, null, { | ||
| MemberExpression(node) { | ||
| if (node.object.type === 'Identifier' && memoized.has(node.object.name)) { | ||
| node.optional = true; |
There was a problem hiding this comment.
I have a suspicion that this is what "fixes" the problem by turning
centerRow.depth != undefined && centerRow.depth > 0into
centerRow_value?.depth != undefined && centerRow_value?.depth > 0which is not really equal.
There was a problem hiding this comment.
@7nik Thanks for double-checking! At first glance it might look a bit different, but I think Svelte still seems to emit the leading guard
return (
centerRow_value != undefined &&
centerRow_value?.depth != undefined &&
centerRow_value?.depth > 0
);From what I can tell, the optional chaining only comes into play after we already know that centerRow_value isn’t null or undefined. In that case, centerRow_value?.depth would effectively behave the same as centerRow_value.depth. The ?. just seems to act as a safety net, in case the value somehow changes between reads while reusing the cached value. So I believe the semantics of the guard essentially remain the same.
There was a problem hiding this comment.
Look at this variant - it fails on this PR as well. The problem is that the nested if / template effect / etc runs when it shouldn't.
There was a problem hiding this comment.
@7nik Oops, you’re right
it looks like this new variant still slips through what I’ve tried so far. From what I can tell, the inner template effect is still calling $.get(centerRow) even after the guard has already short-circuited, which makes the block run when it really shouldn’t.
I see two possible ways we could approach this. One would be to hoist the guard result so that the entire block (effects and nested branches included) reuses the same snapshot. The other would be to change how the template effect checks for guard failure before it runs.
Do you think one of these directions would fit better?
There was a problem hiding this comment.
The outer if should execute first and destroy the branch, thus no execution of centerRow.depth when centerRow is undefined. But I'm not so familiar with the reactivity internals (especially async ones) to say why effects are executed in another order and how to fix it.
There was a problem hiding this comment.
@7nik Thanks for catching that earlier variant! I hoisted the guard value so the entire block (including template effects and nested branches) reuses the same snapshot.
If you’re aware of other guard scenarios we should test, I’d really appreciate the pointers.
Fixes #16850
Starting in Svelte 5.38.2, guard expressions such as
{#if centerRow && centerRow.depth}could end up evaluating$.get(centerRow)multiple times within a single expression. This meant that the first null check and the subsequentcenterRow.depthaccess might see different values, which could cause aCannot read properties of undefinederror. The fix changesbuild_expressionso that it reads a state or derived signal exactly once per expression, caches that intermediate result, and then reuses it, while deliberately leaving reads inside nested functions untouched so that existing behavior continues to work as before. In addition, theIfBlockvisitor was updated to compute the guard expression up front so that any nested blocks share the memoized value. If there is a more robust or idiomatic way to handle this in Svelte, I would very much appreciate any feedback or suggestions.feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint