fix binding for each block local variable (#4861)

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Svelte changelog
 
+## Unreleased
+
+* Fix `bind:this` to the value of an `{#each}` block ([#4517](https://github.com/sveltejs/svelte/issues/4517))
+* Fix binding to contextual `{#each}` values that shadow outer names ([#4757](https://github.com/sveltejs/svelte/issues/4757))
+
 ## 3.23.0
 
 * Update `<select>` with `bind:value` when the available `<option>`s change ([#1764](https://github.com/sveltejs/svelte/issues/1764))

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

@@ -1,7 +1,9 @@
 import { b, x } from 'code-red';
 import Binding from '../../../nodes/Binding';
 import ElementWrapper from '../Element';
+import InlineComponentWrapper from '../InlineComponent';
 import get_object from '../../../utils/get_object';
+import replace_object from '../../../utils/replace_object';
 import Block from '../../Block';
 import Renderer from '../../Renderer';
 import flatten_reference from '../../../utils/flatten_reference';
@@ -10,20 +12,20 @@ import { Node, Identifier } from 'estree';
 
 export default class BindingWrapper {
 	node: Binding;
-	parent: ElementWrapper;
+	parent: ElementWrapper | InlineComponentWrapper;
 
 	object: string;
 	handler: {
 		uses_context: boolean;
 		mutation: (Node | Node[]);
 		contextual_dependencies: Set<string>;
-		snippet?: Node;
+		lhs?: Node;
 	};
 	snippet: Node;
 	is_readonly: boolean;
 	needs_lock: boolean;
 
-	constructor(block: Block, node: Binding, parent: ElementWrapper) {
+	constructor(block: Block, node: Binding, parent: ElementWrapper | InlineComponentWrapper) {
 		this.node = node;
 		this.parent = parent;
 
@@ -33,7 +35,7 @@ export default class BindingWrapper {
 
 		// TODO does this also apply to e.g. `<input type='checkbox' bind:group='foo'>`?
 		if (parent.node.name === 'select') {
-			parent.select_binding_dependencies = dependencies;
+			(parent as ElementWrapper).select_binding_dependencies = dependencies;
 			dependencies.forEach((prop: string) => {
 				parent.renderer.component.indirect_dependencies.set(prop, new Set());
 			});
@@ -207,7 +209,7 @@ export default class BindingWrapper {
 }
 
 function get_dom_updater(
-	element: ElementWrapper,
+	element: ElementWrapper | InlineComponentWrapper,
 	binding: BindingWrapper
 ) {
 	const { node } = element;
@@ -270,21 +272,17 @@ function get_event_handler(
 	contextual_dependencies: Set<string>;
 	lhs?: Node;
 } {
-	const value = get_value_from_dom(renderer, binding.parent, binding);
+	const contextual_dependencies = new Set<string>(binding.node.expression.contextual_dependencies);
-	const contextual_dependencies = new Set(binding.node.expression.contextual_dependencies);
 
 	const context = block.bindings.get(name);
 	let set_store;
 
 	if (context) {
-		const { object, property, modifier, store } = context;
+		const { object, property, store, snippet } = context;
-
+		lhs = replace_object(lhs, snippet);
-		if (lhs.type === 'Identifier') {
-			lhs = modifier(x`${object}[${property}]`);
-
 		contextual_dependencies.add(object.name);
 		contextual_dependencies.add(property.name);
-		}
+		contextual_dependencies.delete(name);
 
 		if (store) {
 			set_store = b`${store}.set(${`$${store}`});`;
@@ -297,6 +295,8 @@ function get_event_handler(
 		}
 	}
 
+	const value = get_value_from_dom(renderer, binding.parent, binding);
+
 	const mutation = b`
 		${lhs} = ${value};
 		${set_store}
@@ -305,20 +305,21 @@ function get_event_handler(
 	return {
 		uses_context: binding.node.is_contextual || binding.node.expression.uses_context, // TODO this is messy
 		mutation,
-		contextual_dependencies
+		contextual_dependencies,
+		lhs,
 	};
 }
 
 function get_value_from_dom(
 	renderer: Renderer,
-	element: ElementWrapper,
+	element: ElementWrapper | InlineComponentWrapper,
 	binding: BindingWrapper
 ) {
 	const { node } = element;
 	const { name } = binding.node;
 
 	if (name === 'this') {
-		return x`$$node`;
+		return x`$$value`;
 	}
 
 	// <select bind:value='selected>

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

@@ -27,6 +27,11 @@ import EventHandler from './EventHandler';
 import { extract_names } from 'periscopic';
 import Action from '../../../nodes/Action';
 
+interface BindingGroup {
+	events: string[];
+	bindings: Binding[];
+}
+
 const events = [
 	{
 		event_names: ['input'],
@@ -436,14 +441,9 @@ export default class ElementWrapper extends Wrapper {
 	}
 
 	add_directives_in_order (block: Block) {
-		interface BindingGroup {
-			events: string[];
-			bindings: Binding[];
-		}
-
 		type OrderedAttribute = EventHandler | BindingGroup | Binding | Action;
 
-		const bindingGroups = events
+		const binding_groups = events
 			.map(event => ({
 				events: event.event_names,
 				bindings: this.bindings
@@ -467,7 +467,7 @@ export default class ElementWrapper extends Wrapper {
 		}
 
 		([
-			...bindingGroups,
+			...binding_groups,
 			...this.event_handlers,
 			this_binding,
 			...this.node.actions
@@ -487,33 +487,31 @@ export default class ElementWrapper extends Wrapper {
 			});
 	}
 
-	add_bindings(block: Block, bindingGroup) {
+	add_bindings(block: Block, binding_group: BindingGroup) {
 		const { renderer } = this;
 
-		if (bindingGroup.bindings.length === 0) return;
+		if (binding_group.bindings.length === 0) return;
 
 		renderer.component.has_reactive_assignments = true;
 
-		const lock = bindingGroup.bindings.some(binding => binding.needs_lock) ?
+		const lock = binding_group.bindings.some(binding => binding.needs_lock) ?
 			block.get_unique_name(`${this.var.name}_updating`) :
 			null;
 
 		if (lock) block.add_variable(lock, x`false`);
 
-		[bindingGroup].forEach(group => {
+		const handler = renderer.component.get_unique_name(`${this.var.name}_${binding_group.events.join('_')}_handler`);
-			const handler = renderer.component.get_unique_name(`${this.var.name}_${group.events.join('_')}_handler`);
 		renderer.add_to_context(handler.name);
 
 		// TODO figure out how to handle locks
-			const needs_lock = group.bindings.some(binding => binding.needs_lock);
+		const needs_lock = binding_group.bindings.some(binding => binding.needs_lock);
 
 		const dependencies: Set<string> = new Set();
 		const contextual_dependencies: Set<string> = new Set();
 
-			group.bindings.forEach(binding => {
+		binding_group.bindings.forEach(binding => {
 			// TODO this is a mess
 			add_to_set(dependencies, binding.get_dependencies());
-				add_to_set(contextual_dependencies, binding.node.expression.contextual_dependencies);
 			add_to_set(contextual_dependencies, binding.handler.contextual_dependencies);
 
 			binding.render(block, lock);
@@ -523,7 +521,7 @@ export default class ElementWrapper extends Wrapper {
 		// fire too infrequently, so we need to take matters into our
 		// own hands
 		let animation_frame;
-			if (group.events[0] === 'timeupdate') {
+		if (binding_group.events[0] === 'timeupdate') {
 			animation_frame = block.get_unique_name(`${this.var.name}_animationframe`);
 			block.add_variable(animation_frame);
 		}
@@ -567,7 +565,7 @@ export default class ElementWrapper extends Wrapper {
 
 		this.renderer.component.partly_hoisted.push(b`
 			function ${handler}(${params}) {
-					${group.bindings.map(b => b.handler.mutation)}
+				${binding_group.bindings.map(b => b.handler.mutation)}
 				${Array.from(dependencies)
 					.filter(dep => dep[0] !== '$')
 					.filter(dep => !contextual_dependencies.has(dep))
@@ -575,7 +573,7 @@ export default class ElementWrapper extends Wrapper {
 			}
 		`);
 
-			group.events.forEach(name => {
+		binding_group.events.forEach(name => {
 			if (name === 'elementresize') {
 				// special case
 				const resize_listener = block.get_unique_name(`${this.var.name}_resize_listener`);
@@ -595,13 +593,13 @@ export default class ElementWrapper extends Wrapper {
 			}
 		});
 
-			const some_initial_state_is_undefined = group.bindings
+		const some_initial_state_is_undefined = binding_group.bindings
 			.map(binding => x`${binding.snippet} === void 0`)
 			.reduce((lhs, rhs) => x`${lhs} || ${rhs}`);
 
 		const should_initialise = (
 			this.node.name === 'select' ||
-				group.bindings.find(binding => {
+			binding_group.bindings.find(binding => {
 				return (
 					binding.node.name === 'indeterminate' ||
 					binding.node.name === 'textContent' ||
@@ -618,12 +616,11 @@ export default class ElementWrapper extends Wrapper {
 			);
 		}
 
-			if (group.events[0] === 'elementresize') {
+		if (binding_group.events[0] === 'elementresize') {
 			block.chunks.hydrate.push(
 				b`@add_render_callback(() => ${callee}.call(${this.var}));`
 			);
 		}
-		});
 
 		if (lock) {
 			block.chunks.update.push(b`${lock} = false;`);
@@ -635,7 +632,7 @@ export default class ElementWrapper extends Wrapper {
 
 		renderer.component.has_reactive_assignments = true;
 
-		const binding_callback = bind_this(renderer.component, block, this_binding.node, this.var);
+		const binding_callback = bind_this(renderer.component, block, this_binding, this.var);
 		block.chunks.mount.push(binding_callback);
 	}
 

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

@@ -1,4 +1,5 @@
 import Wrapper from '../shared/Wrapper';
+import BindingWrapper from '../Element/Binding';
 import Renderer from '../../Renderer';
 import Block from '../../Block';
 import InlineComponent from '../../../nodes/InlineComponent';
@@ -309,7 +310,7 @@ export default class InlineComponentWrapper extends Wrapper {
 			component.has_reactive_assignments = true;
 
 			if (binding.name === 'this') {
-				return bind_this(component, block, binding, this.var);
+				return bind_this(component, block, new BindingWrapper(block, binding, this), this.var);
 			}
 
 			const id = component.get_unique_name(`${this.var.name}_${binding.name}_binding`);

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

@@ -1,50 +1,33 @@
-import flatten_reference from '../../../utils/flatten_reference';
 import { b, x } from 'code-red';
 import Component from '../../../Component';
 import Block from '../../Block';
-import Binding from '../../../nodes/Binding';
+import BindingWrapper from '../Element/Binding';
 import { Identifier } from 'estree';
 
-export default function bind_this(component: Component, block: Block, binding: Binding, variable: Identifier) {
+export default function bind_this(component: Component, block: Block, binding: BindingWrapper, variable: Identifier) {
 	const fn = component.get_unique_name(`${variable.name}_binding`);
 
 	block.renderer.add_to_context(fn.name);
 	const callee = block.renderer.reference(fn.name);
 
-	let lhs;
+	const { contextual_dependencies, mutation, lhs } = binding.handler;
-	let object;
+	const dependencies = binding.get_dependencies();
-	let body;
 
-	if (binding.is_contextual && binding.raw_expression.type === 'Identifier') {
+	const body = b`
-		// bind:x={y} — we can't just do `y = x`, we need to
+		${mutation}
-		// to `array[index] = x;
+		${Array.from(dependencies)
-		const { name } = binding.raw_expression;
+			.filter(dep => dep[0] !== '$')
-		const { snippet } = block.bindings.get(name);
+			.filter(dep => !contextual_dependencies.has(dep))
-		lhs = snippet;
+			.map(dep => b`${block.renderer.invalidate(dep)};`)}
-
-		body = b`${lhs} = $$value`; // TODO we need to invalidate... something
-	} else {
-		object = flatten_reference(binding.raw_expression).name;
-		lhs = binding.raw_expression;
-
-		body = binding.raw_expression.type === 'Identifier'
-			? b`
-				${block.renderer.invalidate(object, x`${lhs} = $$value`)};
-			`
-			: b`
-				${lhs} = $$value;
-				${block.renderer.invalidate(object)};
 	`;
-	}
 
-	const contextual_dependencies: Identifier[] = Array.from(binding.expression.contextual_dependencies).map(name => ({
+	if (contextual_dependencies.size) {
+		const params: Identifier[] = Array.from(contextual_dependencies).map(name => ({
 			type: 'Identifier',
 			name
 		}));
-
-	if (contextual_dependencies.length) {
 		component.partly_hoisted.push(b`
-			function ${fn}($$value, ${contextual_dependencies}) {
+			function ${fn}($$value, ${params}) {
 				if (${lhs} === $$value) return;
 				@binding_callbacks[$$value ? 'unshift' : 'push'](() => {
 					${body}
@@ -53,7 +36,7 @@ export default function bind_this(component: Component, block: Block, binding: B
 		`);
 
 		const args = [];
-		for (const id of contextual_dependencies) {
+		for (const id of params) {
 			args.push(id);
 			if (block.variables.has(id.name)) {
 				if (block.renderer.context_lookup.get(id.name).is_contextual) continue;
@@ -69,7 +52,7 @@ export default function bind_this(component: Component, block: Block, binding: B
 			const ${unassign} = () => ${callee}(null, ${args});
 		`);
 
-		const condition = Array.from(contextual_dependencies)
+		const condition = Array.from(params)
 			.map(name => x`${name} !== ${block.renderer.reference(name.name)}`)
 			.reduce((lhs, rhs) => x`${lhs} || ${rhs}`);
 

src/compiler/compile/utils/replace_object.ts

@@ -0,0 +1,14 @@
+import { Node } from 'estree';
+
+export default function replace_object(node: Node, replacement: Node) {
+	if (node.type === 'Identifier') return replacement;
+
+	const ancestor = node;
+	let parent;
+	while (node.type === 'MemberExpression') {
+		parent = node;
+		node = node.object;
+	}
+	parent.object = replacement;
+	return ancestor;
+}

test/js/samples/loop-protect/expected.js

@@ -116,7 +116,8 @@ function instance($$self, $$props, $$invalidate) {
 
 	function div_binding($$value) {
 		binding_callbacks[$$value ? "unshift" : "push"](() => {
-			$$invalidate(0, node = $$value);
+			node = $$value;
+			$$invalidate(0, node);
 		});
 	}
 

test/runtime/samples/each-block-scope-shadow-bind-2/_config.js

@@ -0,0 +1,23 @@
+export default {
+	html: `
+    Hello
+    <input />
+	`,
+	ssrHtml: `
+		Hello
+		<input value="Hello"/>
+	`,
+	async test({ assert, target, window }) {
+		const input = target.querySelector("input");
+		input.value = "abcd";
+		await input.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+        abcd
+        <input />
+      `
+		);
+	},
+};

test/runtime/samples/each-block-scope-shadow-bind-2/main.svelte

@@ -0,0 +1,10 @@
+<script>
+	let a = [
+		{ a: 'Hello' }
+	];
+</script>
+
+{#each a as { a }}
+	{a}
+	<input bind:value={a} />
+{/each}

test/runtime/samples/each-block-scope-shadow-bind-3/_config.js

@@ -0,0 +1,105 @@
+export default {
+	html: `
+		<div>
+			Hello World
+			<input />
+			<input />
+		</div>
+		<div>
+			Sapper App
+			<input />
+			<input />
+		</div>
+	`,
+
+	ssrHtml: `
+		<div>
+			Hello World
+			<input value="Hello"/>
+			<input value="World"/>
+		</div>
+		<div>
+			Sapper App
+			<input value="Sapper"/>
+			<input value="App"/>
+		</div>
+	`,
+	async test({ assert, target, window }) {
+		const [input1, input2, input3, input4] = target.querySelectorAll("input");
+		input1.value = "Awesome";
+		await input1.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<div>
+					Awesome World
+					<input />
+					<input />
+				</div>
+				<div>
+					Sapper App
+					<input />
+					<input />
+				</div>
+      `
+		);
+
+		input2.value = "Svelte";
+		await input2.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<div>
+					Awesome Svelte
+					<input />
+					<input />
+				</div>
+				<div>
+					Sapper App
+					<input />
+					<input />
+				</div>
+			`
+		);
+
+		input3.value = "Foo";
+		await input3.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<div>
+					Awesome Svelte
+					<input />
+					<input />
+				</div>
+				<div>
+					Foo App
+					<input />
+					<input />
+				</div>
+			`
+		);
+
+		input4.value = "Bar";
+		await input4.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<div>
+					Awesome Svelte
+					<input />
+					<input />
+				</div>
+				<div>
+					Foo Bar
+					<input />
+					<input />
+				</div>
+			`
+		);
+	},
+};

test/runtime/samples/each-block-scope-shadow-bind-3/main.svelte

@@ -0,0 +1,14 @@
+<script>
+	let a = [
+		['Hello', 'World'],
+		['Sapper', 'App'],
+	]
+</script>
+
+{#each a as a}
+	<div>
+		{a[0]} {a[1]}
+		<input bind:value={a[0]}>
+		<input bind:value={a[1]}>
+	</div>
+{/each}

test/runtime/samples/each-block-scope-shadow-bind-4/_config.js

@@ -0,0 +1,64 @@
+export default {
+	html: `
+		<div>
+			b: Hello
+			<input />
+		</div>
+		<button>Button</button>
+	`,
+	ssrHtml: `
+		<div>
+			b: Hello
+			<input value="Hello" />
+		</div>
+		<button>Button</button>
+	`,
+	async test({ assert, target, window }) {
+		const input = target.querySelector("input");
+		const button = target.querySelector("button");
+
+		input.value = "Awesome";
+		await input.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<div>
+					b: Awesome
+					<input />
+				</div>
+				<button>Button</button>
+      `
+		);
+
+
+		await button.dispatchEvent(new window.MouseEvent("click"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<div>
+					c: World
+					<input />
+				</div>
+				<button>Button</button>
+      `
+		);
+
+		assert.equal(input.value, 'World');
+
+		input.value = "Svelte";
+		await input.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<div>
+					c: Svelte
+					<input />
+				</div>
+				<button>Button</button>
+      `
+		);
+	},
+};

test/runtime/samples/each-block-scope-shadow-bind-4/main.svelte

@@ -0,0 +1,14 @@
+<script>
+	let a = [
+		{ a: { b: 'Hello', c: 'World' }, key: 'b' },
+	];
+</script>
+
+{#each a as { a, key }}
+	<div>
+		{key}: {a[key]}
+		<input bind:value={a[key]}>
+	</div>
+{/each}
+
+<button on:click={() => a[0].key = 'c'}>Button</button>

test/runtime/samples/each-block-scope-shadow-bind/_config.js

@@ -0,0 +1,23 @@
+export default {
+	html: `
+    Hello
+    <input />
+	`,
+	ssrHtml: `
+		Hello
+		<input value="Hello"/>
+	`,
+	async test({ assert, target, window }) {
+		const input = target.querySelector("input");
+		input.value = "abcd";
+		await input.dispatchEvent(new window.Event("input"));
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+        abcd
+        <input />
+      `
+		);
+	},
+};

test/runtime/samples/each-block-scope-shadow-bind/main.svelte

@@ -0,0 +1,10 @@
+<script>
+	let a = [
+		'Hello'
+	];
+</script>
+
+{#each a as a}
+	{a}
+	<input bind:value={a} />
+{/each}