fix bind:group in each (#4868)

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Svelte changelog
 
+## Unreleased
+
+* Fix `bind:group` inside `{#each}` ([#3243](https://github.com/sveltejs/svelte/issues/3243))
+
 ## 3.23.1
 
 * Fix checkbox `bind:group` when multiple options have the same value ([#4397](https://github.com/sveltejs/svelte/issues/4397))

src/compiler/compile/nodes/EachBlock.ts

@@ -21,6 +21,7 @@ export default class EachBlock extends AbstractBlock {
 	contexts: Context[];
 	has_animation: boolean;
 	has_binding = false;
+	has_index_binding = false;
 
 	else?: ElseBlock;
 

src/compiler/compile/render_dom/Renderer.ts

@@ -32,7 +32,7 @@ export default class Renderer {
 	blocks: Array<Block | Node | Node[]> = [];
 	readonly: Set<string> = new Set();
 	meta_bindings: Array<Node | Node[]> = []; // initial values for e.g. window.innerWidth, if there's a <svelte:window> meta tag
-	binding_groups: string[] = [];
+	binding_groups: Map<string, { binding_group: (to_reference?: boolean) => Node; is_context: boolean; contexts: string[]; index: number }> = new Map();
 
 	block: Block;
 	fragment: FragmentWrapper;
@@ -63,7 +63,7 @@ export default class Renderer {
 			this.add_to_context('$$slots');
 		}
 
-		if (this.binding_groups.length > 0) {
+		if (this.binding_groups.size > 0) {
 			this.add_to_context('$$binding_groups');
 		}
 

src/compiler/compile/render_dom/index.ts

@@ -416,7 +416,7 @@ export default function dom(
 				${component.slots.size || component.compile_options.dev ? b`let { $$slots = {}, $$scope } = $$props;` : null}
 				${component.compile_options.dev && b`@validate_slots('${component.tag}', $$slots, [${[...component.slots.keys()].map(key => `'${key}'`).join(',')}]);`}
 
-				${renderer.binding_groups.length > 0 && b`const $$binding_groups = [${renderer.binding_groups.map(_ => x`[]`)}];`}
+				${renderer.binding_groups.size > 0 && b`const $$binding_groups = [${[...renderer.binding_groups.keys()].map(_ => x`[]`)}];`}
 
 				${component.partly_hoisted}
 

src/compiler/compile/render_dom/wrappers/EachBlock.ts

@@ -201,7 +201,7 @@ export default class EachBlockWrapper extends Wrapper {
 		this.context_props = this.node.contexts.map(prop => b`child_ctx[${renderer.context_lookup.get(prop.key.name).index}] = ${prop.modifier(x`list[i]`)};`);
 
 		if (this.node.has_binding) this.context_props.push(b`child_ctx[${renderer.context_lookup.get(this.vars.each_block_value.name).index}] = list;`);
-		if (this.node.has_binding || this.node.index) this.context_props.push(b`child_ctx[${renderer.context_lookup.get(this.index_name.name).index}] = i;`);
+		if (this.node.has_binding || this.node.has_index_binding || this.node.index) this.context_props.push(b`child_ctx[${renderer.context_lookup.get(this.index_name.name).index}] = i;`);
 
 		const snippet = this.node.expression.manipulate(block);
 

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

@@ -7,8 +7,9 @@ import replace_object from '../../../utils/replace_object';
 import Block from '../../Block';
 import Renderer from '../../Renderer';
 import flatten_reference from '../../../utils/flatten_reference';
-import EachBlock from '../../../nodes/EachBlock';
 import { Node, Identifier } from 'estree';
+import add_to_set from '../../../utils/add_to_set';
+import mark_each_block_bindings from '../shared/mark_each_block_bindings';
 
 export default class BindingWrapper {
 	node: Binding;
@@ -42,12 +43,7 @@ export default class BindingWrapper {
 		}
 
 		if (node.is_contextual) {
-			// we need to ensure that the each block creates a context including
-			// the list and the index, if they're not otherwise referenced
-			const { name } = get_object(this.node.expression.node);
-			const each_block = this.parent.node.scope.get_owner(name);
-
-			(each_block as EachBlock).has_binding = true;
+			mark_each_block_bindings(this.parent, this.node);
 		}
 
 		this.object = get_object(this.node.expression.node).name;
@@ -123,17 +119,31 @@ export default class BindingWrapper {
 		switch (this.node.name) {
 			case 'group':
 			{
-				const binding_group = get_binding_group(parent.renderer, this.node.expression.node);
+				const { binding_group, is_context, contexts, index } = get_binding_group(parent.renderer, this.node, block);
 
 				block.renderer.add_to_context(`$$binding_groups`);
-				const reference = block.renderer.reference(`$$binding_groups`);
+
+				if (is_context) {
+					if (contexts.length > 1) {
+						let binding_group = x`${block.renderer.reference('$$binding_groups')}[${index}]`;
+						for (const name of contexts.slice(0, -1)) {
+							binding_group = x`${binding_group}[${block.renderer.reference(name)}]`;
+							block.chunks.init.push(
+								b`${binding_group} = ${binding_group} || [];`
+							);
+						}
+					}
+					block.chunks.init.push(
+						b`${binding_group(true)} = [];`
+					);
+				}
 
 				block.chunks.hydrate.push(
-					b`${reference}[${binding_group}].push(${parent.var});`
+					b`${binding_group(true)}.push(${parent.var});`
 				);
 
 				block.chunks.destroy.push(
-					b`${reference}[${binding_group}].splice(${reference}[${binding_group}].indexOf(${parent.var}), 1);`
+					b`${binding_group(true)}.splice(${binding_group(true)}.indexOf(${parent.var}), 1);`
 				);
 				break;
 			}
@@ -245,19 +255,61 @@ function get_dom_updater(
 	return b`${element.var}.${binding.node.name} = ${binding.snippet};`;
 }
 
-function get_binding_group(renderer: Renderer, value: Node) {
-	const { parts } = flatten_reference(value); // TODO handle cases involving computed member expressions
-	const keypath = parts.join('.');
+function get_binding_group(renderer: Renderer, value: Binding, block: Block) {
+	const { parts } = flatten_reference(value.raw_expression);
+	let keypath = parts.join('.');
+
+	const contexts = [];
+
+	for (const dep of value.expression.contextual_dependencies) {
+		const context = block.bindings.get(dep);
+		let key;
+		let name;
+		if (context) {
+			key = context.object.name;
+			name = context.property.name;
+		} else {
+			key = dep;
+			name = dep;
+		}
+		keypath = `${key}@${keypath}`;
+		contexts.push(name);
+	}
+
+	if (!renderer.binding_groups.has(keypath)) {
+		const index = renderer.binding_groups.size;
+
+		contexts.forEach(context => {
+			renderer.add_to_context(context, true);
+		});
 
-	// TODO handle contextual bindings — `keypath` should include unique ID of
-	// each block that provides context
-	let index = renderer.binding_groups.indexOf(keypath);
-	if (index === -1) {
-		index = renderer.binding_groups.length;
-		renderer.binding_groups.push(keypath);
+		renderer.binding_groups.set(keypath, {
+			binding_group: (to_reference: boolean = false) => {
+				let binding_group = '$$binding_groups';
+				let _secondary_indexes = contexts;
+
+				if (to_reference) {
+					binding_group = block.renderer.reference(binding_group);
+					_secondary_indexes = _secondary_indexes.map(name => block.renderer.reference(name));
+				}
+
+				if (_secondary_indexes.length > 0) {
+					let obj = x`${binding_group}[${index}]`;
+					_secondary_indexes.forEach(secondary_index => {
+						obj = x`${obj}[${secondary_index}]`;
+					});
+					return obj;
+				} else {
+					return x`${binding_group}[${index}]`;
+				}
+			},
+			is_context: contexts.length > 0,
+			contexts,
+			index,
+		});
 	}
 
-	return index;
+	return renderer.binding_groups.get(keypath);
 }
 
 function get_event_handler(
@@ -295,7 +347,7 @@ function get_event_handler(
 		}
 	}
 
-	const value = get_value_from_dom(renderer, binding.parent, binding);
+	const value = get_value_from_dom(renderer, binding.parent, binding, block, contextual_dependencies);
 
 	const mutation = b`
 		${lhs} = ${value};
@@ -313,7 +365,9 @@ function get_event_handler(
 function get_value_from_dom(
 	renderer: Renderer,
 	element: ElementWrapper | InlineComponentWrapper,
-	binding: BindingWrapper
+	binding: BindingWrapper,
+	block: Block,
+	contextual_dependencies: Set<string>
 ) {
 	const { node } = element;
 	const { name } = binding.node;
@@ -333,9 +387,10 @@ function get_value_from_dom(
 
 	// <input type='checkbox' bind:group='foo'>
 	if (name === 'group') {
-		const binding_group = get_binding_group(renderer, binding.node.expression.node);
 		if (type === 'checkbox') {
-			return x`@get_binding_group_value($$binding_groups[${binding_group}], this.__value, this.checked)`;
+			const { binding_group, contexts } = get_binding_group(renderer, binding.node, block);
+			add_to_set(contextual_dependencies, contexts);
+			return x`@get_binding_group_value(${binding_group()}, this.__value, this.checked)`;
 		}
 
 		return x`this.__value`;

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

@@ -8,16 +8,15 @@ import { sanitize } from '../../../../utils/names';
 import add_to_set from '../../../utils/add_to_set';
 import { b, x, p } from 'code-red';
 import Attribute from '../../../nodes/Attribute';
-import get_object from '../../../utils/get_object';
 import create_debugging_comment from '../shared/create_debugging_comment';
 import { get_slot_definition } from '../shared/get_slot_definition';
-import EachBlock from '../../../nodes/EachBlock';
 import TemplateScope from '../../../nodes/shared/TemplateScope';
 import is_dynamic from '../shared/is_dynamic';
 import bind_this from '../shared/bind_this';
 import { Node, Identifier, ObjectExpression } from 'estree';
 import EventHandler from '../Element/EventHandler';
 import { extract_names } from 'periscopic';
+import mark_each_block_bindings from '../shared/mark_each_block_bindings';
 
 export default class InlineComponentWrapper extends Wrapper {
 	var: Identifier;
@@ -48,12 +47,7 @@ export default class InlineComponentWrapper extends Wrapper {
 
 		this.node.bindings.forEach(binding => {
 			if (binding.is_contextual) {
-				// we need to ensure that the each block creates a context including
-				// the list and the index, if they're not otherwise referenced
-				const { name } = get_object(binding.expression.node);
-				const each_block = this.node.scope.get_owner(name);
-
-				(each_block as EachBlock).has_binding = true;
+				mark_each_block_bindings(this, binding);
 			}
 
 			block.add_dependencies(binding.expression.dependencies);

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

@@ -0,0 +1,24 @@
+import EachBlock from "../../../nodes/EachBlock";
+import InlineComponentWrapper from "../InlineComponent";
+import ElementWrapper from "../Element";
+import Binding from "../../../nodes/Binding";
+import get_object from "../../../utils/get_object";
+
+export default function mark_each_block_bindings(
+	parent: ElementWrapper | InlineComponentWrapper,
+	binding: Binding
+) {
+	// we need to ensure that the each block creates a context including
+	// the list and the index, if they're not otherwise referenced
+	const object = get_object(binding.expression.node).name;
+	const each_block = parent.node.scope.get_owner(object);
+	(each_block as EachBlock).has_binding = true;
+
+	if (binding.name === "group") {
+		// for `<input bind:group={} >`, we make sure that all the each blocks creates context with `index`
+		for (const name of binding.expression.contextual_dependencies) {
+			const each_block = parent.node.scope.get_owner(name);
+			(each_block as EachBlock).has_index_binding = true;
+		}
+	}
+}

src/compiler/compile/utils/flatten_reference.ts

@@ -3,14 +3,18 @@ import { Node, Identifier } from 'estree';
 export default function flatten_reference(node: Node) {
 	const nodes = [];
 	const parts = [];
-
+	
 	while (node.type === 'MemberExpression') {
 		nodes.unshift(node.property);
 
 		if (!node.computed) {
 			parts.unshift((node.property as Identifier).name);
+		} else {
+			const computed_property = to_string(node.property);
+			if (computed_property) {
+				parts.unshift(`[${computed_property}]`);
+			}
 		}
-
 		node = node.object;
 	}
 
@@ -20,9 +24,16 @@ export default function flatten_reference(node: Node) {
 
 	nodes.unshift(node);
 
-	if (!(node as any).computed) {
-		parts.unshift(name);
-	}
+	parts.unshift(name);
 
 	return { name, nodes, parts };
 }
+
+function to_string(node: Node) {
+	switch (node.type) {
+		case 'Literal':
+			return String(node.value);
+		case 'Identifier':
+			return node.name;
+	}
+}