From bfeda78c55be8f31ed39fa77297ec3c84296e05b Mon Sep 17 00:00:00 2001 From: hackape Date: Tue, 18 Jul 2023 18:21:40 +0800 Subject: [PATCH 1/6] fix: #4265 prevent double update when bind to object --- .../render_dom/wrappers/InlineComponent/index.js | 13 +++++++++---- packages/svelte/src/runtime/internal/Component.js | 13 +++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js b/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js index ef7e5432d47e..cf3625ed4178 100644 --- a/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js +++ b/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js @@ -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` @@ -360,9 +360,14 @@ export default class InlineComponentWrapper extends Wrapper { } `; } + // `skip_binding` is set by runtime/internal `bind()` function only at first call + // this prevents child -> parent reflow that triggers unnecessary $$.update (#4265) + // ignore this flag when parent value is undefined const body = b` - function ${id}(${params}) { - ${invalidate_binding} + function ${id}(#skip_binding, ${params}) { + if (!#skip_binding || ${lhs} === void 0) { + ${invalidate_binding} + } } `; component.partly_hoisted.push(body); diff --git a/packages/svelte/src/runtime/internal/Component.js b/packages/svelte/src/runtime/internal/Component.js index 4733b1d74594..818503cdb688 100644 --- a/packages/svelte/src/runtime/internal/Component.js +++ b/packages/svelte/src/runtime/internal/Component.js @@ -21,10 +21,15 @@ 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; + if (component.$$.dirty[0] !== -1) { + dirty = !!(component.$$.dirty[(i / 31) | 0] & (1 << i % 31)); + } + 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); } } From 1cc1280b7cd6637f93d6230daca2ed6a828b3215 Mon Sep 17 00:00:00 2001 From: hackape Date: Wed, 19 Jul 2023 11:10:25 +0800 Subject: [PATCH 2/6] fix: correctly detect bound value mutation at init phase --- .../compile/render_dom/wrappers/InlineComponent/index.js | 7 ++++--- packages/svelte/src/runtime/internal/Component.js | 7 +++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js b/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js index cf3625ed4178..54a80753ec40 100644 --- a/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js +++ b/packages/svelte/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js @@ -361,11 +361,12 @@ export default class InlineComponentWrapper extends Wrapper { `; } // `skip_binding` is set by runtime/internal `bind()` function only at first call - // this prevents child -> parent reflow that triggers unnecessary $$.update (#4265) - // ignore this flag when parent value is undefined + // 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}(#skip_binding, ${params}) { - if (!#skip_binding || ${lhs} === void 0) { + if (!#skip_binding || ${lhs} !== #value) { ${invalidate_binding} } } diff --git a/packages/svelte/src/runtime/internal/Component.js b/packages/svelte/src/runtime/internal/Component.js index 818503cdb688..5b38f7ffd872 100644 --- a/packages/svelte/src/runtime/internal/Component.js +++ b/packages/svelte/src/runtime/internal/Component.js @@ -24,9 +24,7 @@ export function bind(component, name, callback) { const i = component.$$.props[name]; if (i !== undefined) { let dirty = false; - if (component.$$.dirty[0] !== -1) { - dirty = !!(component.$$.dirty[(i / 31) | 0] & (1 << i % 31)); - } + if (component.$$.bound[i]) 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); @@ -130,8 +128,9 @@ export function init( ? instance(component, options.props || {}, (i, ret, ...rest) => { const value = rest.length ? rest[0] : ret; if ($$.ctx && not_equal($$.ctx[i], ($$.ctx[i] = value))) { - if (!$$.skip_bound && $$.bound[i]) $$.bound[i](value); + if (!$$.skip_bound && is_function($$.bound[i])) $$.bound[i](value); if (ready) make_dirty(component, i); + else $$.bound[i] = true; // dirty flag consumed in bind() } return ret; }) From 0341e0b5d75d6facc4392ef288b6be69e674db57 Mon Sep 17 00:00:00 2001 From: hackape Date: Wed, 19 Jul 2023 17:15:29 +0800 Subject: [PATCH 3/6] chore: add testcases --- .../samples/binding-backflow/Child.svelte | 21 ++++++++++ .../samples/binding-backflow/Parent.svelte | 12 ++++++ .../samples/binding-backflow/_config.js | 40 +++++++++++++++++++ .../samples/binding-backflow/main.svelte | 9 +++++ 4 files changed, 82 insertions(+) create mode 100644 packages/svelte/test/runtime/samples/binding-backflow/Child.svelte create mode 100644 packages/svelte/test/runtime/samples/binding-backflow/Parent.svelte create mode 100644 packages/svelte/test/runtime/samples/binding-backflow/_config.js create mode 100644 packages/svelte/test/runtime/samples/binding-backflow/main.svelte diff --git a/packages/svelte/test/runtime/samples/binding-backflow/Child.svelte b/packages/svelte/test/runtime/samples/binding-backflow/Child.svelte new file mode 100644 index 000000000000..bd398d0c2257 --- /dev/null +++ b/packages/svelte/test/runtime/samples/binding-backflow/Child.svelte @@ -0,0 +1,21 @@ + + +
child: {value?.foo} | updates: {updates.length}
diff --git a/packages/svelte/test/runtime/samples/binding-backflow/Parent.svelte b/packages/svelte/test/runtime/samples/binding-backflow/Parent.svelte new file mode 100644 index 000000000000..6a8ed2fe25f5 --- /dev/null +++ b/packages/svelte/test/runtime/samples/binding-backflow/Parent.svelte @@ -0,0 +1,12 @@ + + +
parent: {value?.foo} | updates: {updates.length}
+ diff --git a/packages/svelte/test/runtime/samples/binding-backflow/_config.js b/packages/svelte/test/runtime/samples/binding-backflow/_config.js new file mode 100644 index 000000000000..02d8c417da86 --- /dev/null +++ b/packages/svelte/test/runtime/samples/binding-backflow/_config.js @@ -0,0 +1,40 @@ +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' } } + ] + }; + }, + + 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); + } +}; diff --git a/packages/svelte/test/runtime/samples/binding-backflow/main.svelte b/packages/svelte/test/runtime/samples/binding-backflow/main.svelte new file mode 100644 index 000000000000..f7cfd4472d17 --- /dev/null +++ b/packages/svelte/test/runtime/samples/binding-backflow/main.svelte @@ -0,0 +1,9 @@ + + +{#each configs as config} + +{/each} From 841b6418637b097484d4bf5e2820f3da7456fe4e Mon Sep 17 00:00:00 2001 From: hackape Date: Sun, 23 Jul 2023 09:09:52 +0800 Subject: [PATCH 4/6] fix: invalidate props in `main_execution_context` --- .../src/compiler/compile/render_dom/invalidate.js | 12 +++++++++--- packages/svelte/src/runtime/internal/Component.js | 10 +++++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/compiler/compile/render_dom/invalidate.js b/packages/svelte/src/compiler/compile/render_dom/invalidate.js index 83423e952bc3..3401e7aec7d0 100644 --- a/packages/svelte/src/compiler/compile/render_dom/invalidate.js +++ b/packages/svelte/src/compiler/compile/render_dom/invalidate.js @@ -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); @@ -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) { const pass_value = extra_args.length > 0 || (node.type === 'AssignmentExpression' && node.left.type !== 'Identifier') || @@ -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); @@ -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})`; } if (main_execution_context) return; // if this is a reactive declaration, invalidate dependencies recursively diff --git a/packages/svelte/src/runtime/internal/Component.js b/packages/svelte/src/runtime/internal/Component.js index 5b38f7ffd872..5463a5c49110 100644 --- a/packages/svelte/src/runtime/internal/Component.js +++ b/packages/svelte/src/runtime/internal/Component.js @@ -24,7 +24,8 @@ export function bind(component, name, callback) { const i = component.$$.props[name]; if (i !== undefined) { let dirty = false; - if (component.$$.bound[i]) dirty = true; + // 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); @@ -127,10 +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 && is_function($$.bound[i])) $$.bound[i](value); + if (!$$.skip_bound && $$.bound[i]) $$.bound[i](value); if (ready) make_dirty(component, i); - else $$.bound[i] = true; // dirty flag consumed in bind() + else $$.bound[i] = null; } return ret; }) From 0e5e4715c6b02176a4d066e57defe35f85cb3770 Mon Sep 17 00:00:00 2001 From: hackape Date: Sun, 23 Jul 2023 09:10:42 +0800 Subject: [PATCH 5/6] fix: update testcase --- .../test/runtime/samples/binding-backflow/Child.svelte | 4 ++++ .../test/runtime/samples/binding-backflow/_config.js | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/svelte/test/runtime/samples/binding-backflow/Child.svelte b/packages/svelte/test/runtime/samples/binding-backflow/Child.svelte index bd398d0c2257..87dcf926f395 100644 --- a/packages/svelte/test/runtime/samples/binding-backflow/Child.svelte +++ b/packages/svelte/test/runtime/samples/binding-backflow/Child.svelte @@ -6,6 +6,10 @@ value = { foo: 'kid' } } + if (testcase === 'init_mutate') { + value.foo = 'kid' + } + $: if (testcase === 'reactive_update') { value = { foo: 'kid' } } diff --git a/packages/svelte/test/runtime/samples/binding-backflow/_config.js b/packages/svelte/test/runtime/samples/binding-backflow/_config.js index 02d8c417da86..5eb30b207f0d 100644 --- a/packages/svelte/test/runtime/samples/binding-backflow/_config.js +++ b/packages/svelte/test/runtime/samples/binding-backflow/_config.js @@ -6,7 +6,8 @@ export default { { 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_update', value: { foo: 'mon' } }, + { testcase: 'init_mutate', value: { foo: 'mon' } } ] }; }, @@ -36,5 +37,9 @@ export default { 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); } }; From 34dcc5d688cc1710f0892384f33ace9c096931fe Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Wed, 20 Sep 2023 10:23:03 +0200 Subject: [PATCH 6/6] try unskipping the other test --- .../samples/binding-no-unnecessary-invalidation.skip/_config.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/svelte/test/runtime/samples/binding-no-unnecessary-invalidation.skip/_config.js b/packages/svelte/test/runtime/samples/binding-no-unnecessary-invalidation.skip/_config.js index 8cf108ab7618..cc93b73a874a 100644 --- a/packages/svelte/test/runtime/samples/binding-no-unnecessary-invalidation.skip/_config.js +++ b/packages/svelte/test/runtime/samples/binding-no-unnecessary-invalidation.skip/_config.js @@ -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,