inline $$invalidate calls - fixes #3512

src/compiler/compile/Component.ts

@@ -813,7 +813,7 @@ export default class Component {
 		const variable = this.var_lookup.get(name);
 
 		if (variable && (variable.subscribable && variable.reassigned)) {
-			return `$$subscribe_${name}(), $$invalidate('${name}', ${value || name})`;
+			return `$$subscribe_${name}($$invalidate('${name}', ${value || name}))`;
 		}
 
 		if (name[0] === '$' && name[1] !== '$') {

src/compiler/compile/nodes/shared/Expression.ts

@@ -10,10 +10,11 @@ import Wrapper from '../../render_dom/wrappers/shared/Wrapper';
 
 import TemplateScope from './TemplateScope';
 import get_object from '../../utils/get_object';
-import { nodes_match } from '../../../utils/nodes_match';
+// import { nodes_match } from '../../../utils/nodes_match';
 import Block from '../../render_dom/Block';
 import { INode } from '../interfaces';
 import is_dynamic from '../../render_dom/wrappers/shared/is_dynamic';
+import { nodes_match } from '../../../utils/nodes_match';
 
 const binary_operators: Record<string, number> = {
 	'**': 15,
@@ -241,7 +242,6 @@ export default class Expression {
 		const { code } = component;
 
 		let function_expression;
-		let pending_assignments: Set<string> = new Set();
 
 		let dependencies: Set<string>;
 		let contextual_dependencies: Set<string>;
@@ -309,16 +309,6 @@ export default class Expression {
 				if (map.has(node)) scope = scope.parent;
 
 				if (node === function_expression) {
-					if (pending_assignments.size > 0) {
-						if (node.type !== 'ArrowFunctionExpression') {
-							// this should never happen!
-							throw new Error(`Well that's odd`);
-						}
-
-						// TOOD optimisation — if this is an event handler,
-						// the return value doesn't matter
-					}
-
 					const name = component.get_unique_name(
 						sanitize(get_function_name(node, owner))
 					);
@@ -334,35 +324,10 @@ export default class Expression {
 						args.push(original_params);
 					}
 
+					// TODO is this still necessary?
 					let body = code.slice(node.body.start, node.body.end).trim();
 					if (node.body.type !== 'BlockStatement') {
-						if (pending_assignments.size > 0) {
+						body = `{\n\treturn ${body};\n}`;
-							const dependencies = new Set();
-							pending_assignments.forEach(name => {
-								if (template_scope.names.has(name)) {
-									template_scope.dependencies_for_name.get(name).forEach(dependency => {
-										dependencies.add(dependency);
-									});
-								} else {
-									dependencies.add(name);
-								}
-							});
-
-							const insert = Array.from(dependencies).map(name => component.invalidate(name)).join('; ');
-							pending_assignments = new Set();
-
-							component.has_reactive_assignments = true;
-
-							body = deindent`
-								{
-									const $$result = ${body};
-									${insert};
-									return $$result;
-								}
-							`;
-						} else {
-							body = `{\n\treturn ${body};\n}`;
-						}
 					}
 
 					const fn = deindent`
@@ -421,65 +386,78 @@ export default class Expression {
 					contextual_dependencies = null;
 				}
 
-				if (node.type === 'AssignmentExpression') {
+				// TODO dry out — most of this is shared with render_dom/index.ts
-					const names = node.left.type === 'MemberExpression'
+				if (node.type === 'AssignmentExpression' || node.type === 'UpdateExpression') {
-						? [get_object(node.left).name]
+					const assignee = node.type === 'AssignmentExpression' ? node.left : node.argument;
-						: extract_names(node.left);
-
-					if (node.operator === '=' && nodes_match(node.left, node.right)) {
-						const dirty = names.filter(name => {
-							return !scope.declarations.has(name);
-						});
 
-						if (dirty.length) component.has_reactive_assignments = true;
+					// normally (`a = 1`, `b.c = 2`), there'll be a single name
+					// (a or b). In destructuring cases (`[d, e] = [e, d]`) there
+					// may be more, in which case we need to tack the extra ones
+					// onto the initial function call
+					const names = new Set(assignee.type === 'MemberExpression'
+						? [get_object(assignee).name]
+						: extract_names(assignee));
 
-						code.overwrite(node.start, node.end, dirty.map(n => component.invalidate(n)).join('; '));
+					const traced: Set<string> = new Set();
-					} else {
+					names.forEach(name => {
-						names.forEach(name => {
+						const dependencies = template_scope.dependencies_for_name.get(name);
-							if (scope.declarations.has(name)) return;
+						if (dependencies) {
+							dependencies.forEach(name => traced.add(name));
+						} else {
+							traced.add(name);
+						}
+					});
 
-							const variable = component.var_lookup.get(name);
+					const [head, ...tail] = Array.from(traced).filter(name => {
-							if (variable && variable.hoistable) return;
+						const owner = scope.find_owner(name);
+						if (owner && owner !== component.instance_scope) return;
+
+						const variable = component.var_lookup.get(name);
+
+						return variable && (
+							!variable.hoistable &&
+							!variable.global &&
+							!variable.module &&
+							(
+								variable.referenced ||
+								variable.is_reactive_dependency ||
+								variable.export_name
+							)
+						);
+					});
 
-							pending_assignments.add(name);
+					if (head) {
-						});
+						component.has_reactive_assignments = true;
-					}
-				} else if (node.type === 'UpdateExpression') {
-					const { name } = get_object(node.argument);
 
-					if (scope.declarations.has(name)) return;
+						if (node.operator === '=' && nodes_match(node.left, node.right) && tail.length === 0) {
+							code.overwrite(node.start, node.end, component.invalidate(head));
+						} else {
+							let suffix = ')';
 
-					const variable = component.var_lookup.get(name);
+							if (head[0] === '$') {
-					if (variable && variable.hoistable) return;
+								code.prependRight(node.start, `${component.helper('set_store_value')}(${head.slice(1)}, `);
+							} else {
+								let prefix = `$$invalidate`;
 
-					pending_assignments.add(name);
+								const variable = component.var_lookup.get(head);
-				}
+								if (variable.subscribable && variable.reassigned) {
+									prefix = `$$subscribe_${head}($$invalidate`;
+									suffix += `)`;
+								}
 
-				if (/Statement/.test(node.type)) {
+								code.prependRight(node.start, `${prefix}('${head}', `);
-					if (pending_assignments.size > 0) {
+							}
-						const has_semi = code.original[node.end - 1] === ';';
 
-						const insert = (
+							const extra_args = tail.map(name => component.invalidate(name));
-							(has_semi ? ' ' : '; ') +
-							Array.from(pending_assignments).map(name => component.invalidate(name)).join('; ')
-						);
 
-						if (/^(Break|Continue|Return)Statement/.test(node.type)) {
+							if (assignee.type !== 'Identifier' || node.type === 'UpdateExpression' && !node.prefix || extra_args.length > 0) {
-							if (node.argument) {
+								extra_args.unshift(head);
-								code.overwrite(node.start, node.argument.start, `var $$result = `);
-								code.appendLeft(node.end, `${insert}; return $$result`);
-							} else {
-								code.prependRight(node.start, `${insert}; `);
 							}
-						} else if (parent && /(If|For(In|Of)?|While)Statement/.test(parent.type) && node.type !== 'BlockStatement') {
-							code.prependRight(node.start, '{ ');
-							code.appendLeft(node.end, `${insert}; }`);
-						} else {
-							code.appendLeft(node.end, `${insert};`);
-						}
 
-						component.has_reactive_assignments = true;
+							suffix = `${extra_args.map(arg => `, ${arg}`).join('')}${suffix}`;
-						pending_assignments = new Set();
+
+							code.appendLeft(node.end, suffix);
+						}
 					}
 				}
 			}

src/compiler/compile/render_dom/index.ts

@@ -8,8 +8,9 @@ import { walk } from 'estree-walker';
 import { stringify_props } from '../utils/stringify_props';
 import add_to_set from '../utils/add_to_set';
 import get_object from '../utils/get_object';
-import { extract_names } from '../utils/scope';
+// import { extract_names } from '../utils/scope';
 import { nodes_match } from '../../utils/nodes_match';
+import { extract_names } from '../utils/scope';
 
 export default function dom(
 	component: Component,
@@ -158,8 +159,6 @@ export default function dom(
 		let scope = component.instance_scope;
 		const map = component.instance_scope_map;
 
-		let pending_assignments = new Set();
-
 		walk(component.ast.instance.content, {
 			enter: (node) => {
 				if (map.has(node)) {
@@ -167,102 +166,78 @@ export default function dom(
 				}
 			},
 
-			leave(node, parent) {
+			leave(node) {
 				if (map.has(node)) {
 					scope = scope.parent;
 				}
 
+				// TODO dry out — most of this is shared with Expression.ts
 				if (node.type === 'AssignmentExpression' || node.type === 'UpdateExpression') {
 					const assignee = node.type === 'AssignmentExpression' ? node.left : node.argument;
-					let names = [];
-
-					if (assignee.type === 'MemberExpression') {
-						const left_object_name = get_object(assignee).name;
-						left_object_name && (names = [left_object_name]);
-					} else {
-						names = extract_names(assignee);
-					}
-
-					if (node.operator === '=' && nodes_match(node.left, node.right)) {
-						const dirty = names.filter(name => {
-							return name[0] === '$' || scope.find_owner(name) === component.instance_scope;
-						});
 
-						if (dirty.length) component.has_reactive_assignments = true;
+					// normally (`a = 1`, `b.c = 2`), there'll be a single name
-
+					// (a or b). In destructuring cases (`[d, e] = [e, d]`) there
-						code.overwrite(node.start, node.end, dirty.map(n => component.invalidate(n)).join('; '));
+					// may be more, in which case we need to tack the extra ones
-					} else {
+					// onto the initial function call
-						const single = (
+					const names = new Set(assignee.type === 'MemberExpression'
-							node.type === 'AssignmentExpression' &&
+						? [get_object(assignee).name]
-							assignee.type === 'Identifier' &&
+						: extract_names(assignee));
-							parent.type === 'ExpressionStatement' &&
+
-							assignee.name[0] !== '$'
+					const [head, ...tail] = Array.from(names).filter(name => {
+						const owner = scope.find_owner(name);
+						if (owner && owner !== component.instance_scope) return;
+
+						const variable = component.var_lookup.get(name);
+
+						return variable && (
+							!variable.hoistable &&
+							!variable.global &&
+							!variable.module &&
+							(
+								variable.referenced ||
+								variable.is_reactive_dependency ||
+								variable.export_name
+							)
 						);
+					});
 
-						names.forEach(name => {
+					if (head) {
-							const owner = scope.find_owner(name);
+						component.has_reactive_assignments = true;
-							if (owner && owner !== component.instance_scope) return;
 
-							const variable = component.var_lookup.get(name);
+						if (node.operator === '=' && nodes_match(node.left, node.right) && tail.length === 0) {
-							if (variable && (variable.hoistable || variable.global || variable.module)) return;
+							code.overwrite(node.start, node.end, component.invalidate(head));
+						} else {
+							let suffix = ')';
 
-							if (single && !(variable.subscribable && variable.reassigned)) {
+							if (head[0] === '$') {
-								if (variable.referenced || variable.is_reactive_dependency || variable.export_name) {
+								code.prependRight(node.start, `${component.helper('set_store_value')}(${head.slice(1)}, `);
-									code.prependRight(node.start, `$$invalidate('${name}', `);
-									code.appendLeft(node.end, `)`);
-								}
 							} else {
-								pending_assignments.add(name);
+								let prefix = `$$invalidate`;
-							}
 
-							component.has_reactive_assignments = true;
+								const variable = component.var_lookup.get(head);
-						});
+								if (variable.subscribable && variable.reassigned) {
-					}
+									prefix = `$$subscribe_${head}($$invalidate`;
-				}
+									suffix += `)`;
-
+								}
-				if (pending_assignments.size > 0) {
-					if (node.type === 'ArrowFunctionExpression') {
-						const insert = Array.from(pending_assignments).map(name => component.invalidate(name)).join('; ');
-						pending_assignments = new Set();
-
-						code.prependRight(node.body.start, `{ const $$result = `);
-						code.appendLeft(node.body.end, `; ${insert}; return $$result; }`);
 
-						pending_assignments = new Set();
+								code.prependRight(node.start, `${prefix}('${head}', `);
-					}
+							}
 
-					else if (/Statement/.test(node.type)) {
+							const extra_args = tail.map(name => component.invalidate(name));
-						const insert = Array.from(pending_assignments).map(name => component.invalidate(name)).join('; ');
 
-						if (/^(Break|Continue|Return)Statement/.test(node.type)) {
+							if (assignee.type !== 'Identifier' || node.type === 'UpdateExpression' && !node.prefix || extra_args.length > 0) {
-							if (node.argument) {
+								extra_args.unshift(head);
-								code.overwrite(node.start, node.argument.start, `var $$result = `);
-								code.appendLeft(node.argument.end, `; ${insert}; return $$result`);
-							} else {
-								code.prependRight(node.start, `${insert}; `);
 							}
-						} else if (parent && /(If|For(In|Of)?|While)Statement/.test(parent.type) && node.type !== 'BlockStatement') {
-							code.prependRight(node.start, '{ ');
-							code.appendLeft(node.end, `${code.original[node.end - 1] === ';' ? '' : ';'} ${insert}; }`);
-						} else {
-							code.appendLeft(node.end, `${code.original[node.end - 1] === ';' ? '' : ';'} ${insert};`);
-						}
 
-						pending_assignments = new Set();
+							suffix = `${extra_args.map(arg => `, ${arg}`).join('')}${suffix}`;
-					} else if (parent && parent.type !== 'ForStatement' && node.type === 'VariableDeclaration') {
-						const insert = Array.from(pending_assignments).map(name => component.invalidate(name)).join('; ');
 
-						code.appendLeft(node.end, `${code.original[node.end - 1] === ';' ? '' : ';'} ${insert};`);
+							code.appendLeft(node.end, suffix);
-						pending_assignments = new Set();
+						}
 					}
 				}
 			}
 		});
 
-		if (pending_assignments.size > 0) {
-			throw new Error(`TODO this should not happen!`);
-		}
-
 		component.rewrite_props(({ name, reassigned }) => {
 			const value = `$${name}`;
 
@@ -395,7 +370,7 @@ export default function dom(
 
 			const store = component.var_lookup.get(name);
 			if (store && store.reassigned) {
-				return `${$name}, $$unsubscribe_${name} = @noop, $$subscribe_${name} = () => { $$unsubscribe_${name}(); $$unsubscribe_${name} = @subscribe(${name}, $$value => { ${$name} = $$value; $$invalidate('${$name}', ${$name}); }) }`;
+				return `${$name}, $$unsubscribe_${name} = @noop, $$subscribe_${name} = () => ($$unsubscribe_${name}(), $$unsubscribe_${name} = @subscribe(${name}, $$value => { ${$name} = $$value; $$invalidate('${$name}', ${$name}); }), ${name})`;
 			}
 
 			return $name;

src/runtime/internal/Component.ts

@@ -101,11 +101,12 @@ export function init(component, options, instance, create_fragment, not_equal, p
 	let ready = false;
 
 	$$.ctx = instance
-		? instance(component, props, (key, value) => {
+		? instance(component, props, (key, ret, value = ret) => {
 			if ($$.ctx && not_equal($$.ctx[key], $$.ctx[key] = value)) {
 				if ($$.bound[key]) $$.bound[key](value);
 				if (ready) make_dirty(component, key);
 			}
+			return ret;
 		})
 		: props;
 

src/runtime/internal/utils.ts

@@ -100,3 +100,8 @@ export function once(fn) {
 export function null_to_empty(value) {
 	return value == null ? '' : value;
 }
+
+export function set_store_value(store, ret, value = ret) {
+	store.set(value);
+	return ret;
+}

test/js/samples/instrumentation-template-if-no-block/expected.js

@@ -61,7 +61,7 @@ function instance($$self, $$props, $$invalidate) {
 	let x = 0;
 
 	function click_handler() {
-		if (true) { x += 1; $$invalidate('x', x); }
+		if (true) $$invalidate('x', x += 1);
 	}
 
 	return { x, click_handler };

test/runtime/samples/invalidation-in-if-condition/_config.js

@@ -0,0 +1,18 @@
+export default {
+	show: 1,
+	html: `<button>false 0</button>`,
+
+	async test({ assert, target, window }) {
+		const button = target.querySelector('button');
+		const click = new window.MouseEvent('click');
+
+		await button.dispatchEvent(click);
+		assert.htmlEqual(target.innerHTML, `<button>true 1</button>`);
+
+		await button.dispatchEvent(click);
+		assert.htmlEqual(target.innerHTML, `<button>false 1</button>`);
+
+		await button.dispatchEvent(click);
+		assert.htmlEqual(target.innerHTML, `<button>true 2</button>`);
+	}
+};

test/runtime/samples/invalidation-in-if-condition/main.svelte

@@ -0,0 +1,12 @@
+<script>
+	let current = { active: false };
+	let count = 0;
+
+	function toggle() {
+		if (current.active = !current.active) {
+			count += 1;
+		}
+	}
+</script>
+
+<button on:click={toggle}>{current.active} {count}</button>

test/runtime/samples/store-resubscribe/_config.js

@@ -5,7 +5,7 @@ export default {
 		<button>reset</button>
 	`,
 
-	async test({ assert, component, target, window }) {
+	async test({ assert, target, window }) {
 		const buttons = target.querySelectorAll('button');
 		const click = new window.MouseEvent('click');