feat: improve bind:group behavior (#7892)

track all `#each` variables that could result in a change to the inputs and also update the `$$binding_groups` variable which holds the references to the inputs of each group accordingly.

Fixes #7633
Fixes #6112
Fixes #7884

Author: tanhauhau

src/compiler/compile/render_dom/Block.ts

@@ -1,4 +1,4 @@
-import Renderer from './Renderer';
+import Renderer, { BindingGroup } from './Renderer';
 import Wrapper from './wrappers/shared/Wrapper';
 import { b, x } from 'code-red';
 import { Node, Identifier, ArrayPattern } from 'estree';
@@ -40,6 +40,7 @@ export default class Block {
 
 	bindings: Map<string, Bindings>;
 	binding_group_initialised: Set<string> = new Set();
+	binding_groups: Set<BindingGroup> = new Set();
 
 	chunks: {
 		declarations: Array<Node | Node[]>;
@@ -249,6 +250,7 @@ export default class Block {
 			}
 		}
 
+		this.render_binding_groups();
 		this.render_listeners();
 
 		const properties: Record<string, any> = {};
@@ -500,4 +502,10 @@ export default class Block {
 			}
 		}
 	}
+
+	render_binding_groups() {
+		for (const binding_group of this.binding_groups) {
+			binding_group.render();
+		}
+	}
 }

src/compiler/compile/render_dom/Renderer.ts

@@ -22,6 +22,15 @@ type BitMasks = Array<{
 	names: string[];
 }>;
 
