perform dirty check before updating keyed each blocks (#4413)

Author: tanhauhau

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Unreleased
 
 * Fix indirect bindings involving elements with spreads ([#3680](https://github.com/sveltejs/svelte/issues/3680))
+* Fix unneeded updating of keyed each blocks ([#4373](https://github.com/sveltejs/svelte/issues/4373))
 
 ## 3.18.2
 

src/compiler/compile/render_dom/wrappers/EachBlock.ts

@@ -412,16 +412,25 @@ export default class EachBlockWrapper extends Wrapper {
 				? `@outro_and_destroy_block`
 				: `@destroy_block`;
 
-		block.chunks.update.push(b`
-			const ${this.vars.each_block_value} = ${snippet};
+		const all_dependencies = new Set(this.block.dependencies); // TODO should be dynamic deps only
+		this.node.expression.dynamic_dependencies().forEach((dependency: string) => {
+			all_dependencies.add(dependency);
+		});
 
-			${this.block.has_outros && b`@group_outros();`}
-			${this.node.has_animation && b`for (let #i = 0; #i < ${view_length}; #i += 1) ${iterations}[#i].r();`}
-			${this.renderer.options.dev && b`@validate_each_keys(#ctx, ${this.vars.each_block_value}, ${this.vars.get_each_context}, ${get_key});`}
-			${iterations} = @update_keyed_each(${iterations}, #dirty, ${get_key}, ${dynamic ? 1 : 0}, #ctx, ${this.vars.each_block_value}, ${lookup}, ${update_mount_node}, ${destroy}, ${create_each_block}, ${update_anchor_node}, ${this.vars.get_each_context});
-			${this.node.has_animation && b`for (let #i = 0; #i < ${view_length}; #i += 1) ${iterations}[#i].a();`}
-			${this.block.has_outros && b`@check_outros();`}
-		`);
+		if (all_dependencies.size) {
+			block.chunks.update.push(b`
+				if (${block.renderer.dirty(Array.from(all_dependencies))}) {
+					const ${this.vars.each_block_value} = ${snippet};
+
+					${this.block.has_outros && b`@group_outros();`}
+					${this.node.has_animation && b`for (let #i = 0; #i < ${view_length}; #i += 1) ${iterations}[#i].r();`}
+					${this.renderer.options.dev && b`@validate_each_keys(#ctx, ${this.vars.each_block_value}, ${this.vars.get_each_context}, ${get_key});`}
+					${iterations} = @update_keyed_each(${iterations}, #dirty, ${get_key}, ${dynamic ? 1 : 0}, #ctx, ${this.vars.each_block_value}, ${lookup}, ${update_mount_node}, ${destroy}, ${create_each_block}, ${update_anchor_node}, ${this.vars.get_each_context});
+					${this.node.has_animation && b`for (let #i = 0; #i < ${view_length}; #i += 1) ${iterations}[#i].a();`}
+					${this.block.has_outros && b`@check_outros();`}
+				}
+			`);
+		}
 
 		if (this.block.has_outros) {
 			block.chunks.outro.push(b`

test/js/samples/each-block-keyed-animated/expected.js

@@ -92,10 +92,12 @@ function create_fragment(ctx) {
 			insert(target, each_1_anchor, anchor);
 		},
 		p(ctx, [dirty]) {
-			const each_value = /*things*/ ctx[0];
-			for (let i = 0; i < each_blocks.length; i += 1) each_blocks[i].r();
-			each_blocks = update_keyed_each(each_blocks, dirty, get_key, 1, ctx, each_value, each_1_lookup, each_1_anchor.parentNode, fix_and_destroy_block, create_each_block, each_1_anchor, get_each_context);
-			for (let i = 0; i < each_blocks.length; i += 1) each_blocks[i].a();
+			if (dirty & /*things*/ 1) {
+				const each_value = /*things*/ ctx[0];
+				for (let i = 0; i < each_blocks.length; i += 1) each_blocks[i].r();
+				each_blocks = update_keyed_each(each_blocks, dirty, get_key, 1, ctx, each_value, each_1_lookup, each_1_anchor.parentNode, fix_and_destroy_block, create_each_block, each_1_anchor, get_each_context);
+				for (let i = 0; i < each_blocks.length; i += 1) each_blocks[i].a();
+			}
 		},
 		i: noop,
 		o: noop,

test/js/samples/each-block-keyed/expected.js

@@ -77,8 +77,10 @@ function create_fragment(ctx) {
 			insert(target, each_1_anchor, anchor);
 		},
 		p(ctx, [dirty]) {
-			const each_value = /*things*/ ctx[0];
-			each_blocks = update_keyed_each(each_blocks, dirty, get_key, 1, ctx, each_value, each_1_lookup, each_1_anchor.parentNode, destroy_block, create_each_block, each_1_anchor, get_each_context);
+			if (dirty & /*things*/ 1) {
+				const each_value = /*things*/ ctx[0];
+				each_blocks = update_keyed_each(each_blocks, dirty, get_key, 1, ctx, each_value, each_1_lookup, each_1_anchor.parentNode, destroy_block, create_each_block, each_1_anchor, get_each_context);
+			}
 		},
 		i: noop,
 		o: noop,

test/runtime/samples/component-binding-blowback-c/main.svelte

@@ -4,7 +4,7 @@
 	export let count;
 	export let idToValue = Object.create(null);
 
-	function ids() {
+	function ids(count) {
 		return new Array(count)
 			.fill(null)
 			.map((_, i) => ({ id: 'id-' + i}))
@@ -15,7 +15,7 @@
 <input type='number' bind:value={count}>
 
 <ol>
-	{#each ids() as object (object.id)}
+	{#each ids(count) as object (object.id)}
 		<Nested bind:value={idToValue[object.id]} id={object.id}>
 			{object.id}: value is {idToValue[object.id]}
 		</Nested>

test/runtime/samples/each-block-keyed-dynamic-2/_config.js

@@ -0,0 +1,23 @@
+export default {
+	html: `
+	<button>Click Me</button>
+	0
+	<ul></ul>
+	`,
+
+	async test({ assert, component, target, window }) {
+		const button = target.querySelector("button");
+
+		const event = new window.MouseEvent("click");
+		await button.dispatchEvent(event);
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<button>Click Me</button>
+			1
+			<ul></ul>
+		`
+		);
+	}
+};

test/runtime/samples/each-block-keyed-dynamic-2/main.svelte

@@ -0,0 +1,20 @@
+<script>
+	let num = 0;
+	let cards = [];
+	
+	function click() {
+		// updating cards via push should have no effect to the ul,
+		// since its being mutated instead of reassigned
+		cards.push(num++);
+	}
+</script>
+<button on:click={click}>
+	Click Me
+</button>
+
+{num}
+<ul>
+	{#each cards as c, i (i)}
+		<li>{c}</li>
+  {/each}
+</ul>