From 1f0345644fe16a961545028fc249bbb64d62499d Mon Sep 17 00:00:00 2001
From: Rich Harris <richard.a.harris@gmail.com>
Date: Sat, 17 Nov 2018 20:50:12 -0500
Subject: [PATCH] instrument event handlers

---
 .gitignore                                    |  3 +-
 src/Stats.ts                                  |  9 +--
 src/compile/Component.ts                      | 56 +++++++----------
 src/compile/nodes/Action.ts                   |  2 -
 src/compile/nodes/Animation.ts                |  2 -
 src/compile/nodes/EventHandler.ts             | 39 +++++++++---
 src/compile/nodes/Transition.ts               |  2 -
 src/compile/nodes/shared/Expression.ts        |  6 +-
 src/compile/render-dom/index.ts               | 10 +++-
 .../render-dom/wrappers/Element/index.ts      | 57 +++++++-----------
 src/compile/render-dom/wrappers/Window.ts     | 60 ++++++++++---------
 11 files changed, 123 insertions(+), 123 deletions(-)

diff --git a/.gitignore b/.gitignore
index 0c3b104cc2..bd20c2c1ae 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,4 +17,5 @@ node_modules
 /src/compile/shared.ts
 /store.umd.js
 /yarn-error.log
-_actual*.*
\ No newline at end of file
+_actual*.*
+_
\ No newline at end of file
diff --git a/src/Stats.ts b/src/Stats.ts
index b2e16a805c..210475e608 100644
--- a/src/Stats.ts
+++ b/src/Stats.ts
@@ -96,11 +96,12 @@ export default class Stats {
 			}
 		});
 
