From dbbdb0664a5f8b99a057c4ffdeaeaed0b9925107 Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Fri, 25 Aug 2017 05:19:30 -0400 Subject: [PATCH 1/3] Reduce memory allocation/usage across multiple event handlers - `handleEvent` is a very useful tool. - Always use `addEventListener`/`removeEventListener`, since it's required for this optimization. - Change log updated. - Drive-by: make DOM mock work with both event listener types. - Drive-by: eliminate possibility of `Object.prototype` interference. --- docs/change-log.md | 3 +++ render/render.js | 40 ++++++++++++++++++++++++---------------- test-utils/domMock.js | 4 +++- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/docs/change-log.md b/docs/change-log.md index 82686f0e..01b4810c 100644 --- a/docs/change-log.md +++ b/docs/change-log.md @@ -28,6 +28,9 @@ - API: `m.route.set()` causes all mount points to be redrawn ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592)) - API: If a user sets the Content-Type header within a request's options, that value will be the entire header value rather than being appended to the default value ([#1924](https://github.com/MithrilJS/mithril.js/pull/1924)) - API: Using style objects in hyperscript calls will now properly diff style properties from one render to another as opposed to re-writing all element style properties every render. +- core: `addEventListener` and `removeEventListener` are always used to manage event subscriptions, preventing external interference. +- core: Event listeners allocate less memory, swap at low cost, and are properly diffed now when rendered via `m.mount()`/`m.redraw()`. +- core: `Object.prototype` properties can no longer interfere with event listener calls. --- diff --git a/render/render.js b/render/render.js index f6782297..bcecf29a 100644 --- a/render/render.js +++ b/render/render.js @@ -575,24 +575,32 @@ module.exports = function($window) { } } + // Here's an explanation of how this works: + // 1. The event names are always (by design) prefixed by `on`. + // 2. The EventListener interface accepts either a function or an object + // with a `handleEvent` method. + // 3. The object does not inherit from `Object.prototype`, to avoid + // any potential interference with that (e.g. setters). + // 4. The event name is remapped to the handler before calling it. + // 5. In function-based event handlers, `ev.target === this`. We replicate + // that below. + function EventDict() {} + EventDict.prototype = Object.create(null) + EventDict.prototype.handleEvent = function (ev) { + this["on" + ev.type].call(ev.target, ev) + if (typeof onevent === "function") onevent.call(ev.target, ev) + } + //event function updateEvent(vnode, key, value) { - var element = vnode.dom - var callback = typeof onevent !== "function" ? value : function(e) { - var result = value.call(element, e) - onevent.call(element, e) - return result - } - if (key in element) element[key] = typeof value === "function" ? callback : null - else { - var eventName = key.slice(2) - if (vnode.events === undefined) vnode.events = {} - if (vnode.events[key] === callback) return - if (vnode.events[key] != null) element.removeEventListener(eventName, vnode.events[key], false) - if (typeof value === "function") { - vnode.events[key] = callback - element.addEventListener(eventName, vnode.events[key], false) - } + if (typeof value === "function") { + if (vnode.events == null) vnode.events = new EventDict() + if (vnode.events[key] === value) return + if (vnode.events[key] == null) vnode.dom.addEventListener(key.slice(2), vnode.events, false) + vnode.events[key] = value + } else if (vnode.events != null) { + if (vnode.events[key] != null) vnode.dom.removeEventListener(key.slice(2), vnode.events, false) + delete vnode.events[key] } } diff --git a/test-utils/domMock.js b/test-utils/domMock.js index 060ba4f7..d99dce47 100644 --- a/test-utils/domMock.js +++ b/test-utils/domMock.js @@ -278,7 +278,9 @@ module.exports = function(options) { e.target = this if (events[e.type] != null) { for (var i = 0; i < events[e.type].length; i++) { - events[e.type][i].call(this, e) + var handler = events[e.type][i] + if (typeof handler === "function") handler.call(this, e) + else handler.handleEvent(e) } } e.preventDefault = function() { From 2c92d8405840ef59e50c3298a645f24f636a7119 Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Fri, 25 Aug 2017 06:06:46 -0400 Subject: [PATCH 2/3] Add support for object event handlers (using `handleEvent`) - `handleEvent` is checked on dispatch, like in the DOM. - Had to reorder attribute key checking so `undefined` events still got removed. - Drive-by: Optimize the initial attribute key checking a little. - Drive-by: Fix changelog v2.0.0 link in TOC. --- docs/change-log.md | 4 +- render/render.js | 16 ++-- render/tests/test-event.js | 191 ++++++++++++++++++++++++++++++++++++- 3 files changed, 200 insertions(+), 11 deletions(-) diff --git a/docs/change-log.md b/docs/change-log.md index 01b4810c..8e263532 100644 --- a/docs/change-log.md +++ b/docs/change-log.md @@ -1,6 +1,6 @@ # Change log -- [v2.0.0](#v113) +- [v2.0.0](#v200-wip) - [v1.1.4](#v113) - [v1.1.3](#v113) - [v1.1.2](#v112) @@ -22,6 +22,7 @@ #### News - API: Introduction of `m.redraw.sync()` ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592)) +- API: Event handlers may also be objects with `handleEvent` methods ([#1939](https://github.com/MithrilJS/mithril.js/issues/1939)). #### Bug fixes @@ -31,6 +32,7 @@ - core: `addEventListener` and `removeEventListener` are always used to manage event subscriptions, preventing external interference. - core: Event listeners allocate less memory, swap at low cost, and are properly diffed now when rendered via `m.mount()`/`m.redraw()`. - core: `Object.prototype` properties can no longer interfere with event listener calls. +- API: Event handlers, when set to literally `undefined` (or any non-function), are now correctly removed. --- diff --git a/render/render.js b/render/render.js index bcecf29a..1ed2f473 100644 --- a/render/render.js +++ b/render/render.js @@ -472,13 +472,11 @@ module.exports = function($window) { } } function setAttr(vnode, key, old, value, ns) { + if (key === "key" || key === "is" || isLifecycleMethod(key)) return + if (key[0] === "o" && key[1] === "n") return updateEvent(vnode, key, value) + if ((old === value && !isFormAttribute(vnode, key)) && typeof value !== "object" || value === undefined) return var element = vnode.dom - if (key === "key" || key === "is" || (old === value && !isFormAttribute(vnode, key)) && typeof value !== "object" || typeof value === "undefined" || isLifecycleMethod(key)) return - var nsLastIndex = key.indexOf(":") - if (nsLastIndex > -1 && key.substr(0, nsLastIndex) === "xlink") { - element.setAttributeNS("http://www.w3.org/1999/xlink", key.slice(nsLastIndex + 1), value) - } - else if (key[0] === "o" && key[1] === "n" && typeof value === "function") updateEvent(vnode, key, value) + if (key.slice(0, 6) === "xlink:") element.setAttributeNS("http://www.w3.org/1999/xlink", key.slice(6), value) else if (key === "style") updateStyle(element, old, value) else if (key in element && !isAttribute(key) && ns === undefined && !isCustomElement(vnode)) { if (key === "value") { @@ -587,13 +585,15 @@ module.exports = function($window) { function EventDict() {} EventDict.prototype = Object.create(null) EventDict.prototype.handleEvent = function (ev) { - this["on" + ev.type].call(ev.target, ev) + var handler = this["on" + ev.type] + if (typeof handler === "function") handler.call(ev.target, ev) + else if (typeof handler.handleEvent === "function") handler.handleEvent(ev) if (typeof onevent === "function") onevent.call(ev.target, ev) } //event function updateEvent(vnode, key, value) { - if (typeof value === "function") { + if (typeof value === "function" || value != null && typeof value === "object") { if (vnode.events == null) vnode.events = new EventDict() if (vnode.events[key] === value) return if (vnode.events[key] == null) vnode.dom.addEventListener(key.slice(2), vnode.events, false) diff --git a/render/tests/test-event.js b/render/tests/test-event.js index 1f7eec2a..b6cb1540 100644 --- a/render/tests/test-event.js +++ b/render/tests/test-event.js @@ -33,7 +33,27 @@ o.spec("event", function() { o(onevent.args[0].type).equals("click") o(onevent.args[0].target).equals(div.dom) }) - + + o("handles click EventListener object", function() { + var spy = o.spy() + var listener = {handleEvent: spy} + var div = {tag: "div", attrs: {onclick: listener}} + var e = $window.document.createEvent("MouseEvents") + e.initEvent("click", true, true) + + render(root, [div]) + div.dom.dispatchEvent(e) + + o(spy.callCount).equals(1) + o(spy.this).equals(listener) + o(spy.args[0].type).equals("click") + o(spy.args[0].target).equals(div.dom) + o(onevent.callCount).equals(1) + o(onevent.this).equals(div.dom) + o(onevent.args[0].type).equals("click") + o(onevent.args[0].target).equals(div.dom) + }) + o("removes event", function() { var spy = o.spy() var vnode = {tag: "a", attrs: {onclick: spy}} @@ -45,7 +65,130 @@ o.spec("event", function() { var e = $window.document.createEvent("MouseEvents") e.initEvent("click", true, true) vnode.dom.dispatchEvent(e) - + + o(spy.callCount).equals(0) + }) + + o("removes event when null", function() { + var spy = o.spy() + var vnode = {tag: "a", attrs: {onclick: spy}} + var updated = {tag: "a", attrs: {onclick: null}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("MouseEvents") + e.initEvent("click", true, true) + vnode.dom.dispatchEvent(e) + + o(spy.callCount).equals(0) + }) + + o("removes event when undefined", function() { + var spy = o.spy() + var vnode = {tag: "a", attrs: {onclick: spy}} + var updated = {tag: "a", attrs: {onclick: undefined}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("MouseEvents") + e.initEvent("click", true, true) + vnode.dom.dispatchEvent(e) + + o(spy.callCount).equals(0) + }) + + o("removes event added via addEventListener when null", function() { + var spy = o.spy() + var vnode = {tag: "a", attrs: {ontouchstart: spy}} + var updated = {tag: "a", attrs: {ontouchstart: null}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("TouchEvents") + e.initEvent("touchstart", true, true) + vnode.dom.dispatchEvent(e) + + o(spy.callCount).equals(0) + }) + + o("removes event added via addEventListener", function() { + var spy = o.spy() + var vnode = {tag: "a", attrs: {ontouchstart: spy}} + var updated = {tag: "a", attrs: {}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("TouchEvents") + e.initEvent("touchstart", true, true) + vnode.dom.dispatchEvent(e) + + o(spy.callCount).equals(0) + }) + + o("removes event added via addEventListener when undefined", function() { + var spy = o.spy() + var vnode = {tag: "a", attrs: {ontouchstart: spy}} + var updated = {tag: "a", attrs: {ontouchstart: undefined}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("TouchEvents") + e.initEvent("touchstart", true, true) + vnode.dom.dispatchEvent(e) + + o(spy.callCount).equals(0) + }) + + o("removes EventListener object", function() { + var spy = o.spy() + var listener = {handleEvent: spy} + var vnode = {tag: "a", attrs: {onclick: listener}} + var updated = {tag: "a", attrs: {}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("MouseEvents") + e.initEvent("click", true, true) + vnode.dom.dispatchEvent(e) + + o(spy.callCount).equals(0) + }) + + o("removes EventListener object when null", function() { + var spy = o.spy() + var listener = {handleEvent: spy} + var vnode = {tag: "a", attrs: {onclick: listener}} + var updated = {tag: "a", attrs: {onclick: null}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("MouseEvents") + e.initEvent("click", true, true) + vnode.dom.dispatchEvent(e) + + o(spy.callCount).equals(0) + }) + + o("removes EventListener object when undefined", function() { + var spy = o.spy() + var listener = {handleEvent: spy} + var vnode = {tag: "a", attrs: {onclick: listener}} + var updated = {tag: "a", attrs: {onclick: undefined}} + + render(root, [vnode]) + render(root, [updated]) + + var e = $window.document.createEvent("MouseEvents") + e.initEvent("click", true, true) + vnode.dom.dispatchEvent(e) + o(spy.callCount).equals(0) }) @@ -72,6 +215,30 @@ o.spec("event", function() { o(div.dom.attributes["id"].value).equals("b") }) + o("fires click EventListener object only once after redraw", function() { + var spy = o.spy() + var listener = {handleEvent: spy} + var div = {tag: "div", attrs: {id: "a", onclick: listener}} + var updated = {tag: "div", attrs: {id: "b", onclick: listener}} + var e = $window.document.createEvent("MouseEvents") + e.initEvent("click", true, true) + + render(root, [div]) + render(root, [updated]) + div.dom.dispatchEvent(e) + + o(spy.callCount).equals(1) + o(spy.this).equals(listener) + o(spy.args[0].type).equals("click") + o(spy.args[0].target).equals(div.dom) + o(onevent.callCount).equals(1) + o(onevent.this).equals(div.dom) + o(onevent.args[0].type).equals("click") + o(onevent.args[0].target).equals(div.dom) + o(div.dom).equals(updated.dom) + o(div.dom.attributes["id"].value).equals("b") + }) + o("handles ontransitionend", function() { var spy = o.spy() var div = {tag: "div", attrs: {ontransitionend: spy}} @@ -90,4 +257,24 @@ o.spec("event", function() { o(onevent.args[0].type).equals("transitionend") o(onevent.args[0].target).equals(div.dom) }) + + o("handles transitionend EventListener object", function() { + var spy = o.spy() + var listener = {handleEvent: spy} + var div = {tag: "div", attrs: {ontransitionend: listener}} + var e = $window.document.createEvent("HTMLEvents") + e.initEvent("transitionend", true, true) + + render(root, [div]) + div.dom.dispatchEvent(e) + + o(spy.callCount).equals(1) + o(spy.this).equals(listener) + o(spy.args[0].type).equals("transitionend") + o(spy.args[0].target).equals(div.dom) + o(onevent.callCount).equals(1) + o(onevent.this).equals(div.dom) + o(onevent.args[0].type).equals("transitionend") + o(onevent.args[0].target).equals(div.dom) + }) }) From a1a7038e55b0f058f5f86d06479d6a7c1f46c8e1 Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Fri, 1 Sep 2017 17:06:19 -0400 Subject: [PATCH 3/3] Rework event diffing for better optimizability Re-ordered the type checks so that I can avoid polymorphic property lookups in event updates. (It improved the common case of no change by a little over ~40%.) --- render/render.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/render/render.js b/render/render.js index 8cc2efb4..d8de6e0d 100644 --- a/render/render.js +++ b/render/render.js @@ -593,14 +593,19 @@ module.exports = function($window) { //event function updateEvent(vnode, key, value) { - if (typeof value === "function" || value != null && typeof value === "object") { - if (vnode.events == null) vnode.events = new EventDict() + if (vnode.events != null) { if (vnode.events[key] === value) return - if (vnode.events[key] == null) vnode.dom.addEventListener(key.slice(2), vnode.events, false) + if (value != null && (typeof value === "function" || typeof value === "object")) { + if (vnode.events[key] == null) vnode.dom.addEventListener(key.slice(2), vnode.events, false) + vnode.events[key] = value + } else { + if (vnode.events[key] != null) vnode.dom.removeEventListener(key.slice(2), vnode.events, false) + vnode.events[key] = undefined + } + } else if (value != null && (typeof value === "function" || typeof value === "object")) { + vnode.events = new EventDict() + vnode.dom.addEventListener(key.slice(2), vnode.events, false) vnode.events[key] = value - } else if (vnode.events != null) { - if (vnode.events[key] != null) vnode.dom.removeEventListener(key.slice(2), vnode.events, false) - delete vnode.events[key] } }