Merge pull request #3529 from sveltejs/gh-3526

only update attributes with dynamic dependencies

src/compiler/compile/nodes/Attribute.ts

@@ -5,27 +5,28 @@ import Node from './shared/Node';
 import Element from './Element';
 import Text from './Text';
 import Expression from './shared/Expression';
+import TemplateScope from './shared/TemplateScope';
 
 export default class Attribute extends Node {
 	type: 'Attribute';
 	start: number;
 	end: number;
+	scope: TemplateScope;
 
 	component: Component;
 	parent: Element;
 	name: string;
 	is_spread: boolean;
 	is_true: boolean;
-	is_dynamic: boolean;
 	is_static: boolean;
 	is_synthetic: boolean;
-	should_cache: boolean;
 	expression?: Expression;
 	chunks: Array<Text | Expression>;
 	dependencies: Set<string>;
 
 	constructor(component, parent, scope, info) {
 		super(component, parent, scope, info);
+		this.scope = scope;
 
 		if (info.type === 'Spread') {
 			this.name = null;
@@ -37,9 +38,7 @@ export default class Attribute extends Node {
 			this.dependencies = this.expression.dependencies;
 			this.chunks = null;
 
-			this.is_dynamic = true; // TODO not necessarily
 			this.is_static = false;
-			this.should_cache = false; // TODO does this mean anything here?
 		}
 
 		else {
@@ -62,22 +61,13 @@ export default class Attribute extends Node {
 					add_to_set(this.dependencies, expression.dependencies);
 					return expression;
 				});
-
-			this.is_dynamic = this.dependencies.size > 0;
-
-			this.should_cache = this.is_dynamic
-				? this.chunks.length === 1
-					// @ts-ignore todo: probably error
-					? this.chunks[0].node.type !== 'Identifier' || scope.names.has(this.chunks[0].node.name)
-					: true
-				: false;
 		}
 	}
 
 	get_dependencies() {
 		if (this.is_spread) return this.expression.dynamic_dependencies();
 
-		const dependencies = new Set();
+		const dependencies: Set<string> = new Set();
 		this.chunks.forEach(chunk => {
 			if (chunk.type === 'Expression') {
 				add_to_set(dependencies, chunk.dynamic_dependencies());
@@ -113,7 +103,7 @@ export default class Attribute extends Node {
 	}
 
 	get_static_value() {
-		if (this.is_spread || this.is_dynamic) return null;
+		if (this.is_spread || this.dependencies.size > 0) return null;
 
 		return this.is_true
 			? true
@@ -122,4 +112,13 @@ export default class Attribute extends Node {
 				? (this.chunks[0] as Text).data
 				: '';
 	}
+
+	should_cache() {
+		return this.is_static
+			? false
+			: this.chunks.length === 1
+				// @ts-ignore todo: probably error
+				? this.chunks[0].node.type !== 'Identifier' || this.scope.names.has(this.chunks[0].node.name)
+				: true;
+	}
 }

src/compiler/compile/render_dom/wrappers/Element/Attribute.ts

@@ -70,7 +70,8 @@ export default class AttributeWrapper {
 
 		const is_legacy_input_type = element.renderer.component.compile_options.legacy && name === 'type' && this.parent.node.name === 'input';
 
-		if (this.node.is_dynamic) {
+		const dependencies = this.node.get_dependencies();
+		if (dependencies.length > 0) {
 			let value;
 
 			// TODO some of this code is repeated in Tag.ts — would be good to
@@ -92,7 +93,7 @@ export default class AttributeWrapper {
 			const is_select_value_attribute =
 				name === 'value' && element.node.name === 'select';
 
-			const should_cache = (this.node.should_cache || is_select_value_attribute);
+			const should_cache = (this.node.should_cache() || is_select_value_attribute);
 
 			const last = should_cache && block.get_unique_name(
 				`${element.var}_${name.replace(/[^a-zA-Z_$]/g, '_')}_value`
@@ -147,25 +148,21 @@ export default class AttributeWrapper {
 				updater = `${method}(${element.var}, "${name}", ${should_cache ? last : value});`;
 			}
 
-			// only add an update if mutations are involved (or it's a select?)
+			const changed_check = (
-			const dependencies = this.node.get_dependencies();
+				(block.has_outros ? `!#current || ` : '') +
-			if (dependencies.length > 0 || is_select_value_attribute) {
+				dependencies.map(dependency => `changed.${dependency}`).join(' || ')
-				const changed_check = (
+			);
-					(block.has_outros ? `!#current || ` : '') +
-					dependencies.map(dependency => `changed.${dependency}`).join(' || ')
-				);
 
-				const update_cached_value = `${last} !== (${last} = ${value})`;
+			const update_cached_value = `${last} !== (${last} = ${value})`;
 
-				const condition = should_cache
+			const condition = should_cache
-					? (dependencies.length ? `(${changed_check}) && ${update_cached_value}` : update_cached_value)
+				? (dependencies.length ? `(${changed_check}) && ${update_cached_value}` : update_cached_value)
-					: changed_check;
+				: changed_check;
 
-				block.builders.update.add_conditional(
+			block.builders.update.add_conditional(
-					condition,
+				condition,
-					updater
+				updater
-				);
+			);
-			}
 		} else {
 			const value = this.node.get_value(block);
 
@@ -189,7 +186,7 @@ export default class AttributeWrapper {
 			const update_value = `${element.var}.value = ${element.var}.__value;`;
 
 			block.builders.hydrate.add_line(update_value);
-			if (this.node.is_dynamic) block.builders.update.add_line(update_value);
+			if (this.node.get_dependencies().length > 0) block.builders.update.add_line(update_value);
 		}
 	}
 

src/compiler/compile/render_dom/wrappers/Element/index.ts

@@ -555,8 +555,9 @@ export default class ElementWrapper extends Wrapper {
 	add_attributes(block: Block) {
 		// Get all the class dependencies first
 		this.attributes.forEach((attribute) => {
-			if (attribute.node.name === 'class' && attribute.node.is_dynamic) {
+			if (attribute.node.name === 'class') {
-				this.class_dependencies.push(...attribute.node.dependencies);
+				const dependencies = attribute.node.get_dependencies();
+				this.class_dependencies.push(...dependencies);
 			}
 		});
 

src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts

@@ -168,7 +168,9 @@ export default class InlineComponentWrapper extends Wrapper {
 
 		const non_let_dependencies = Array.from(fragment_dependencies).filter(name => !this.node.scope.is_let(name));
 
-		if (!uses_spread && (this.node.attributes.filter(a => a.is_dynamic).length || this.node.bindings.length || non_let_dependencies.length > 0)) {
+		const dynamic_attributes = this.node.attributes.filter(a => a.get_dependencies().length > 0);
+
+		if (!uses_spread && (dynamic_attributes.length > 0 || this.node.bindings.length > 0 || non_let_dependencies.length > 0)) {
 			updates.push(`var ${name_changes} = {};`);
 		}
 
@@ -225,19 +227,16 @@ export default class InlineComponentWrapper extends Wrapper {
 					]) : {}` : '{}'};
 				`);
 			} else {
-				this.node.attributes
+				dynamic_attributes.forEach((attribute: Attribute) => {
-					.filter((attribute: Attribute) => attribute.is_dynamic)
+					const dependencies = attribute.get_dependencies();
-					.forEach((attribute: Attribute) => {
+					if (dependencies.length > 0) {
-						if (attribute.dependencies.size > 0) {
+						const condition = dependencies.map(dependency => `changed.${dependency}`).join(' || ');
-							/* eslint-disable @typescript-eslint/indent,indent */
+
-							updates.push(deindent`
+						updates.push(deindent`
-								if (${[...attribute.dependencies]
+							if (${condition}) ${name_changes}${quote_prop_if_necessary(attribute.name)} = ${attribute.get_value(block)};
-									.map(dependency => `changed.${dependency}`)
+						`);
-									.join(' || ')}) ${name_changes}${quote_prop_if_necessary(attribute.name)} = ${attribute.get_value(block)};
+					}
-							`);
+				});
-							/* eslint-enable @typescript-eslint/indent,indent */
-						}
-					});
 			}
 		}
 

test/js/samples/component-static-var/expected.js

@@ -0,0 +1,112 @@
+/* generated by Svelte vX.Y.Z */
+import {
+	SvelteComponent,
+	destroy_component,
+	detach,
+	element,
+	init,
+	insert,
+	listen,
+	mount_component,
+	safe_not_equal,
+	set_input_value,
+	space,
+	transition_in,
+	transition_out
+} from "svelte/internal";
+import Foo from "./Foo.svelte";
+import Bar from "./Bar.svelte";
+
+function create_fragment(ctx) {
+	var t0, t1, input, current, dispose;
+
+	var foo = new Foo({ props: { x: y } });
+
+	var bar = new Bar({ props: { x: ctx.z } });
+
+	return {
+		c() {
+			foo.$$.fragment.c();
+			t0 = space();
+			bar.$$.fragment.c();
+			t1 = space();
+			input = element("input");
+			dispose = listen(input, "input", ctx.input_input_handler);
+		},
+
+		m(target, anchor) {
+			mount_component(foo, target, anchor);
+			insert(target, t0, anchor);
+			mount_component(bar, target, anchor);
+			insert(target, t1, anchor);
+			insert(target, input, anchor);
+
+			set_input_value(input, ctx.z);
+
+			current = true;
+		},
+
+		p(changed, ctx) {
+			var bar_changes = {};
+			if (changed.z) bar_changes.x = ctx.z;
+			bar.$set(bar_changes);
+
+			if (changed.z && (input.value !== ctx.z)) set_input_value(input, ctx.z);
+		},
+
+		i(local) {
+			if (current) return;
+			transition_in(foo.$$.fragment, local);
+
+			transition_in(bar.$$.fragment, local);
+
+			current = true;
+		},
+
+		o(local) {
+			transition_out(foo.$$.fragment, local);
+			transition_out(bar.$$.fragment, local);
+			current = false;
+		},
+
+		d(detaching) {
+			destroy_component(foo, detaching);
+
+			if (detaching) {
+				detach(t0);
+			}
+
+			destroy_component(bar, detaching);
+
+			if (detaching) {
+				detach(t1);
+				detach(input);
+			}
+
+			dispose();
+		}
+	};
+}
+
+let y = 1;
+
+function instance($$self, $$props, $$invalidate) {
+
+	let z = 2;
+
+	function input_input_handler() {
+		z = this.value;
+		$$invalidate('z', z);
+	}
+
+	return { z, input_input_handler };
+}
+
+class Component extends SvelteComponent {
+	constructor(options) {
+		super();
+		init(this, options, instance, create_fragment, safe_not_equal, []);
+	}
+}
+
+export default Component;

test/js/samples/component-static-var/input.svelte

@@ -0,0 +1,13 @@
+<script>
+	import Foo from './Foo.svelte';
+	import Bar from './Bar.svelte';
+
+	let y = 1;
+	let z = 2;
+</script>
+
+<Foo x={y}/>
+<Bar x={z}/>
+
+<!-- ensure z is considered dynamic, even though the binding is encountered late -->
+<input bind:value={z}>