From 24f8c2ab78b5da6956589a946f2cc441b664e892 Mon Sep 17 00:00:00 2001
From: Tan Li Hau <tanhauhau@users.noreply.github.com>
Date: Wed, 24 Jun 2020 03:55:33 +0800
Subject: [PATCH] fix <select> one-way bind when options change (#5054)

---
 CHANGELOG.md                                  |  1 +
 .../render_dom/wrappers/Element/Attribute.ts  | 32 +++++++++++++----
 .../render_dom/wrappers/Element/Binding.ts    |  9 ++---
 .../Element/handle_select_value_binding.ts    | 16 +++++++++
 .../samples/select-dynamic-value/expected.js  |  8 ++---
 .../samples/binding-select-late-3/_config.js  | 34 +++++++++++++++++++
 .../samples/binding-select-late-3/main.svelte | 12 +++++++
 7 files changed, 95 insertions(+), 17 deletions(-)
 create mode 100644 src/compiler/compile/render_dom/wrappers/Element/handle_select_value_binding.ts
 create mode 100644 test/runtime/samples/binding-select-late-3/_config.js
 create mode 100644 test/runtime/samples/binding-select-late-3/main.svelte

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6757221f48..85654104e2 100644
--- a/CHANGELOG.md
+++ b/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
 
diff --git a/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts b/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts
index ca941b7be9..86b67ca47e 100644
--- a/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts
+++ b/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)];
diff --git a/src/compiler/compile/render_dom/wrappers/Element/Binding.ts b/src/compiler/compile/render_dom/wrappers/Element/Binding.ts
index 33214ffad3..8e27a7f511 100644
--- a/src/compiler/compile/render_dom/wrappers/Element/Binding.ts
+++ b/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);
diff --git a/src/compiler/compile/render_dom/wrappers/Element/handle_select_value_binding.ts b/src/compiler/compile/render_dom/wrappers/Element/handle_select_value_binding.ts
new file mode 100644
index 0000000000..93a83298f8
--- /dev/null
+++ b/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());
+		});
+	}
+}
diff --git a/test/js/samples/select-dynamic-value/expected.js b/test/js/samples/select-dynamic-value/expected.js
index f1a913c65f..aa4e5004fd 100644
--- a/test/js/samples/select-dynamic-value/expected.js
+++ b/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,
diff --git a/test/runtime/samples/binding-select-late-3/_config.js b/test/runtime/samples/binding-select-late-3/_config.js
new file mode 100644
index 0000000000..42c45d1366
--- /dev/null
+++ b/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>
+		`);
+	}
+};
diff --git a/test/runtime/samples/binding-select-late-3/main.svelte b/test/runtime/samples/binding-select-late-3/main.svelte
new file mode 100644
index 0000000000..ec9ac8d345
--- /dev/null
+++ b/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>
\ No newline at end of file