update bind:this references when setting to null - fixes #2034

Author: Rich-Harris

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

@@ -324,7 +324,7 @@ export default class InlineComponentWrapper extends Wrapper {
 
 			component.partly_hoisted.push(body);
 
-			return `@add_binding_callback(() => @bind(${this.var}, '${binding.name}', ${name}));`;
+			return `@binding_callbacks.push(() => @bind(${this.var}, '${binding.name}', ${name}));`;
 		});
 
 		const munged_handlers = this.node.handlers.map(handler => {

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

@@ -15,6 +15,7 @@ export default function bind_this(component: Component, block: Block, binding: B
 
 	let lhs;
 	let object;
+	let body;
 
 	if (binding.is_contextual && binding.expression.node.type === 'Identifier') {
 		// bind:x={y} — we can't just do `y = x`, we need to
@@ -23,10 +24,19 @@ export default function bind_this(component: Component, block: Block, binding: B
 		const { snippet } = block.bindings.get(name);
 		lhs = snippet;
 
-		// TODO we need to invalidate... something
+		body = `${lhs} = $$value`; // 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();
+
+		body = binding.expression.node.type === 'Identifier'
+			? deindent`
+				${component.invalidate(object, `${lhs} = $$value`)};
+			`
+			: deindent`
+				${lhs} = $$value;
+				${component.invalidate(object)};
+			`
 	}
 
 	const contextual_dependencies = Array.from(binding.expression.contextual_dependencies);
@@ -35,8 +45,9 @@ export default function bind_this(component: Component, block: Block, binding: B
 		component.partly_hoisted.push(deindent`
 			function ${fn}(${['$$value', ...contextual_dependencies].join(', ')}) {
 				if (${lhs} === $$value) return;
-				${lhs} = $$value;
-				${object && component.invalidate(object)}
+				@binding_callbacks[$$value ? 'unshift' : 'push'](() => {
+					${body}
+				});
 			}
 		`);
 
@@ -56,25 +67,29 @@ export default function bind_this(component: Component, block: Block, binding: B
 
 		const condition = Array.from(contextual_dependencies).map(name => `${name} !== ctx.${name}`).join(' || ');
 
+		// we push unassign and unshift assign so that references are
+		// nulled out before they're created, to avoid glitches
+		// with shifting indices
 		block.builders.update.add_line(deindent`
 			if (${condition}) {
 				${unassign}();
 				${args.map(a => `${a} = ctx.${a}`).join(', ')};
-				@add_binding_callback(${assign});
+				${assign}();
 			}`
 		);
 
 		block.builders.destroy.add_line(`${unassign}();`);
-		return `@add_binding_callback(${assign});`;
+		return `${assign}();`;
 	}
 
 	component.partly_hoisted.push(deindent`
 		function ${fn}($$value) {
-			${lhs} = $$value;
-			${object && component.invalidate(object)}
+			@binding_callbacks[$$value ? 'unshift' : 'push'](() => {
+				${body}
+			});
 		}
 	`);
 
 	block.builders.destroy.add_line(`ctx.${fn}(null);`);
-	return `@add_binding_callback(() => ctx.${fn}(${variable}));`;
+	return `ctx.${fn}(${variable});`;
 }

src/runtime/internal/scheduler.ts

@@ -4,12 +4,13 @@ import { set_current_component } from './lifecycle';
 export const dirty_components = [];
 export const intros = { enabled: false };
 
-const resolved_promise = Promise.resolve();
-let update_scheduled = false;
-const binding_callbacks = [];
+export const binding_callbacks = [];
 const render_callbacks = [];
 const flush_callbacks = [];
 
+const resolved_promise = Promise.resolve();
+let update_scheduled = false;
+
 export function schedule_update() {
 	if (!update_scheduled) {
 		update_scheduled = true;
@@ -22,10 +23,6 @@ export function tick() {
 	return resolved_promise;
 }
 
-export function add_binding_callback(fn) {
-	binding_callbacks.push(fn);
-}
-
 export function add_render_callback(fn) {
 	render_callbacks.push(fn);
 }

test/runtime/index.js

@@ -98,12 +98,10 @@ describe("runtime", () => {
 					};
 					set_now(() => raf.time);
 					set_raf(cb => {
-						let called = false;
 						raf.callback = () => {
-							if (!called) {
-								called = true;
-								cb();
-							}
+							raf.callback = null;
+							cb();
+							flush();
 						};
 					});
 

test/runtime/samples/binding-this-element-reactive-b/_config.js

@@ -0,0 +1,21 @@
+export default {
+	skip_if_ssr: true,
+
+	html: `
+		<div>The text is hello</div>
+		<h1>hello</h1>
+	`,
+
+	test({ assert, component, target }) {
+		component.visible = false;
+		assert.htmlEqual(target.innerHTML, `
+			<div>The text is missing</div>
+		`);
+
+		component.visible = true;
+		assert.htmlEqual(target.innerHTML, `
+			<div>The text is hello</div>
+			<h1>hello</h1>
+		`);
+	}
+};

test/runtime/samples/binding-this-element-reactive-b/main.svelte

@@ -0,0 +1,9 @@
+<script>
+	export let visible = true;
+	let h1;
+</script>
+
+<div>The text is {h1 ? h1.textContent : 'missing'}</div>
+{#if visible}
+	<h1 bind:this={h1}>hello</h1>
+{/if}

test/runtime/samples/transition-js-if-block-outro-timeout/_config.js

@@ -8,7 +8,6 @@ export default {
 
 		raf.tick(200);
 		assert.equal(window.getComputedStyle(div).opacity, 0.5);
-		component.blabla = false;
 
 		raf.tick(400);
 		assert.equal(window.getComputedStyle(div).opacity, 0);