+export interface BindingGroup {
+	binding_group: (to_reference?: boolean) => Node;
+	contexts: string[];
+	list_dependencies: Set<string>;
+	keypath: string;
+	elements: Identifier[];
+	render: () => void;
+}
+
 export default class Renderer {
 	component: Component; // TODO Maybe Renderer shouldn't know about Component?
 	options: CompileOptions;
@@ -33,7 +42,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: Map<string, { binding_group: (to_reference?: boolean) => Node; is_context: boolean; contexts: string[]; index: number; keypath: string }> = new Map();
+	binding_groups: Map<string, BindingGroup> = new Map();
 
 	block: Block;
 	fragment: FragmentWrapper;
@@ -64,10 +73,6 @@ export default class Renderer {
 			this.add_to_context('#slots');
 		}
 
-		if (this.binding_groups.size > 0) {
-			this.add_to_context('$$binding_groups');
-		}
-
 		// main block
 		this.block = new Block({
 			renderer: this,

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

@@ -85,6 +85,7 @@ export default class AttributeWrapper extends BaseAttributeWrapper {
 
 			if (node.name === 'value') {
 				handle_select_value_binding(this, node.dependencies);
+				this.parent.has_dynamic_value = true;
 			}
 		}
 
@@ -180,6 +181,17 @@ export default class AttributeWrapper extends BaseAttributeWrapper {
 			`;
 		}
 
+		if (this.node.name === 'value' && dependencies.length > 0) {
+			if (this.parent.bindings.some(binding => binding.node.name === 'group')) {
+				this.parent.dynamic_value_condition = block.get_unique_name('value_has_changed');
+				block.add_variable(this.parent.dynamic_value_condition, x`false`);
+				updater = b`
+					${updater}
+					${this.parent.dynamic_value_condition} = true;
+				`;
+			}
+		}
+
 		if (dependencies.length > 0) {
 			const condition = this.get_dom_update_conditions(block, block.renderer.dirty(dependencies));
 

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

@@ -5,7 +5,7 @@ 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 Renderer, { BindingGroup } from '../../Renderer';
 import flatten_reference from '../../../utils/flatten_reference';
 import { Node, Identifier } from 'estree';
 import add_to_set from '../../../utils/add_to_set';
@@ -26,6 +26,7 @@ export default class BindingWrapper {
 	snippet: Node;
 	is_readonly: boolean;
 	needs_lock: boolean;
+	binding_group: BindingGroup;
 
 	constructor(block: Block, node: Binding, parent: ElementWrapper | InlineComponentWrapper) {
 		this.node = node;
@@ -45,6 +46,10 @@ export default class BindingWrapper {
 
 		this.object = get_object(this.node.expression.node).name;
 
+		if (this.node.name === 'group') {
+			this.binding_group = get_binding_group(parent.renderer, this, block);
+		}
+
 		// view to model
 		this.handler = get_event_handler(this, parent.renderer, block, this.object, this.node.raw_expression);
 
@@ -67,6 +72,10 @@ export default class BindingWrapper {
 			}
 		});
 
+		if (this.binding_group) {
+			this.binding_group.list_dependencies.forEach(dep => dependencies.add(dep));
+		}
+
 		return dependencies;
 	}
 
@@ -105,6 +114,7 @@ export default class BindingWrapper {
 
 		const update_conditions: any[] = this.needs_lock ? [x`!${lock}`] : [];
 		const mount_conditions: any[] = [];
+		let update_or_condition: any = null;
 
 		const dependency_array = Array.from(this.get_dependencies());
 
@@ -142,33 +152,12 @@ export default class BindingWrapper {
 		switch (this.node.name) {
 			case 'group':
 			{
-				const { binding_group, is_context, contexts, index, keypath } = get_binding_group(parent.renderer, this.node, block);
-
 				block.renderer.add_to_context('$$binding_groups');
+				this.binding_group.elements.push(this.parent.var);
 
-				if (is_context && !block.binding_group_initialised.has(keypath)) {
-					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.binding_group_initialised.add(keypath);
+				if ((this.parent as ElementWrapper).has_dynamic_value) {	
+					update_or_condition = (this.parent as ElementWrapper).dynamic_value_condition;	
 				}
-
-				block.chunks.hydrate.push(
-					b`${binding_group(true)}.push(${parent.var});`
-				);
-
-				block.chunks.destroy.push(
-					b`${binding_group(true)}.splice(${binding_group(true)}.indexOf(${parent.var}), 1);`
-				);
 				break;
 			}
 
@@ -214,7 +203,8 @@ export default class BindingWrapper {
 
 		if (update_dom) {
 			if (update_conditions.length > 0) {
-				const condition = update_conditions.reduce((lhs, rhs) => x`${lhs} && ${rhs}`);
+				let condition = update_conditions.reduce((lhs, rhs) => x`${lhs} && ${rhs}`);
+				if (update_or_condition) condition = x`${update_or_condition} || (${condition})`;
 
 				block.chunks.update.push(b`
 					if (${condition}) {
@@ -279,7 +269,8 @@ function get_dom_updater(
 	return b`${element.var}.${binding.node.name} = ${binding.snippet};`;
 }
 
-function get_binding_group(renderer: Renderer, value: Binding, block: Block) {
+function get_binding_group(renderer: Renderer, binding: BindingWrapper, block: Block) {
+	const value = binding.node;
 	const { parts } = flatten_reference(value.raw_expression);
 	let keypath = parts.join('.');
 
@@ -314,41 +305,75 @@ function get_binding_group(renderer: Renderer, value: Binding, block: Block) {
 		contexts.push(name);
 	}
 
+	// create a global binding_group across blocks
 	if (!renderer.binding_groups.has(keypath)) {
 		const index = renderer.binding_groups.size;
+		// the bind:group depends on the list in the {#each} block as well
+		// as reordering (removing and adding back to the DOM) may affect the value
+		const list_dependencies = new Set<string>();
+		let parent = value.parent;
+		while (parent) {
+			if (parent.type === 'EachBlock') {
+				for (const dep of parent.expression.dynamic_dependencies()) {
+					list_dependencies.add(dep);
+				}
+			}
+			parent = parent.parent;
+		}
+
+		const elements = [];
 
 		contexts.forEach(context => {
 			renderer.add_to_context(context, true);
 		});
 
 		renderer.binding_groups.set(keypath, {
-			binding_group: (to_reference: boolean = false) => {
-				let binding_group = '$$binding_groups';
-				let _secondary_indexes = contexts;
+			binding_group: () => {
+				let obj = x`$$binding_groups[${index}]`;
 
-				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 => {
+				if (contexts.length > 0) {
+					contexts.forEach(secondary_index => {
 						obj = x`${obj}[${secondary_index}]`;
 					});
-					return obj;
-				} else {
-					return x`${binding_group}[${index}]`;
 				}
+				return obj;
 			},
-			is_context: contexts.length > 0,
 			contexts,
-			index,
-			keypath
+			list_dependencies,
+			keypath,
+			elements,
+			render() {
+				const local_name = block.get_unique_name('binding_group');
+				const binding_group = block.renderer.reference('$$binding_groups');
+				block.add_variable(local_name);
+				if (contexts.length > 0) {
+					const indexes = { type: 'ArrayExpression', elements: contexts.map(name => block.renderer.reference(name)) };
+					block.chunks.init.push(
+						b`${local_name} = @init_binding_group_dynamic(${binding_group}[${index}], ${indexes})`
+					);
+					block.chunks.update.push(
+						b`if (${block.renderer.dirty(Array.from(list_dependencies))}) ${local_name}.u(${indexes})`
+					);
+				} else {
+					block.chunks.init.push(
+						b`${local_name} = @init_binding_group(${binding_group}[${index}])`
+					);
+				}
+				block.chunks.hydrate.push(
+					b`${local_name}.p(${elements})`
+				);
+				block.chunks.destroy.push(
+					b`${local_name}.r()`
+				);
+			}
 		});
 	}
 
-	return renderer.binding_groups.get(keypath);
+	// register the binding_group for the block
+	const binding_group = renderer.binding_groups.get(keypath);
+	block.binding_groups.add(binding_group);
+
+	return binding_group;
 }
 
 function get_event_handler(
@@ -386,7 +411,7 @@ function get_event_handler(
 		}
 	}
 
-	const value = get_value_from_dom(renderer, binding.parent, binding, block, contextual_dependencies);
+	const value = get_value_from_dom(renderer, binding.parent, binding, contextual_dependencies);
 
 	const mutation = b`
 		${lhs} = ${value};
@@ -402,10 +427,9 @@ function get_event_handler(
 }
 
 function get_value_from_dom(
-	renderer: Renderer,
+	_renderer: Renderer,
 	element: ElementWrapper | InlineComponentWrapper,
 	binding: BindingWrapper,
-	block: Block,
 	contextual_dependencies: Set<string>
 ) {
 	const { node } = element;
@@ -427,7 +451,7 @@ function get_value_from_dom(
 	// <input type='checkbox' bind:group='foo'>
 	if (name === 'group') {
 		if (type === 'checkbox') {
-			const { binding_group, contexts } = get_binding_group(renderer, binding.node, block);
+			const { binding_group, contexts } = binding.binding_group;
 			add_to_set(contextual_dependencies, contexts);
 			return x`@get_binding_group_value(${binding_group()}, this.__value, this.checked)`;
 		}

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

@@ -163,6 +163,8 @@ export default class ElementWrapper extends Wrapper {
 	has_dynamic_attribute: boolean;
 
 	select_binding_dependencies?: Set<string>;
+	has_dynamic_value: boolean;
+	dynamic_value_condition: any;
 
 	var: any;
 	void: boolean;