fix <select> one-way bind when options change (#5054)

Author: tanhauhau

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Unreleased
 
 * Fix handling of `this` in inline function expressions in the template ([#5033](https://github.com/sveltejs/svelte/issues/5033))
+* Update `<select>` with one-way `value` binding when the available `<option>`s change ([#5051](https://github.com/sveltejs/svelte/issues/5051))
 
 ## 3.23.2
 

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

@@ -6,6 +6,7 @@ import { string_literal } from '../../../utils/stringify';
 import { b, x } from 'code-red';
 import Expression from '../../../nodes/shared/Expression';
 import Text from '../../../nodes/Text';
+import handle_select_value_binding from './handle_select_value_binding';
 
 export default class AttributeWrapper {
 	node: Attribute;
@@ -36,6 +37,10 @@ export default class AttributeWrapper {
 					});
 				}
 			}
+
+			if (node.name === 'value') {
+				handle_select_value_binding(this, node.dependencies);
+			}
 		}
 	}
 
@@ -74,7 +79,7 @@ export default class AttributeWrapper {
 
 		const is_legacy_input_type = element.renderer.component.compile_options.legacy && name === 'type' && this.parent.node.name === 'input';
 
-		const dependencies = this.node.get_dependencies();
+		const dependencies = this.get_dependencies();
 		const value = this.get_value(block);
 
 		const is_src = this.node.name === 'src'; // TODO retire this exception in favour of https://github.com/sveltejs/svelte/issues/3750
@@ -83,7 +88,7 @@ export default class AttributeWrapper {
 
 		const is_input_value = name === 'value' && element.node.name === 'input';
 
-		const should_cache = is_src || this.node.should_cache() || is_select_value_attribute; // TODO is this necessary?
+		const should_cache = is_src || this.node.should_cache();
 
 		const last = should_cache && block.get_unique_name(
 			`${element.var.name}_${name.replace(/[^a-zA-Z_$]/g, '_')}_value`
@@ -104,13 +109,12 @@ export default class AttributeWrapper {
 			const is_multiple_select = element.node.get_static_attribute_value('multiple');
 
 			if (is_multiple_select) {
-				updater = b`@select_options(${element.var}, ${last});`;
+				updater = b`@select_options(${element.var}, ${value});`;
 			} else {
-				updater = b`@select_option(${element.var}, ${last});`;
+				updater = b`@select_option(${element.var}, ${value});`;
 			}
 
 			block.chunks.mount.push(b`
-				${last} = ${value};
 				${updater}
 			`);
 		} else if (is_src) {
@@ -168,10 +172,26 @@ export default class AttributeWrapper {
 			const update_value = b`${element.var}.value = ${element.var}.__value;`;
 
 			block.chunks.hydrate.push(update_value);
-			if (this.node.get_dependencies().length > 0) block.chunks.update.push(update_value);
+			if (dependencies.length > 0) block.chunks.update.push(update_value);
 		}
 	}
 
+	get_dependencies() {
+		const node_dependencies = this.node.get_dependencies();
+		const dependencies = new Set(node_dependencies);
+
+		node_dependencies.forEach((prop: string) => {
+			const indirect_dependencies = this.parent.renderer.component.indirect_dependencies.get(prop);
+			if (indirect_dependencies) {
+				indirect_dependencies.forEach(indirect_dependency => {
+					dependencies.add(indirect_dependency);
+				});
+			}
+		});
+
+		return Array.from(dependencies);
+	}
+
 	get_metadata() {
 		if (this.parent.node.namespace) return null;
 		const metadata = attribute_lookup[fix_attribute_casing(this.node.name)];

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

@@ -10,6 +10,7 @@ import flatten_reference from '../../../utils/flatten_reference';
 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';
+import handle_select_value_binding from './handle_select_value_binding';
 
 export default class BindingWrapper {
 	node: Binding;
@@ -35,12 +36,8 @@ export default class BindingWrapper {
 		block.add_dependencies(dependencies);
 
 		// TODO does this also apply to e.g. `<input type='checkbox' bind:group='foo'>`?
-		if (parent.node.name === 'select') {
-			(parent as ElementWrapper).select_binding_dependencies = dependencies;
-			dependencies.forEach((prop: string) => {
-				parent.renderer.component.indirect_dependencies.set(prop, new Set());
-			});
-		}
+
+		handle_select_value_binding(this, dependencies);
 
 		if (node.is_contextual) {
 			mark_each_block_bindings(this.parent, this.node);

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

@@ -0,0 +1,16 @@
+import AttributeWrapper from "./Attribute";
+import BindingWrapper from "./Binding";
+import ElementWrapper from "./index";
+
+export default function handle_select_value_binding(
+	attr: AttributeWrapper | BindingWrapper,
+	dependencies: Set<string>
+) {
+	const { parent } = attr;
+	if (parent.node.name === "select") {
+		(parent as ElementWrapper).select_binding_dependencies = dependencies;
+		dependencies.forEach((prop: string) => {
+			parent.renderer.component.indirect_dependencies.set(prop, new Set());
+		});
+	}
+}

test/js/samples/select-dynamic-value/expected.js

@@ -15,7 +15,6 @@ function create_fragment(ctx) {
 	let select;
 	let option0;
 	let option1;
-	let select_value_value;
 
 	return {
 		c() {
@@ -33,12 +32,11 @@ function create_fragment(ctx) {
 			insert(target, select, anchor);
 			append(select, option0);
 			append(select, option1);
-			select_value_value = /*current*/ ctx[0];
-			select_option(select, select_value_value);
+			select_option(select, /*current*/ ctx[0]);
 		},
 		p(ctx, [dirty]) {
-			if (dirty & /*current*/ 1 && select_value_value !== (select_value_value = /*current*/ ctx[0])) {
-				select_option(select, select_value_value);
+			if (dirty & /*current*/ 1) {
+				select_option(select, /*current*/ ctx[0]);
 			}
 		},
 		i: noop,

test/runtime/samples/binding-select-late-3/_config.js

@@ -0,0 +1,34 @@
+export default {
+	props: {
+		items: [],
+		selected: 'two'
+	},
+
+	html: `
+		<select></select>
+		<p>selected: two</p>
+	`,
+
+	ssrHtml: `
+		<select value="two"></select>
+		<p>selected: two</p>
+	`,
+
+	test({ assert, component, target }) {
+		component.items = [ 'one', 'two', 'three' ];
+
+		const options = target.querySelectorAll('option');
+		assert.ok(!options[0].selected);
+		assert.ok(options[1].selected);
+		assert.ok(!options[2].selected);
+
+		assert.htmlEqual(target.innerHTML, `
+			<select>
+				<option value='one'>one</option>
+				<option value='two'>two</option>
+				<option value='three'>three</option>
+			</select>
+			<p>selected: two</p>
+		`);
+	}
+};

test/runtime/samples/binding-select-late-3/main.svelte

@@ -0,0 +1,12 @@
+<script>
+	export let selected;
+	export let items;
+</script>
+
+<select value={selected} on:blur={e => selected = e.target.value}>
+	{#each items as item}
+		<option>{item}</option>
+	{/each}
+</select>
+
+<p>selected: {selected || 'nothing'}</p>