Merge pull request #1977 from isiahmeadows/remove-legacy-state

Remove support for `vnode.state = ...`

docs/change-log.md

@@ -18,6 +18,7 @@
 
 - API: `m.redraw()` is always asynchronous ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592))
 - API: `m.mount()` will only render its own root when called, it will not trigger a `redraw()` ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592))
+- API: Assigning to `vnode.state` (as in `vnode.state = ...`) is no longer supported. Instead, an error is thrown if `vnode.state` changes upon the invocation of a lifecycle hook.
 
 #### News
 

docs/vnodes.md

@@ -74,7 +74,6 @@ Property   | Type                             | Description
 `dom`      | `Element?`                       | Points to the element that corresponds to the vnode. This property is `undefined` in the `oninit` lifecycle method. In fragments and trusted HTML vnodes, `dom` points to the first element in the range.
 `domSize`  | `Number?`                        | This is only set in fragment and trusted HTML vnodes, and it's `undefined` in all other vnode types. It defines the number of DOM elements that the vnode represents (starting from the element referenced by the `dom` property).
 `state`    | `Object?`                        | An object that is persisted between redraws. It is provided by the core engine when needed. In POJO component vnodes, the `state` inherits prototypically from the component object/class. In class component vnodes it is an instance of the class. In closure components it is the object returned by the closure.
-`_state`   | `Object?`                        | For components, a reference to the original `vnode.state` object, used to lookup the `view` and hooks. This property is only used internally by Mithril, do not use or modify it.
 `events`   | `Object?`                        | An object that is persisted between redraws and that stores event handlers so that they can be removed using the DOM API. The `events` property is `undefined` if there are no event handlers defined. This property is only used internally by Mithril, do not use or modify it.
 `instance` | `Object?`                        | For components, a storage location for the value returned by the `view`. This property is only used internally by Mithril, do not use or modify it.
 `skip`     | `Boolean`                        | This property is only used internally by Mithril when diffing keyed lists, do not use or modify it.

render/render.js

@@ -18,6 +18,24 @@ module.exports = function($window) {
 		return vnode.attrs && vnode.attrs.xmlns || nameSpace[vnode.tag]
 	}
 
