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

fix: #4265 prevent double update when bind to object #8992

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions packages/svelte/src/compiler/compile/render_dom/invalidate.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ export function invalidate(renderer, scope, node, names, main_execution_context
* @param {import('estree').Expression} [node]
*/
function get_invalidated(variable, node) {
if (main_execution_context && !variable.subscribable && variable.name[0] !== '$') {
const is_props = !!variable.export_name;
if (main_execution_context && !is_props && !variable.subscribable && variable.name[0] !== '$') {
return node;
}
return renderer_invalidate(renderer, variable.name, undefined, main_execution_context);
Expand All @@ -61,8 +62,9 @@ export function invalidate(renderer, scope, node, names, main_execution_context
return x`@set_store_value(${head.name.slice(1)}, ${node}, ${head.name}, ${extra_args})`;
}

const is_props = !!head.export_name;
let invalidate;
if (!main_execution_context) {
if (!main_execution_context || is_props) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const pass_value =
extra_args.length > 0 ||
(node.type === 'AssignmentExpression' && node.left.type !== 'Identifier') ||
Expand Down Expand Up @@ -96,8 +98,9 @@ export function invalidate(renderer, scope, node, names, main_execution_context
*/
export function renderer_invalidate(renderer, name, value, main_execution_context = false) {
const variable = renderer.component.var_lookup.get(name);
const is_props = variable && variable.export_name && !variable.module;
if (variable && variable.subscribable && (variable.reassigned || variable.export_name)) {
if (main_execution_context) {
if (main_execution_context && !is_props) {
return x`${`$$subscribe_${name}`}(${value || name})`;
} else {
const member = renderer.context_lookup.get(name);
Expand All @@ -124,6 +127,9 @@ export function renderer_invalidate(renderer, name, value, main_execution_contex
const member = renderer.context_lookup.get(name);
return x`$$invalidate(${member.index}, ${value})`;
}
} else if (main_execution_context && is_props) {
const member = renderer.context_lookup.get(name);
return x`$$invalidate(${member.index}, ${name})`;
Comment on lines +130 to +132
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if (main_execution_context) return;
// if this is a reactive declaration, invalidate dependencies recursively
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,8 @@ export default class InlineComponentWrapper extends Wrapper {
block.maintain_context = true; // TODO put this somewhere more logical
}
block.chunks.init.push(b`
function ${id}(#value) {
${callee}(${args});
function ${id}(#value, #skip_binding) {
${callee}(#skip_binding, ${args});
}
`);
let invalidate_binding = b`
Expand All @@ -360,9 +360,15 @@ export default class InlineComponentWrapper extends Wrapper {
}
`;
}
// `skip_binding` is set by runtime/internal `bind()` function only at first call
// this prevents child -> parent backflow that triggers unnecessary $$.update (#4265)
// the flag is used to detech reactive mutation to object in `child.$$.update`
// ignore this flag when parent_value !== child_value
const body = b`
function ${id}(${params}) {
${invalidate_binding}
function ${id}(#skip_binding, ${params}) {
if (!#skip_binding || ${lhs} !== #value) {
${invalidate_binding}
}
Comment on lines +369 to +371
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
`;
component.partly_hoisted.push(body);
Expand Down
16 changes: 12 additions & 4 deletions packages/svelte/src/runtime/internal/Component.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ import { transition_in } from './transitions.js';

/** @returns {void} */
export function bind(component, name, callback) {
const index = component.$$.props[name];
if (index !== undefined) {
component.$$.bound[index] = callback;
callback(component.$$.ctx[index]);
const i = component.$$.props[name];
if (i !== undefined) {
let dirty = false;
// special dirty flag for bind
if (component.$$.bound[i] === null) dirty = true;
component.$$.bound[i] = callback;
// first binding call, if child value is not yet dirty, skip to prevent unnecessary backflow
callback(component.$$.ctx[i], /** skip_binding */ !dirty);
}
}

Expand Down Expand Up @@ -124,9 +128,13 @@ export function init(
$$.ctx = instance
? instance(component, options.props || {}, (i, ret, ...rest) => {
const value = rest.length ? rest[0] : ret;
// `$$.bound[i] = null` as a special dirty flag to prevent unnecessary backflow, consumed in bind()
// only set at init phase during `instance()` call, and 1st `$$.update()` call before `ready`
if (!$$.ctx) $$.bound[i] = null;
if ($$.ctx && not_equal($$.ctx[i], ($$.ctx[i] = value))) {
if (!$$.skip_bound && $$.bound[i]) $$.bound[i](value);
if (ready) make_dirty(component, i);
else $$.bound[i] = null;
}
return ret;
})
Expand Down
25 changes: 25 additions & 0 deletions packages/svelte/test/runtime/samples/binding-backflow/Child.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<script>
export let testcase;
export let value = { foo: 'kid' };

if (testcase === 'init_update') {
value = { foo: 'kid' }
}

if (testcase === 'init_mutate') {
value.foo = 'kid'
}

$: if (testcase === 'reactive_update') {
value = { foo: 'kid' }
}

$: if (testcase === 'reactive_mutate') {
value.foo = 'kid'
}

export let updates = [];
$: updates = [...updates, value];
</script>

<div>child: {value?.foo} | updates: {updates.length}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<script>
import Child from './Child.svelte';
export let child;

export let testcase;
export let value;
export let updates = [];
$: updates = [...updates, value];
</script>

<div>parent: {value?.foo} | updates: {updates.length}</div>
<Child bind:value bind:this={child} {testcase} />
45 changes: 45 additions & 0 deletions packages/svelte/test/runtime/samples/binding-backflow/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
export default {
get props() {
return {
configs: [
{ testcase: 'parent_override_child_default', value: { foo: 'mon' } },
{ testcase: 'child_default_populate_parent', value: undefined },
{ testcase: 'reactive_update', value: { foo: 'mon' } },
{ testcase: 'reactive_mutate', value: { foo: 'mon' } },
{ testcase: 'init_update', value: { foo: 'mon' } },
{ testcase: 'init_mutate', value: { foo: 'mon' } }
]
};
},

async test({ assert, component }) {
const parents = component.parents;

// first testcase should update once
// the rest should update twice
let p;
p = parents['parent_override_child_default'];
assert.deepEqual(p.value, { foo: 'mon' });
assert.equal(p.updates.length, 1);

p = parents['child_default_populate_parent'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 2);

p = parents['reactive_update'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 2);

p = parents['reactive_mutate'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 2);

p = parents['init_update'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 2);

p = parents['init_mutate'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 2);
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<script>
import Parent from './Parent.svelte';
export let configs = [];
export let parents = {};
</script>

{#each configs as config}
<Parent bind:this={parents[config.testcase]} value={config.value} testcase={config.testcase} />
{/each}
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// this test currently fails because the fix that made it pass broke other tests,
// see https://github.com/sveltejs/svelte/pull/8114 for more context.
export default {
skip: true,
async test({ assert, target }) {
assert.htmlEqual(
target.innerHTML,
Expand Down