fix: #4265 prevent double update when bind to object

[hackape]

fix #4265

This is a long standing issue that's been reported multiple times, see #4430 #4447 #5555 #6590, the list goes on...It was once partially fixed in #7981 but then reverted in #8114.

Main idea of the fix

The thing that bugs ppl the most is double firing update when bind to non-primitive value at component init phase. This fix targets this scenario specifically.

<script> // Parent.svelte
import Child from './Child.svelte';
let value = ['foo']
$: console.log('parent value', value);
</script>

<Child bind:value />

<script> // Child.svelte
export let value = ['zoo'] 
$: console.log('child value', value);
</script>

<div>{JSON.stringify(value)}</div>

I'm gonna use the term "backflow" to refer to the process of child_value_binding() -> invalidate(parent_value). Backflow is necessary to sync child value back to parent. But it could be unnecessary in some cases. Because safe_not_equal conservatively invalidates non-primitive value, it's a failed guard. So this fix propose an extra flag skip_binding to guard against unnecessary backflow.

Noted that first backflow happen at flush -> binding_callbacks.pop()() -> bind -> child_value_binding. By this time we've completed init phase, and every component is ready.

Now focus on init phase and consider the following cases (✅ = necessary backflow, ❌ = unnecessary backflow)

1. ✅ first to tick off the obvious, when Parent value is undefined, Child shall backflow to populate Parent value.
   Below we discuss cases where Parent pass non-undefined down to Child.
2. ❌ Child does not touch the value at all, then value is already at-sync after init, no backflow needed.
   This is also THE MOST COMMON USE CASE which current PR seek to fix.
3. ✅ Child reactively (within $: statements) modifies the value, during child.$$.update() call, need backflow.
4. ✅ Child imperatively modifies the value at init phase, during init -> instance call, need backflow.
   This case should be further broken down to 2 sub-cases:
   4.1. Immutable modification, value's reference changed, e.g. export let value; value = ["default"];
   4.2. Mutable modification, e.g. export let value; value.foo = "bar";

Proposed fix correctly handles backflow in all above cases EXCEPT sub-case 4.2.

Proposed fix correctly handles backflow in all above cases.

Implementation

Changes made to three places:

1. The $$invalidate runtime function, when ready == false sets $$.bound[i] = true in order to cater for case 3. This mark is essentially the same as $$.dirty which is not available before ready.
2. The bind runtime function triggers the first backflow during flush, here I pass a skip_binding flag to child_value_binding. The flag is determined by $$.bound[i] dirty mark. This takes care of case 2 and 3.
3. The generated child_value_binding function, I add if (!#skip_binding || ${lhs} !== #value) guard. It prevents the unnecessary backflow of case 2, also ${lhs} !== #value check allows backflow for case 1 and 4.1.

Case 4.2. needs backflow but is unfortunately left out, because a) $$invalidate is not called, b) ${lhs} !== #value fails to detect, we don't have the tool to distinguish this case. and is now detected by inserting $$invalidate call on prop assignment at instance top level context.

---

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.

Tests and linting

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

[changeset-bot]

⚠️ No Changeset found

Latest commit: 34dcc5d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[hackape]

The $$invalidate runtime function, when ready == false sets $$.bound[i] = true in order to cater for case 3. This mark is essentially the same as $$.dirty which is not available before ready.

[hackape]

The bind runtime function triggers the first backflow during flush, here I pass a skip_binding flag to child_value_binding. The flag is determined by $$.bound[i] dirty mark. This takes care of case 2 and 3.

Case 2: Child does not touch passed-in prop value
Case 3: Child modifies the value inside $: reactive statements

[hackape]

if (!#skip_binding || ${lhs} !== #value) guard prevents the unnecessary backflow of case 2, also ${lhs} !== #value check allows backflow for case 1 and 4.1.

Case 2: Child does not touch passed-in prop value, regardless it's primitive or object, backflow is unnecessary
Case 1 and 4.1: Child value is referentially different from Parent value

[dummdidumm]

4.2 working now but not working anymore is a breaking change very strictly speaking - though this case is probably rare and you shouldn't be doing something like this anyway, so I could be persuaded to agree with this PR. I'll wait on the opinion of other maintainers here, cc @benmccann / @Conduitry

[hackape]

@dummdidumm It's possible to cater for case 4.2 and get rid of breaking change. Basically we need to insert $$invalidate calls also at instance top level, so that $$.bound[i] = true can get a chance to pick up the signal.

<script>
	export let value = {}
	value.foo = 'bar'
</script>

compiled to

function instance($$self, $$props, $$invalidate) {
    let { value = {} } = $$props;
-   value.foo = 'bar';
+   $$invalidate(0, value.foo = 'bar', value);

IMO that comes at the cost of breaking the elegancy of instance, which in its current state leaves most of the things untouched. To me, "init phase is plain JS without magic" is a mind model that I love a lot.

So I don't think it's worth it, since 4.2 feels like a really bad pattern to begin with and shouldn't be encouraged. But if you guys decide that it's really important to stick to non-breaking-change policy, I'll look further into it. Look forward to feedback.

[hackape]

Compiler also inserts $$invalidate around exported variables at instance main_execution_context.

[dummdidumm]

@Conduitry / @tanhauhau is this ok to add invalidation in this case, too? Why's invalidation generally forbidden in the main execution context?

[hackape]

Compiler also inserts $$invalidate around exported variables at instance main_execution_context.

[hackape]

svelte/packages/svelte/src/compiler/compile/render_dom/invalidate.js

Lines 130 to 132 in 0e5e471

	} else if (main_execution_context && is_props) {
	const member = renderer.context_lookup.get(name);
	return x`$$invalidate(${member.index}, ${name})`;

Mod invalidate.js to also account for props at main_execution_context, so that we detect assign-to-prop at init phase. Now case 4.2 is correctly handled, no more breaking change 🥹

[hackape]

Hi @dummdidumm and other core team members, I'd like to push forward this PR. Now that I've removed the breaking change issue, would you take another look and give me some feedback on how to proceed? cc @benmccann @Conduitry

[coryvirok]

Any update on if this is ready to be merged?