Merge pull request #3478 from sveltejs/gh-3447

Don't generate code for a non-updating if block
sveltejs · Sep 4, 2019 · d75b638 · d75b638
2 parents 85cfa52 + c5e117a
commit d75b638
Show file tree

Hide file tree

Showing 7 changed files with 169 additions and 59 deletions.
diff --git a/src/compiler/compile/render_dom/wrappers/IfBlock.ts b/src/compiler/compile/render_dom/wrappers/IfBlock.ts
@@ -39,7 +39,7 @@ class IfBlockBranch extends Wrapper {
 		const is_else = !expression;
 
 		if (expression) {
-			const dependencies = expression.dynamic_dependencies();
+			this.dependencies = expression.dynamic_dependencies();
 
 			// TODO is this the right rule? or should any non-reference count?
 			// const should_cache = !is_reference(expression.node, null) && dependencies.length > 0;
@@ -55,7 +55,6 @@ class IfBlockBranch extends Wrapper {
 			if (should_cache) {
 				this.condition = block.get_unique_name(`show_if`);
 				this.snippet = expression.render(block);
-				this.dependencies = dependencies;
 			} else {
 				this.condition = expression.render(block);
 			}
@@ -244,7 +243,7 @@ export default class IfBlockWrapper extends Wrapper {
 			function ${select_block_type}(changed, ctx) {
 				${this.branches.map(({ dependencies, condition, snippet, block }) => condition
 				? deindent`
-				${dependencies && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`}
+				${snippet && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`}
 				if (${condition}) return ${block.name};`
 				: `return ${block.name};`)}
 			}
@@ -327,7 +326,7 @@ export default class IfBlockWrapper extends Wrapper {
 			function ${select_block_type}(changed, ctx) {
 				${this.branches.map(({ dependencies, condition, snippet }, i) => condition
 				? deindent`
-				${dependencies && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`}
+				${snippet && `if ((${condition} == null) || ${dependencies.map(n => `changed.${n}`).join(' || ')}) ${condition} = !!(${snippet})`}
 				if (${condition}) return ${String(i)};`
 				: `return ${i};`)}
 				${!has_else && `return -1;`}
@@ -441,58 +440,62 @@ export default class IfBlockWrapper extends Wrapper {
 			`if (${name}) ${name}.m(${initial_mount_node}, ${anchor_node});`
 		);
 
-		const update_mount_node = this.get_update_mount_node(anchor);
-
-		const enter = dynamic
-			? deindent`
-				if (${name}) {
-					${name}.p(changed, ctx);
-					${has_transitions && `@transition_in(${name}, 1);`}
-				} else {
-					${name} = ${branch.block.name}(ctx);
-					${name}.c();
-					${has_transitions && `@transition_in(${name}, 1);`}
-					${name}.m(${update_mount_node}, ${anchor});
-				}
-			`
-			: deindent`
-				if (!${name}) {
-					${name} = ${branch.block.name}(ctx);
-					${name}.c();
-					${has_transitions && `@transition_in(${name}, 1);`}
-					${name}.m(${update_mount_node}, ${anchor});
-				${has_transitions && `} else {
-					@transition_in(${name}, 1);`}
-				}
-			`;
+		if (branch.dependencies.length > 0) {
+			const update_mount_node = this.get_update_mount_node(anchor);
 
-		if (branch.snippet) {
-			block.builders.update.add_block(`if (${branch.dependencies.map(n => `changed.${n}`).join(' || ')}) ${branch.condition} = ${branch.snippet}`);
-		}
+			const enter = dynamic
+				? deindent`
+					if (${name}) {
+						${name}.p(changed, ctx);
+						${has_transitions && `@transition_in(${name}, 1);`}
+					} else {
+						${name} = ${branch.block.name}(ctx);
+						${name}.c();
+						${has_transitions && `@transition_in(${name}, 1);`}
+						${name}.m(${update_mount_node}, ${anchor});
+					}
+				`
+				: deindent`
+					if (!${name}) {
+						${name} = ${branch.block.name}(ctx);
+						${name}.c();
+						${has_transitions && `@transition_in(${name}, 1);`}
+						${name}.m(${update_mount_node}, ${anchor});
+					} ${has_transitions && `else @transition_in(${name}, 1);`}
+				`;
+
+			if (branch.snippet) {
+				block.builders.update.add_block(`if (${branch.dependencies.map(n => `changed.${n}`).join(' || ')}) ${branch.condition} = ${branch.snippet}`);
+			}
 
-		// no `p()` here — we don't want to update outroing nodes,
-		// as that will typically result in glitching
-		if (branch.block.has_outro_method) {
-			block.builders.update.add_block(deindent`
-				if (${branch.condition}) {
-					${enter}
-				} else if (${name}) {
-					@group_outros();
-					@transition_out(${name}, 1, 1, () => {
+			// no `p()` here — we don't want to update outroing nodes,
+			// as that will typically result in glitching
+			if (branch.block.has_outro_method) {
+				block.builders.update.add_block(deindent`
+					if (${branch.condition}) {
+						${enter}
+					} else if (${name}) {
+						@group_outros();
+						@transition_out(${name}, 1, 1, () => {
+							${name} = null;
+						});
+						@check_outros();
+					}
+				`);
+			} else {
+				block.builders.update.add_block(deindent`
+					if (${branch.condition}) {
+						${enter}
+					} else if (${name}) {
+						${name}.d(1);
 						${name} = null;
-					});
-					@check_outros();
-				}
-			`);
-		} else {
-			block.builders.update.add_block(deindent`
-				if (${branch.condition}) {
-					${enter}
-				} else if (${name}) {
-					${name}.d(1);
-					${name} = null;
-				}
-			`);
+					}
+				`);
+			}
+		} else if (dynamic) {
+			block.builders.update.add_block(
+				`if (${branch.condition}) ${name}.p(changed, ctx);`
+			);
 		}
 
 		block.builders.destroy.add_line(`${if_name}${name}.d(${detaching});`);

diff --git a/test/js/samples/if-block-complex/expected.js b/test/js/samples/if-block-complex/expected.js
@@ -0,0 +1,81 @@
+/* generated by Svelte vX.Y.Z */
+import {
+	SvelteComponent,
+	attr,
+	detach,
+	element,
+	empty,
+	init,
+	insert,
+	noop,
+	safe_not_equal
+} from "svelte/internal";
+
+// (7:0) {#if (item.divider && item.divider.includes(1))}
+function create_if_block(ctx) {
+	var div;
+
+	return {
+		c() {
+			div = element("div");
+			attr(div, "class", "divider");
+		},
+
+		m(target, anchor) {
+			insert(target, div, anchor);
+		},
+
+		d(detaching) {
+			if (detaching) {
+				detach(div);
+			}
+		}
+	};
+}
+
+function create_fragment(ctx) {
+	var show_if = (ctx.item.divider && ctx.item.divider.includes(1)), if_block_anchor;
+
+	var if_block = (show_if) && create_if_block(ctx);
+
+	return {
+		c() {
+			if (if_block) if_block.c();
+			if_block_anchor = empty();
+		},
+
+		m(target, anchor) {
+			if (if_block) if_block.m(target, anchor);
+			insert(target, if_block_anchor, anchor);
+		},
+
+		p: noop,
+		i: noop,
+		o: noop,
+
+		d(detaching) {
+			if (if_block) if_block.d(detaching);
+
+			if (detaching) {
+				detach(if_block_anchor);
+			}
+		}
+	};
+}
+
+function instance($$self) {
+	let item = {
+		divider: [1]
+	}
+
+	return { item };
+}
+
+class Component extends SvelteComponent {
+	constructor(options) {
+		super();
+		init(this, options, instance, create_fragment, safe_not_equal, []);
+	}
+}
+
+export default Component;
diff --git a/test/js/samples/if-block-complex/input.svelte b/test/js/samples/if-block-complex/input.svelte
@@ -0,0 +1,9 @@
+<script>
+	let item = {
+		divider: [1]
+	}
+</script>
+
+{#if (item.divider && item.divider.includes(1))}
+<div class="divider"></div>
+{/if}
diff --git a/test/js/samples/transition-local/expected.js b/test/js/samples/transition-local/expected.js
@@ -37,9 +37,7 @@ function create_if_block(ctx) {
 					if_block.c();
 					transition_in(if_block, 1);
 					if_block.m(if_block_anchor.parentNode, if_block_anchor);
-				} else {
-									transition_in(if_block, 1);
-				}
+				} else transition_in(if_block, 1);
 			} else if (if_block) {
 				if_block.d(1);
 				if_block = null;

diff --git a/test/js/samples/transition-repeated-outro/expected.js b/test/js/samples/transition-repeated-outro/expected.js
@@ -76,9 +76,7 @@ function create_fragment(ctx) {
 					if_block.c();
 					transition_in(if_block, 1);
 					if_block.m(if_block_anchor.parentNode, if_block_anchor);
-				} else {
-									transition_in(if_block, 1);
-				}
+				} else transition_in(if_block, 1);
 			} else if (if_block) {
 				group_outros();
 				transition_out(if_block, 1, 1, () => {

diff --git a/test/runtime/samples/if-block-static-with-dynamic-contents/_config.js b/test/runtime/samples/if-block-static-with-dynamic-contents/_config.js
@@ -0,0 +1,12 @@
+export default {
+	props: {
+		foo: 42
+	},
+
+	html: '<p>42</p>',
+
+	test({ assert, component, target }) {
+		component.foo = 43;
+		assert.htmlEqual(target.innerHTML, '<p>43</p>');
+	}
+};
diff --git a/test/runtime/samples/if-block-static-with-dynamic-contents/main.svelte b/test/runtime/samples/if-block-static-with-dynamic-contents/main.svelte
@@ -0,0 +1,9 @@
+<script>
+	export let foo;
+
+	const show = () => true;
+</script>
+
+{#if show()}
+	<p>{foo}</p>
+{/if}