Merge pull request #591 from sveltejs/gh-590

On `<select>`, don't try generating prop code until visiting attributes

src/generators/dom/visitors/Element/Element.js

@@ -46,7 +46,7 @@ export default function visitElement ( generator, block, state, node ) {
 		block.builders.create.addLine( `${generator.helper( 'setAttribute' )}( ${name}, '${generator.cssId}', '' );` );
 	}
 
-	function visitAttributes () {
+	function visitAttributesAndAddProps () {
 		let intro;
 		let outro;
 
@@ -63,25 +63,6 @@ export default function visitElement ( generator, block, state, node ) {
 			});
 
 		if ( intro || outro ) addTransitions( generator, block, childState, node, intro, outro );
-	}
-
-	if ( !state.parentNode ) {
-		// TODO we eventually need to consider what happens to elements
-		// that belong to the same outgroup as an outroing element...
-		block.builders.detach.addLine( `${generator.helper( 'detachNode' )}( ${name} );` );
-	}
-
-	if ( node.name !== 'select' ) {
-		// <select> value attributes are an annoying special case — it must be handled
-		// *after* its children have been updated
-		visitAttributes();
-	}
-
-	// special case – bound <option> without a value attribute
-	if ( node.name === 'option' && !node.attributes.find( attribute => attribute.type === 'Attribute' && attribute.name === 'value' ) ) { 	// TODO check it's bound
-		const statement = `${name}.__value = ${name}.textContent;`;
-		node.initialUpdate = node.lateUpdate = statement;
-	}
 
 		if ( childState.allUsedContexts.length || childState.usesComponent ) {
 			const initialProps = [];
@@ -113,6 +94,25 @@ export default function visitElement ( generator, block, state, node ) {
 				block.builders.update.addBlock( updates.join( '\n' ) );
 			}
 		}
+	}
+
+	if ( !state.parentNode ) {
+		// TODO we eventually need to consider what happens to elements
+		// that belong to the same outgroup as an outroing element...
+		block.builders.detach.addLine( `${generator.helper( 'detachNode' )}( ${name} );` );
+	}
+
+	if ( node.name !== 'select' ) {
+		// <select> value attributes are an annoying special case — it must be handled
+		// *after* its children have been updated
+		visitAttributesAndAddProps();
+	}
+
+	// special case – bound <option> without a value attribute
+	if ( node.name === 'option' && !node.attributes.find( attribute => attribute.type === 'Attribute' && attribute.name === 'value' ) ) { 	// TODO check it's bound
+		const statement = `${name}.__value = ${name}.textContent;`;
+		node.initialUpdate = node.lateUpdate = statement;
+	}
 
 	node.children.forEach( child => {
 		visit( generator, block, childState, child );
@@ -123,7 +123,7 @@ export default function visitElement ( generator, block, state, node ) {
 	}
 
 	if ( node.name === 'select' ) {
-		visitAttributes();
+		visitAttributesAndAddProps();
 	}
 
 	if ( node.initialUpdate ) {

test/runtime/samples/select-props/_config.js

@@ -0,0 +1,15 @@
+export default {
+	test ( assert, component, target, window ) {
+		const selects = document.querySelectorAll( 'select' );
+
+		const event1 = new window.Event( 'change' );
+		selects[0].value = 'b';
+		selects[0].dispatchEvent(event1);
+
+		const event2 = new window.Event( 'change' );
+		selects[1].value = 'b';
+		selects[1].dispatchEvent(event2);
+
+		assert.deepEqual( component.get( 'log' ), [ 1, 2 ] );
+	}
+};

test/runtime/samples/select-props/main.html

@@ -0,0 +1,19 @@
+{{#each foo as bar}}
+	<select on:change='handler(bar)'>
+		<option>a</option>
+		<option>b</option>
+	</select>
+{{/each}}
+
+<script>
+	export default {
+		data: () => ({ foo: [ 1, 2 ], log: [] }),
+		methods: {
+			handler ( bar ) {
+				let { log } = this.get();
+				log.push( bar );
+				this.set( { log } );
+			}
+		}
+	}
+</script>