Fix contextual bind:this (#2806)

Author: Rich-Harris

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

@@ -19,6 +19,7 @@ import add_event_handlers from '../shared/add_event_handlers';
 import add_actions from '../shared/add_actions';
 import create_debugging_comment from '../shared/create_debugging_comment';
 import { get_context_merger } from '../shared/get_context_merger';
+import bind_this from '../shared/bind_this';
 
 const events = [
 	{
@@ -540,38 +541,9 @@ export default class ElementWrapper extends Wrapper {
 
 		const this_binding = this.bindings.find(b => b.node.name === 'this');
 		if (this_binding) {
-			const name = renderer.component.get_unique_name(`${this.var}_binding`);
+			const binding_callback = bind_this(renderer.component, block, this_binding.node, this.var);
 
-			renderer.component.add_var({
-				name,
-				internal: true,
-				referenced: true
-			});
-
-			const { handler, object } = this_binding;
-
-			const args = [];
-			for (const arg of handler.contextual_dependencies) {
-				args.push(arg);
-				block.add_variable(arg, `ctx.${arg}`);
-			}
-
-			renderer.component.partly_hoisted.push(deindent`
-				function ${name}(${['$$node', 'check'].concat(args).join(', ')}) {
-					${handler.snippet ? `if ($$node || (!$$node && ${handler.snippet} === check)) ` : ''}${handler.mutation}
-					${renderer.component.invalidate(object)};
-				}
-			`);
-
-			block.builders.mount.add_line(`@add_binding_callback(() => ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}));`);
-			block.builders.destroy.add_line(`ctx.${name}(${['null', this.var].concat(args).join(', ')});`);
-			block.builders.update.add_line(deindent`
-				if (changed.items) {
-					ctx.${name}(${['null', this.var].concat(args).join(', ')});
-					${args.map(a => `${a} = ctx.${a}`).join(', ')};
-					ctx.${name}(${[this.var, 'null'].concat(args).join(', ')});
-				}`
-			);
+			block.builders.mount.add_line(binding_callback);
 		}
 	}
 

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

@@ -9,11 +9,11 @@ import add_to_set from '../../../utils/add_to_set';
 import deindent from '../../../utils/deindent';
 import Attribute from '../../../nodes/Attribute';
 import get_object from '../../../utils/get_object';
-import flatten_reference from '../../../utils/flatten_reference';
 import create_debugging_comment from '../shared/create_debugging_comment';
 import { get_context_merger } from '../shared/get_context_merger';
 import EachBlock from '../../../nodes/EachBlock';
 import TemplateScope from '../../../nodes/shared/TemplateScope';
+import bind_this from '../shared/bind_this';
 
 export default class InlineComponentWrapper extends Wrapper {
 	var: string;
@@ -252,41 +252,7 @@ export default class InlineComponentWrapper extends Wrapper {
 			component.has_reactive_assignments = true;
 
 			if (binding.name === 'this') {
-				const fn = component.get_unique_name(`${this.var}_binding`);
-
-				component.add_var({
-					name: fn,
-					internal: true,
-					referenced: true
-				});
-
-				let lhs;
-				let object;
-
-				if (binding.is_contextual && binding.expression.node.type === 'Identifier') {
-					// bind:x={y} — we can't just do `y = x`, we need to
-					// to `array[index] = x;
-					const { name } = binding.expression.node;
-					const { snippet } = block.bindings.get(name);
-					lhs = snippet;
-
-					// TODO we need to invalidate... something
-				} else {
-					object = flatten_reference(binding.expression.node).name;
-					lhs = component.source.slice(binding.expression.node.start, binding.expression.node.end).trim();
-				}
-
-				const contextual_dependencies = [...binding.expression.contextual_dependencies];
-
-				component.partly_hoisted.push(deindent`
-					function ${fn}(${['$$component', ...contextual_dependencies].join(', ')}) {
-						${lhs} = $$component;
-						${object && component.invalidate(object)}
-					}
-				`);
-
-				block.builders.destroy.add_line(`ctx.${fn}(null);`);
-				return `@add_binding_callback(() => ctx.${fn}(${[this.var, ...contextual_dependencies.map(name => `ctx.${name}`)].join(', ')}));`;
+				return bind_this(component, block, binding, this.var);
 			}
 
 			const name = component.get_unique_name(`${this.var}_${binding.name}_binding`);

src/compiler/compile/render_dom/wrappers/shared/bind_this.ts

@@ -0,0 +1,80 @@
+import flatten_reference from '../../../utils/flatten_reference';
+import deindent from '../../../utils/deindent';
+import Component from '../../../Component';
+import Block from '../../Block';
+import Binding from '../../../nodes/Binding';
+
+export default function bind_this(component: Component, block: Block, binding: Binding, variable: string) {
+	const fn = component.get_unique_name(`${variable}_binding`);
+
+	component.add_var({
+		name: fn,
+		internal: true,
+		referenced: true
+	});
+
+	let lhs;
+	let object;
+
+	if (binding.is_contextual && binding.expression.node.type === 'Identifier') {
+		// bind:x={y} — we can't just do `y = x`, we need to
+		// to `array[index] = x;
+		const { name } = binding.expression.node;
+		const { snippet } = block.bindings.get(name);
+		lhs = snippet;
+
+		// TODO we need to invalidate... something
+	} else {
+		object = flatten_reference(binding.expression.node).name;
+		lhs = component.source.slice(binding.expression.node.start, binding.expression.node.end).trim();
+	}
+
+	const contextual_dependencies = Array.from(binding.expression.contextual_dependencies);
+
+	if (contextual_dependencies.length) {
+		component.partly_hoisted.push(deindent`
+			function ${fn}(${['$$value', ...contextual_dependencies].join(', ')}) {
+				if (${lhs} === $$value) return;
+				${lhs} = $$value;
+				${object && component.invalidate(object)}
+			}
+		`);
+
+		const args = [];
+		for (const arg of contextual_dependencies) {
+			args.push(arg);
+			block.add_variable(arg, `ctx.${arg}`);
+		}
+
+		const assign = block.get_unique_name(`assign_${variable}`);
+		const unassign = block.get_unique_name(`unassign_${variable}`);
+
+		block.builders.init.add_block(deindent`
+			const ${assign} = () => ctx.${fn}(${[variable].concat(args).join(', ')});
+			const ${unassign} = () => ctx.${fn}(${['null'].concat(args).join(', ')});
+		`);
+
+		const condition = Array.from(contextual_dependencies).map(name => `${name} !== ctx.${name}`).join(' || ');
+
+		block.builders.update.add_line(deindent`
+			if (${condition}) {
+				${unassign}();
+				${args.map(a => `${a} = ctx.${a}`).join(', ')};
+				@add_binding_callback(${assign});
+			}`
+		);
+
+		block.builders.destroy.add_line(`${unassign}();`);
+		return `@add_binding_callback(${assign});`;
+	}
+
+	component.partly_hoisted.push(deindent`
+		function ${fn}($$value) {
+			${lhs} = $$value;
+			${object && component.invalidate(object)}
+		}
+	`);
+
+	block.builders.destroy.add_line(`ctx.${fn}(null);`);
+	return `@add_binding_callback(() => ctx.${fn}(${variable}));`;
+}

src/runtime/internal/scheduler.ts

@@ -46,7 +46,7 @@ export function flush() {
 			update(component.$$);
 		}
 
-		while (binding_callbacks.length) binding_callbacks.shift()();
+		while (binding_callbacks.length) binding_callbacks.pop()();
 
 		// then, once components are updated, call
 		// afterUpdate functions. This may cause

test/runtime/samples/binding-this-each-block-property-component/Foo.svelte

@@ -0,0 +1,7 @@
+<script>
+	export function isFoo() {
+		return true;
+	}
+</script>
+
+<p><slot></slot></p>

test/runtime/samples/binding-this-each-block-property-component/_config.js

@@ -0,0 +1,12 @@
+export default {
+	html: ``,
+
+	async test({ assert, component, target }) {
+		component.visible = true;
+		assert.htmlEqual(target.innerHTML, `
+			<p>a</p>
+		`);
+
+		assert.ok(component.items[0].ref.isFoo());
+	}
+};

test/runtime/samples/binding-this-each-block-property-component/main.svelte

@@ -0,0 +1,13 @@
+<script>
+	import Foo from './Foo.svelte';
+
+	export let visible = false;
+
+	export let items = [{ value: 'a', ref: null }];
+</script>
+
+{#if visible}
+	{#each items as item}
+		<Foo bind:this={item.ref}>{item.value}</Foo>
+	{/each}
+{/if}

test/runtime/samples/binding-this-each-block-property/_config.js

@@ -0,0 +1,12 @@
+export default {
+	html: ``,
+
+	async test({ assert, component, target }) {
+		component.visible = true;
+		assert.htmlEqual(target.innerHTML, `
+			<div>a</div>
+		`);
+
+		assert.equal(component.items[0].ref, target.querySelector('div'));
+	}
+};

test/runtime/samples/binding-this-each-block-property/main.svelte

@@ -0,0 +1,11 @@
+<script>
+	export let visible = false;
+
+	export let items = [{ value: 'a', ref: null }];
+</script>
+
+{#if visible}
+	{#each items as item}
+		<div bind:this={item.ref}>{item.value}</div>
+	{/each}
+{/if}