+	//sanity check to discourage people from doing `vnode.state = ...`
+	function checkState(vnode, original) {
+		if (vnode.state !== original) throw new Error("`vnode.state` must not be modified")
+	}
+
+	//Note: the hook is passed as the `this` argument to allow proxying the
+	//arguments without requiring a full array allocation to do so. It also
+	//takes advantage of the fact the current `vnode` is the first argument in
+	//all lifecycle methods.
+	function callHook(vnode) {
+		var original = vnode.state
+		try {
+			return this.apply(original, arguments)
+		} finally {
+			checkState(vnode, original)
+		}
+	}
+
 	//create
 	function createNodes(parent, vnodes, start, end, hooks, nextSibling, ns) {
 		for (var i = start; i < end; i++) {
@@ -121,10 +139,9 @@ module.exports = function($window) {
 			sentinel.$$reentrantLock$$ = true
 			vnode.state = (vnode.tag.prototype != null && typeof vnode.tag.prototype.view === "function") ? new vnode.tag(vnode) : vnode.tag(vnode)
 		}
-		vnode._state = vnode.state
 		if (vnode.attrs != null) initLifecycle(vnode.attrs, vnode, hooks)
-		initLifecycle(vnode._state, vnode, hooks)
+		initLifecycle(vnode.state, vnode, hooks)
-		vnode.instance = Vnode.normalize(vnode._state.view.call(vnode.state, vnode))
+		vnode.instance = Vnode.normalize(callHook.call(vnode.state.view, vnode))
 		if (vnode.instance === vnode) throw Error("A view cannot return the vnode it received as argument")
 		sentinel.$$reentrantLock$$ = null
 	}
@@ -240,7 +257,6 @@ module.exports = function($window) {
 		var oldTag = old.tag, tag = vnode.tag
 		if (oldTag === tag) {
 			vnode.state = old.state
-			vnode._state = old._state
 			vnode.events = old.events
 			if (!recycling && shouldNotUpdate(vnode, old)) return
 			if (typeof oldTag === "string") {
@@ -321,10 +337,10 @@ module.exports = function($window) {
 		if (recycling) {
 			initComponent(vnode, hooks)
 		} else {
-			vnode.instance = Vnode.normalize(vnode._state.view.call(vnode.state, vnode))
+			vnode.instance = Vnode.normalize(callHook.call(vnode.state.view, vnode))
 			if (vnode.instance === vnode) throw Error("A view cannot return the vnode it received as argument")
 			if (vnode.attrs != null) updateLifecycle(vnode.attrs, vnode, hooks)
-			updateLifecycle(vnode._state, vnode, hooks)
+			updateLifecycle(vnode.state, vnode, hooks)
 		}
 		if (vnode.instance != null) {
 			if (old.instance == null) createNode(parent, vnode.instance, hooks, ns, nextSibling)
@@ -410,15 +426,16 @@ module.exports = function($window) {
 	}
 	function removeNode(vnode, context) {
 		var expected = 1, called = 0
+		var original = vnode.state
 		if (vnode.attrs && typeof vnode.attrs.onbeforeremove === "function") {
-			var result = vnode.attrs.onbeforeremove.call(vnode.state, vnode)
+			var result = callHook.call(vnode.attrs.onbeforeremove, vnode)
 			if (result != null && typeof result.then === "function") {
 				expected++
 				result.then(continuation, continuation)
 			}
 		}
-		if (typeof vnode.tag !== "string" && typeof vnode._state.onbeforeremove === "function") {
+		if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeremove === "function") {
-			var result = vnode._state.onbeforeremove.call(vnode.state, vnode)
+			var result = callHook.call(vnode.state.onbeforeremove, vnode)
 			if (result != null && typeof result.then === "function") {
 				expected++
 				result.then(continuation, continuation)
@@ -427,6 +444,7 @@ module.exports = function($window) {
 		continuation()
 		function continuation() {
 			if (++called === expected) {
+				checkState(vnode, original)
 				onremove(vnode)
 				if (vnode.dom) {
 					var count = vnode.domSize || 1
@@ -450,9 +468,9 @@ module.exports = function($window) {
 		if (parent != null) parent.removeChild(node)
 	}
 	function onremove(vnode) {
-		if (vnode.attrs && typeof vnode.attrs.onremove === "function") vnode.attrs.onremove.call(vnode.state, vnode)
+		if (vnode.attrs && typeof vnode.attrs.onremove === "function") callHook.call(vnode.attrs.onremove, vnode)
 		if (typeof vnode.tag !== "string") {
-			if (typeof vnode._state.onremove === "function") vnode._state.onremove.call(vnode.state, vnode)
+			if (typeof vnode.state.onremove === "function") callHook.call(vnode.state.onremove, vnode)
 			if (vnode.instance != null) onremove(vnode.instance)
 		} else {
 			var children = vnode.children
@@ -611,16 +629,20 @@ module.exports = function($window) {
 
 	//lifecycle
 	function initLifecycle(source, vnode, hooks) {
-		if (typeof source.oninit === "function") source.oninit.call(vnode.state, vnode)
+		if (typeof source.oninit === "function") callHook.call(source.oninit, vnode)
-		if (typeof source.oncreate === "function") hooks.push(source.oncreate.bind(vnode.state, vnode))
+		if (typeof source.oncreate === "function") hooks.push(callHook.bind(source.oncreate, vnode))
 	}
 	function updateLifecycle(source, vnode, hooks) {
-		if (typeof source.onupdate === "function") hooks.push(source.onupdate.bind(vnode.state, vnode))
+		if (typeof source.onupdate === "function") hooks.push(callHook.bind(source.onupdate, vnode))
 	}
 	function shouldNotUpdate(vnode, old) {
 		var forceVnodeUpdate, forceComponentUpdate
-		if (vnode.attrs != null && typeof vnode.attrs.onbeforeupdate === "function") forceVnodeUpdate = vnode.attrs.onbeforeupdate.call(vnode.state, vnode, old)
+		if (vnode.attrs != null && typeof vnode.attrs.onbeforeupdate === "function") {
-		if (typeof vnode.tag !== "string" && typeof vnode._state.onbeforeupdate === "function") forceComponentUpdate = vnode._state.onbeforeupdate.call(vnode.state, vnode, old)
+			forceVnodeUpdate = callHook.call(vnode.attrs.onbeforeupdate, vnode, old)
+		}
+		if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeupdate === "function") {
+			forceComponentUpdate = callHook.call(vnode.state.onbeforeupdate, vnode, old)
+		}
 		if (!(forceVnodeUpdate === undefined && forceComponentUpdate === undefined) && !forceVnodeUpdate && !forceComponentUpdate) {
 			vnode.dom = old.dom
 			vnode.domSize = old.domSize

render/tests/test-component.js

@@ -764,97 +764,6 @@ o.spec("component", function() {
 						o(attrs[hook].callCount).equals(methods[hook].callCount)(hook)
 					})
 				})
-				o("lifecycle timing megatest (for a single component with the state overwritten)", function() {
-					var methods = {
-						view: o.spy(function(vnode) {
-							o(vnode.state).equals(1)
-							return ""
-						})
-					}
-					var attrs = {}
-					var hooks = [
-						"oninit", "oncreate", "onbeforeupdate",
-						"onupdate", "onbeforeremove", "onremove"
-					]
-					hooks.forEach(function(hook) {
-						// the `attrs` hooks are called before  the component ones
-						attrs[hook] = o.spy(function(vnode) {
-							o(vnode.state).equals(1)
-							o(attrs[hook].callCount).equals(methods[hook].callCount + 1)
-						})
-						methods[hook] = o.spy(function(vnode) {
-							o(vnode.state).equals(1)
-							o(attrs[hook].callCount).equals(methods[hook].callCount)
-						})
-					})
-
-					var attrsOninit = attrs.oninit
-					var methodsOninit = methods.oninit
-					attrs.oninit = o.spy(function(vnode){
-						vnode.state = 1
-						return attrsOninit.call(this, vnode)
-					})
-					methods.oninit = o.spy(function(vnode){
-						vnode.state = 1
-						return methodsOninit.call(this, vnode)
-					})
-
-					var component = createComponent(methods)
-
-					o(methods.view.callCount).equals(0)
-					o(methods.oninit.callCount).equals(0)
-					o(methods.oncreate.callCount).equals(0)
-					o(methods.onbeforeupdate.callCount).equals(0)
-					o(methods.onupdate.callCount).equals(0)
-					o(methods.onbeforeremove.callCount).equals(0)
-					o(methods.onremove.callCount).equals(0)
-
-					hooks.forEach(function(hook) {
-						o(attrs[hook].callCount).equals(methods[hook].callCount)(hook)
-					})
-
-					render(root, [{tag: component, attrs: attrs}])
-
-					o(methods.view.callCount).equals(1)
-					o(methods.oninit.callCount).equals(1)
-					o(methods.oncreate.callCount).equals(1)
-					o(methods.onbeforeupdate.callCount).equals(0)
-					o(methods.onupdate.callCount).equals(0)
-					o(methods.onbeforeremove.callCount).equals(0)
-					o(methods.onremove.callCount).equals(0)
-
-					hooks.forEach(function(hook) {
-						o(attrs[hook].callCount).equals(methods[hook].callCount)(hook)
-					})
-
-					render(root, [{tag: component, attrs: attrs}])
-
-					o(methods.view.callCount).equals(2)
-					o(methods.oninit.callCount).equals(1)
-					o(methods.oncreate.callCount).equals(1)
-					o(methods.onbeforeupdate.callCount).equals(1)
-					o(methods.onupdate.callCount).equals(1)
-					o(methods.onbeforeremove.callCount).equals(0)
-					o(methods.onremove.callCount).equals(0)
-
-					hooks.forEach(function(hook) {
-						o(attrs[hook].callCount).equals(methods[hook].callCount)(hook)
-					})
-
-					render(root, [])
-
-					o(methods.view.callCount).equals(2)
-					o(methods.oninit.callCount).equals(1)
-					o(methods.oncreate.callCount).equals(1)
-					o(methods.onbeforeupdate.callCount).equals(1)
-					o(methods.onupdate.callCount).equals(1)
-					o(methods.onbeforeremove.callCount).equals(1)
-					o(methods.onremove.callCount).equals(1)
-
-					hooks.forEach(function(hook) {
-						o(attrs[hook].callCount).equals(methods[hook].callCount)(hook)
-					})
-				})
 				o("hook state and arguments validation", function(){
 					var methods = {
 						view: o.spy(function(vnode) {

render/tests/test-onbeforeremove.js

@@ -36,10 +36,7 @@ o.spec("onbeforeremove", function() {
 		o(update.callCount).equals(0)
 	})
 	o("calls onbeforeremove when removing element", function(done) {
-		var vnode = {tag: "div", attrs: {
+		var vnode = {tag: "div", attrs: {onbeforeremove: remove}}
-			oninit: function() {vnode.state = {}},
-			onbeforeremove: remove
-		}}
 
 		render(root, [vnode])
 		render(root, [])
@@ -47,6 +44,7 @@ o.spec("onbeforeremove", function() {
 		function remove(node) {
 			o(node).equals(vnode)
 			o(this).equals(vnode.state)
+			o(this != null && typeof this === "object").equals(true)
 			o(root.childNodes.length).equals(1)
 			o(root.firstChild).equals(vnode.dom)
 

render/vnode.js

@@ -1,7 +1,7 @@
 "use strict"
 
 function Vnode(tag, key, attrs, children, text, dom) {
-	return {tag: tag, key: key, attrs: attrs, children: children, text: text, dom: dom, domSize: undefined, state: undefined, _state: undefined, events: undefined, instance: undefined, skip: false}
+	return {tag: tag, key: key, attrs: attrs, children: children, text: text, dom: dom, domSize: undefined, state: undefined, events: undefined, instance: undefined, skip: false}
 }
 Vnode.normalize = function(node) {
 	if (Array.isArray(node)) return Vnode("[", undefined, undefined, Vnode.normalizeChildren(node), undefined, undefined)