+		// TODO
 		const hooks: Record<string, boolean> = component && {
-			oncreate: !!component.templateProperties.oncreate,
-			ondestroy: !!component.templateProperties.ondestroy,
-			onstate: !!component.templateProperties.onstate,
-			onupdate: !!component.templateProperties.onupdate
+			oncreate: false,
+			ondestroy: false,
+			onstate: false,
+			onupdate: false
 		};
 
 		return {
diff --git a/src/compile/Component.ts b/src/compile/Component.ts
index 4a3cbeb4a6..8dcdd77a50 100644
--- a/src/compile/Component.ts
+++ b/src/compile/Component.ts
@@ -10,7 +10,7 @@ import namespaces from '../utils/namespaces';
 import { removeNode } from '../utils/removeNode';
 import nodeToString from '../utils/nodeToString';
 import wrapModule from './wrapModule';
-import { createScopes, extractNames } from '../utils/annotateWithScopes';
+import { createScopes, extractNames, Scope } from '../utils/annotateWithScopes';
 import getName from '../utils/getName';
 import Stylesheet from './css/Stylesheet';
 import { test } from '../config';
@@ -24,6 +24,7 @@ import checkForDupes from './validate/js/utils/checkForDupes';
 import propValidators from './validate/js/propValidators';
 import fuzzymatch from './validate/utils/fuzzymatch';
 import flattenReference from '../utils/flattenReference';
+import { instrument } from '../utils/instrument';
 
 interface Computation {
 	key: string;
@@ -102,6 +103,7 @@ export default class Component {
 	name: string;
 	options: CompileOptions;
 	fragment: Fragment;
+	scope: Scope;
 
 	meta: {
 		namespace?: string;
@@ -124,24 +126,13 @@ export default class Component {
 	animations: Set<string>;
 	transitions: Set<string>;
 	actions: Set<string>;
-	importedComponents: Map<string, string>;
 	namespace: string;
 	hasComponents: boolean;
-	computations: Computation[];
-	templateProperties: Record<string, Node>;
 	javascript: string;
 
-	used: {
-		components: Set<string>;
-		helpers: Set<string>;
-		events: Set<string>;
-		animations: Set<string>;
-		transitions: Set<string>;
-		actions: Set<string>;
-	};
-
 	declarations: string[];
 	exports: Array<{ name: string, as: string }>;
+	event_handlers: Array<{ name: string, body: string }>;
 	props: string[];
 
 	refCallees: Node[];
@@ -189,19 +180,10 @@ export default class Component {
 		this.animations = new Set();
 		this.transitions = new Set();
 		this.actions = new Set();
-		this.importedComponents = new Map();
-
-		this.used = {
-			components: new Set(),
-			helpers: new Set(),
-			events: new Set(),
-			animations: new Set(),
-			transitions: new Set(),
-			actions: new Set(),
-		};
 
 		this.declarations = [];
 		this.exports = [];
+		this.event_handlers = [];
 
 		this.refs = new Set();
 		this.refCallees = [];
@@ -230,8 +212,6 @@ export default class Component {
 		this.aliases = new Map();
 		this.usedNames = new Set();
 
-		this.computations = [];
-		this.templateProperties = {};
 		this.properties = new Map();
 
 		this.walkJs();
@@ -486,6 +466,7 @@ export default class Component {
 		});
 
 		let { scope, map, globals } = createScopes(js.content);
+		this.scope = scope;
 
 		scope.declarations.forEach(name => {
 			this.userVars.add(name);
@@ -546,7 +527,7 @@ export default class Component {
 		const top_scope = scope;
 
 		walk(js.content, {
-			enter(node) {
+			enter: (node, parent) => {
 				if (map.has(node)) {
 					scope = map.get(node);
 				}
@@ -555,15 +536,9 @@ export default class Component {
 					const { name } = flattenReference(node.left);
 
 					if (scope.findOwner(name) === top_scope) {
-						// TODO verify that it needs to be reactive, i.e. is
-						// used in the template (and not just in an event
-						// handler or transition etc)
-
-						// TODO handle arrow function expressions etc
-						code.appendLeft(node.end, `; $$make_dirty('${name}')`);
+						this.instrument(node, parent, name);
 					}
 				}
-
 			},
 
 			leave(node) {
@@ -581,6 +556,21 @@ export default class Component {
 
 		this.javascript = a !== b ? `[✂${a}-${b}✂]` : '';
 	}
+
+	instrument(node, parent, name) {
+		// TODO only make values reactive if they're used
+		// in the template
+
+		if (parent.type === 'ArrowFunctionExpression' && node === parent.body) {
+			// TODO don't do the $$result dance if this is an event handler
+			this.code.prependRight(node.start, `{ const $$result = `);
+			this.code.appendLeft(node.end, `; $$make_dirty('${name}'); return $$result; }`);
+		}
+
+		else {
+			this.code.appendLeft(node.end, `; $$make_dirty('${name}')`);
+		}
+	}
 }
 
 type Meta = {
diff --git a/src/compile/nodes/Action.ts b/src/compile/nodes/Action.ts
index fcf96b46c2..79b8f7bdcb 100644
--- a/src/compile/nodes/Action.ts
+++ b/src/compile/nodes/Action.ts
@@ -11,8 +11,6 @@ export default class Action extends Node {
 
 		this.name = info.name;
 
-		component.used.actions.add(this.name);
-
 		this.expression = info.expression
 			? new Expression(component, this, scope, info.expression)
 			: null;
diff --git a/src/compile/nodes/Animation.ts b/src/compile/nodes/Animation.ts
index 8a52a05ecd..31e39da69c 100644
--- a/src/compile/nodes/Animation.ts
+++ b/src/compile/nodes/Animation.ts
@@ -11,8 +11,6 @@ export default class Animation extends Node {
 
 		this.name = info.name;
 
-		component.used.animations.add(this.name);
-
 		if (parent.animation) {
 			component.error(this, {
 				code: `duplicate-animation`,
diff --git a/src/compile/nodes/EventHandler.ts b/src/compile/nodes/EventHandler.ts
index 69a76a48e9..6df7fa98b2 100644
--- a/src/compile/nodes/EventHandler.ts
+++ b/src/compile/nodes/EventHandler.ts
@@ -4,6 +4,8 @@ import addToSet from '../../utils/addToSet';
 import flattenReference from '../../utils/flattenReference';
 import validCalleeObjects from '../../utils/validCalleeObjects';
 import list from '../../utils/list';
+import { createScopes } from '../../utils/annotateWithScopes';
+import { walk } from 'estree-walker';
 
 const validBuiltins = new Set(['set', 'fire', 'destroy']);
 
@@ -18,30 +20,51 @@ export default class EventHandler extends Node {
 	usesContext: boolean;
 	usesEventObject: boolean;
 	isCustomEvent: boolean;
-	shouldHoist: boolean;
 
 	insertionPoint: number;
 	args: Expression[];
 	snippet: string;
 
-	constructor(component, parent, scope, info) {
-		super(component, parent, scope, info);
+	constructor(component, parent, template_scope, info) {
+		super(component, parent, template_scope, info);
 
 		this.name = info.name;
 		this.modifiers = new Set(info.modifiers);
 
-		component.used.events.add(this.name);
-
 		this.dependencies = new Set();
 
 		if (info.expression) {
-			this.expression = new Expression(component, parent, scope, info.expression);
+			this.expression = new Expression(component, parent, template_scope, info.expression, true);
 			this.snippet = this.expression.snippet;
+
+			let { scope, map } = createScopes(info.expression);
+
+			walk(this.expression, {
+				enter: (node, parent) => {
+					if (map.has(node)) {
+						scope = map.get(node);
+					}
+
+					if (node.type === 'AssignmentExpression') {
+						const { name } = flattenReference(node.left);
+
+						if (!scope.has(name)) {
+							component.instrument(node, parent, name);
+						}
+					}
+				},
+
+				leave(node) {
+					if (map.has(node)) {
+						scope = scope.parent;
+					}
+				}
+			});
 		} else {
 			this.snippet = null; // TODO handle shorthand events here?
 		}
 
-		this.isCustomEvent = component.events.has(this.name);
-		this.shouldHoist = !this.isCustomEvent && parent.hasAncestor('EachBlock');
+		// TODO figure out what to do about custom events
+		// this.isCustomEvent = component.events.has(this.name);
 	}
 }
\ No newline at end of file
diff --git a/src/compile/nodes/Transition.ts b/src/compile/nodes/Transition.ts
index 5e6aa922d2..e296d2a24b 100644
--- a/src/compile/nodes/Transition.ts
+++ b/src/compile/nodes/Transition.ts
@@ -26,8 +26,6 @@ export default class Transition extends Node {
 			});
 		}
 
-		this.component.used.transitions.add(this.name);
-
 		this.expression = info.expression
 			? new Expression(component, this, scope, info.expression)
 			: null;
diff --git a/src/compile/nodes/shared/Expression.ts b/src/compile/nodes/shared/Expression.ts
index 0281a6c62c..1d52a10593 100644
--- a/src/compile/nodes/shared/Expression.ts
+++ b/src/compile/nodes/shared/Expression.ts
@@ -65,7 +65,7 @@ export default class Expression {
 
 	thisReferences: Array<{ start: number, end: number }>;
 
-	constructor(component, parent, scope, info) {
+	constructor(component, parent, scope, info, isEventHandler?: boolean) {
 		// TODO revert to direct property access in prod?
 		Object.defineProperties(this, {
 			component: {
@@ -113,8 +113,6 @@ export default class Expression {
 						let object = node;
 						while (object.type === 'MemberExpression') object = object.object;
 
-						component.used.helpers.add(name);
-
 						const alias = component.templateVars.get(`helpers-${name}`);
 						if (alias !== name) code.overwrite(object.start, object.end, alias);
 						return;
@@ -122,7 +120,7 @@ export default class Expression {
 
 					expression.usesContext = true;
 
-					if (!isSynthetic) {
+					if (!isSynthetic && !isEventHandler) {
 						// <option> value attribute could be synthetic — avoid double editing
 						code.prependRight(node.start, key === 'key' && parent.shorthand
 							? `${name}: ctx.`
diff --git a/src/compile/render-dom/index.ts b/src/compile/render-dom/index.ts
index d009e0e97f..0d599f6832 100644
--- a/src/compile/render-dom/index.ts
+++ b/src/compile/render-dom/index.ts
@@ -109,14 +109,20 @@ export default function dom(
 	} else {
 		const refs = Array.from(component.refs);
 
+		const declarations = component.declarations.concat(
+			component.event_handlers.map(handler => handler.name)
+		);
+
 		builder.addBlock(deindent`
 			class ${name} extends @SvelteComponent {
 				$$init($$make_dirty) {
 					${component.javascript || component.exports.map(x => `let ${x.name};`)}
 
+					${component.event_handlers.map(handler => handler.body)}
+
 					return [
 						// TODO only what's needed by the template
-						() => ({ ${(component.declarations).join(', ')} }),
+						() => ({ ${declarations.join(', ')} }),
 						props => {
 							// TODO only do this for export let|var
 							${(component.exports.map(name =>
@@ -148,8 +154,6 @@ export default function dom(
 		`);
 	}
 
-	const immutable = templateProperties.immutable ? templateProperties.immutable.value.value : options.immutable;
-
 	let result = builder.toString();
 
 	return component.generate(result, options, {
diff --git a/src/compile/render-dom/wrappers/Element/index.ts b/src/compile/render-dom/wrappers/Element/index.ts
index 9f9e9893a9..1396420117 100644
--- a/src/compile/render-dom/wrappers/Element/index.ts
+++ b/src/compile/render-dom/wrappers/Element/index.ts
@@ -606,60 +606,43 @@ export default class ElementWrapper extends Wrapper {
 		const { component } = renderer;
 
 		this.node.handlers.forEach(handler => {
-			const isCustomEvent = component.events.has(handler.name);
-
-			const target = handler.shouldHoist ? 'this' : this.var;
+			const { isCustomEvent } = handler;
 
 			// get a name for the event handler that is globally unique
 			// if hoisted, locally unique otherwise
-			const handlerName = (handler.shouldHoist ? component : block).getUniqueName(
+			const handler_name = component.getUniqueName(
 				`${handler.name.replace(/[^a-zA-Z0-9_$]/g, '_')}_handler`
 			);
 
 			const component_name = block.alias('component'); // can't use #component, might be hoisted
 
-			// create the handler body
-			const handlerBody = deindent`
-				${handler.shouldHoist && (
-					handler.usesComponent || handler.usesContext
-						? `const { ${[handler.usesComponent && 'component', handler.usesContext && 'ctx'].filter(Boolean).join(', ')} } = ${target}._svelte;`
-						: null
-				)}
-
-				${handler.snippet ?
-					handler.snippet :
-					`${component_name}.fire("${handler.name}", event);`}
-			`;
-
 			if (isCustomEvent) {
-				block.addVariable(handlerName);
+				throw new Error(`TODO figure out custom events`);
+				block.addVariable(handler_name);
 
 				block.builders.hydrate.addBlock(deindent`
-					${handlerName} = %events-${handler.name}.call(${component_name}, ${this.var}, function(event) {
-						${handlerBody}
+					${handler_name} = ctx.${handler.name}.call(${component_name}, ${this.var}, function(event) {
+						${handler.snippet}
 					});
 				`);
 
 				block.builders.destroy.addLine(deindent`
-					${handlerName}.destroy();
+					${handler_name}.destroy();
 				`);
 			} else {
 				const modifiers = [];
 				if (handler.modifiers.has('preventDefault')) modifiers.push('event.preventDefault();');
 				if (handler.modifiers.has('stopPropagation')) modifiers.push('event.stopPropagation();');
 
-				const handlerFunction = deindent`
-					function ${handlerName}(event) {
-						${modifiers}
-						(${handlerBody})(event);
-					}
-				`;
-
-				if (handler.shouldHoist) {
-					renderer.blocks.push(handlerFunction);
-				} else {
-					block.builders.init.addBlock(handlerFunction);
-				}
+				component.event_handlers.push({
+					name: handler_name,
+					body: deindent`
+						function ${handler_name}(event) {
+							${modifiers}
+							(${handler.snippet})(event);
+						}
+					`
+				});
 
 				const opts = ['passive', 'once', 'capture'].filter(mod => handler.modifiers.has(mod));
 				if (opts.length) {
@@ -668,19 +651,19 @@ export default class ElementWrapper extends Wrapper {
 						: `{ ${opts.map(opt => `${opt}: true`).join(', ')} }`;
 
 					block.builders.hydrate.addLine(
-						`@addListener(${this.var}, "${handler.name}", ${handlerName}, ${optString});`
+						`@addListener(${this.var}, "${handler.name}", ctx.${handler_name}, ${optString});`
 					);
 
 					block.builders.destroy.addLine(
-						`@removeListener(${this.var}, "${handler.name}", ${handlerName}, ${optString});`
+						`@removeListener(${this.var}, "${handler.name}", ctx.${handler_name}, ${optString});`
 					);
 				} else {
 					block.builders.hydrate.addLine(
-						`@addListener(${this.var}, "${handler.name}", ${handlerName});`
+						`@addListener(${this.var}, "${handler.name}", ctx.${handler_name});`
 					);
 
 					block.builders.destroy.addLine(
-						`@removeListener(${this.var}, "${handler.name}", ${handlerName});`
+						`@removeListener(${this.var}, "${handler.name}", ctx.${handler_name});`
 					);
 				}
 			}
diff --git a/src/compile/render-dom/wrappers/Window.ts b/src/compile/render-dom/wrappers/Window.ts
index 11914bce9e..4adeaa4840 100644
--- a/src/compile/render-dom/wrappers/Window.ts
+++ b/src/compile/render-dom/wrappers/Window.ts
@@ -47,36 +47,42 @@ export default class WindowWrapper extends Wrapper {
 
 			let usesState = handler.dependencies.size > 0;
 
-			const handlerName = block.getUniqueName(`onwindow${handler.name}`);
-			const handlerBody = deindent`
+			const handler_name = block.getUniqueName(`onwindow${handler.name}`);
+			const handler_body = deindent`
 				${usesState && `var ctx = #component.get();`}
 				${handler.snippet}
 			`;
 
 			if (isCustomEvent) {
 				// TODO dry this out
-				block.addVariable(handlerName);
+				block.addVariable(handler_name);
 
 				block.builders.hydrate.addBlock(deindent`
-					${handlerName} = %events-${handler.name}.call(#component, window, function(event) {
-						(${handlerBody})(event);
+					${handler_name} = %events-${handler.name}.call(#component, window, function(event) {
+						(${handler_body})(event);
 					});
 				`);
 
 				block.builders.destroy.addLine(deindent`
-					${handlerName}.destroy();
+					${handler_name}.destroy();
 				`);
 			} else {
-				block.builders.init.addBlock(deindent`
-					function ${handlerName}(event) {
-						(${handlerBody})(event);
-					}
-					window.addEventListener("${handler.name}", ${handlerName});
-				`);
+				component.event_handlers.push({
+					name: handler_name,
+					body: deindent`
+						function ${handler_name}(event) {
+							(${handler.snippet})(event);
+						}
+					`
+				});
 
-				block.builders.destroy.addBlock(deindent`
-					window.removeEventListener("${handler.name}", ${handlerName});
-				`);
+				block.builders.init.addLine(
+					`window.addEventListener("${handler.name}", ctx.${handler_name});`
+				);
+
+				block.builders.destroy.addLine(
+					`window.removeEventListener("${handler.name}", ctx.${handler_name});`
+				);
 			}
 		});
 
@@ -106,7 +112,7 @@ export default class WindowWrapper extends Wrapper {
 		const timeout = block.getUniqueName(`window_updating_timeout`);
 
 		Object.keys(events).forEach(event => {
-			const handlerName = block.getUniqueName(`onwindow${event}`);
+			const handler_name = block.getUniqueName(`onwindow${event}`);
 			const props = events[event];
 
 			if (event === 'scroll') {
@@ -138,7 +144,7 @@ export default class WindowWrapper extends Wrapper {
 				});
 			}
 
-			const handlerBody = deindent`
+			const handler_body = deindent`
 				${event === 'scroll' && deindent`
 					if (${lock}) return;
 					${lock} = true;
@@ -154,14 +160,14 @@ export default class WindowWrapper extends Wrapper {
 			`;
 
 			block.builders.init.addBlock(deindent`
-				function ${handlerName}(event) {
-					${handlerBody}
+				function ${handler_name}(event) {
+					${handler_body}
 				}
-				window.addEventListener("${event}", ${handlerName});
+				window.addEventListener("${event}", ${handler_name});
 			`);
 
 			block.builders.destroy.addBlock(deindent`
-				window.removeEventListener("${event}", ${handlerName});
+				window.removeEventListener("${event}", ${handler_name});
 			`);
 		});
 
@@ -189,15 +195,15 @@ export default class WindowWrapper extends Wrapper {
 
 		// another special case. (I'm starting to think these are all special cases.)
 		if (bindings.online) {
-			const handlerName = block.getUniqueName(`onlinestatuschanged`);
+			const handler_name = block.getUniqueName(`onlinestatuschanged`);
 			block.builders.init.addBlock(deindent`
-				function ${handlerName}(event) {
+				function ${handler_name}(event) {
 					${component.options.dev && `component._updatingReadonlyProperty = true;`}
 					#component.set({ ${bindings.online}: navigator.onLine });
 					${component.options.dev && `component._updatingReadonlyProperty = false;`}
 				}
-				window.addEventListener("online", ${handlerName});
-				window.addEventListener("offline", ${handlerName});
+				window.addEventListener("online", ${handler_name});
+				window.addEventListener("offline", ${handler_name});
 			`);
 
 			// add initial value
@@ -206,8 +212,8 @@ export default class WindowWrapper extends Wrapper {
 			);
 
 			block.builders.destroy.addBlock(deindent`
-				window.removeEventListener("online", ${handlerName});
-				window.removeEventListener("offline", ${handlerName});
+				window.removeEventListener("online", ${handler_name});
+				window.removeEventListener("offline", ${handler_name});
 			`);
 		}
 	}