Add more type checks, unify auto-redraw logic, optimize element creation better

Author: dead-claudia

src/core.js

@@ -1,5 +1,5 @@
 /* eslint-disable no-bitwise */
-import {hasOwn} from "./util.js"
+import {checkCallback, hasOwn, invokeRedrawable} from "./util.js"
 
 export {m as default}
 
@@ -286,43 +286,23 @@ m.TYPE_SET_CONTEXT = TYPE_SET_CONTEXT
 m.TYPE_USE = TYPE_USE
 m.TYPE_INLINE = TYPE_INLINE
 
-// Simple and sweet. Also useful for idioms like `onfoo: m.capture` to drop events without
-// redrawing.
+// Simple and sweet. Also useful for idioms like `onfoo: m.capture` to completely drop events while
+// otherwise ignoring them.
 m.capture = (ev) => {
 	ev.preventDefault()
 	ev.stopPropagation()
-	return false
+	return "skip-redraw"
 }
 
 m.retain = () => Vnode(TYPE_RETAIN, null, null, null)
-
-m.inline = (callback) => {
-	if (typeof callback !== "function") {
-		throw new TypeError("Callback must be a function.")
-	}
-	return Vnode(TYPE_INLINE, null, callback, null)
-}
-
-m.layout = (callback) => {
-	if (typeof callback !== "function") {
-		throw new TypeError("Callback must be a function.")
-	}
-	return Vnode(TYPE_LAYOUT, null, callback, null)
-}
-
-m.remove = (callback) => {
-	if (typeof callback !== "function") {
-		throw new TypeError("Callback must be a function.")
-	}
-	return Vnode(TYPE_REMOVE, null, callback, null)
-}
+m.inline = (view) => Vnode(TYPE_INLINE, null, checkCallback(view, false, "view"), null)
+m.layout = (callback) => Vnode(TYPE_LAYOUT, null, checkCallback(callback), null)
+m.remove = (callback) => Vnode(TYPE_REMOVE, null, checkCallback(callback), null)
 
 m.Fragment = (attrs) => attrs.children
 
 m.keyed = (values, view) => {
-	if (view != null && typeof view !== "function") {
-		throw new TypeError("Callback must be a function if provided")
-	}
+	view = checkCallback(view, true, "view")
 	var map = new Map()
 	for (var value of values) {
 		if (typeof view === "function") value = view(value)
@@ -586,19 +566,19 @@ var updateText = (old, vnode) => {
 
 var handleAttributeError = (old, e, force) => {
 	if (currentRemoveOnThrow || force) {
-		removeNode(old)
-		updateFragment(old, null)
+		if (old) removeElement(old)
 		throw e
 	}
 	console.error(e)
 }
 
 var updateElement = (old, vnode) => {
 	var prevParent = currentParent
+	var prevRefNode = currentRefNode
 	var prevNamespace = currentNamespace
 	var mask = vnode.m
 	var attrs = vnode.a
-	var element , oldAttrs
+	var element, oldAttrs
 
 	if (old == null) {
 		var entry = selectorCache.get(vnode.t)
@@ -608,11 +588,11 @@ var updateElement = (old, vnode) => {
 		var ns = attrs && attrs.xmlns || nameSpace[tag] || prevNamespace
 		var opts = is ? {is} : null
 
-		insertAfterCurrentRefNode(element = vnode.d = (
+		element = (
 			ns
 				? currentDocument.createElementNS(ns, tag, opts)
 				: currentDocument.createElement(tag, opts)
-		))
+		)
 
 		if (ns == null) {
 			// Doing it this way since it doesn't seem Terser is smart enough to optimize the `if` with
@@ -709,8 +689,17 @@ var updateElement = (old, vnode) => {
 	}
 
 	currentParent = prevParent
-	currentRefNode = element
+	currentRefNode = prevRefNode
 	currentNamespace = prevNamespace
+
+	// Do this as late as possible to reduce how much work browsers have to do to reduce style
+	// recalcs during initial (sub)tree construction. Also will defer `adoptNode` callbacks in
+	// custom elements until the last possible point (which will help accelerate some of them).
+	if (old == null) {
+		insertAfterCurrentRefNode(vnode.d = element)
+	}
+
+	currentRefNode = element
 }
 
 var updateComponent = (old, vnode) => {
@@ -757,6 +746,11 @@ var removeNode = (old) => {
 	}
 }
 
+var removeElement = (old) => {
+	removeNode(old)
+	updateFragment(old, null)
+}
+
 var removeInstance = (old) => updateNode(old.c, null)
 
 // Replaces an otherwise necessary `switch`.
@@ -777,10 +771,7 @@ var removeNodeDispatch = [
 	removeFragment,
 	removeKeyed,
 	removeNode,
-	(old) => {
-		removeNode(old)
-		updateFragment(old, null)
-	},
+	removeElement,
 	removeInstance,
 	() => {},
 	(old) => currentHooks.push(old),
@@ -1096,13 +1087,7 @@ var setAttr = (vnode, element, mask, key, old, attrs) => {
 //    return a promise that resolves to it.
 class EventDict extends Map {
 	async handleEvent(ev) {
-		var handler = this.get(`on${ev.type}`)
-		if (typeof handler === "function") {
-			var result = handler.call(ev.currentTarget, ev)
-			if (result === false) return
-			if (result && typeof result.then === "function" && (await result) === false) return
-			(0, this._)()
-		}
+		invokeRedrawable(this._, this.get(`on${ev.type}`), ev.currentTarget, ev)
 	}
 }
 
@@ -1111,13 +1096,16 @@ class EventDict extends Map {
 var currentlyRendering = []
 
 m.render = (dom, vnode, {redraw, removeOnThrow} = {}) => {
-	if (!dom) throw new TypeError("DOM element being rendered to does not exist.")
-	if (currentlyRendering.some((d) => d === dom || d.contains(dom))) {
-		throw new TypeError("Node is currently being rendered to and thus is locked.")
+	if (!dom) {
+		throw new TypeError("DOM element being rendered to does not exist.")
 	}
 
-	if (redraw != null && typeof redraw !== "function") {
-		throw new TypeError("Redraw must be a function if given.")
+	checkCallback(redraw, true, "redraw")
+
+	for (var root of currentlyRendering) {
+		if (dom.contains(root)) {
+			throw new TypeError("Node is currently being rendered to and thus is locked.")
+		}
 	}
 
 	var active = dom.ownerDocument.activeElement

src/std/init.js

@@ -1,14 +1,14 @@
 import m from "../core.js"
 
+import {checkCallback, invokeRedrawable} from "../util.js"
+
 function Init({f}, old) {
 	if (old) return m.retain()
 	var ctrl = new AbortController()
-	queueMicrotask(async () => {
-		if ((await f(ctrl.signal)) !== false) this.redraw()
-	})
+	queueMicrotask(() => invokeRedrawable(this.redraw, f, undefined, ctrl.signal))
 	return m.remove(() => ctrl.abort())
 }
 
-var init = (f) => m(Init, {f})
+var init = (f) => m(Init, {f: checkCallback(f)})
 
 export {init as default}

src/std/lazy.js

@@ -1,11 +1,17 @@
 import m from "../core.js"
 
+import {checkCallback} from "../util.js"
+
 var lazy = (opts) => {
+	checkCallback(opts.fetch, false, "opts.fetch")
+	checkCallback(opts.pending, true, "opts.pending")
+	checkCallback(opts.error, true, "opts.error")
+
 	// Capture the error here so stack traces make more sense
 	var error = new ReferenceError("Component not found")
 	var redraws = new Set()
 	var Comp = function () {
-		redraws.add(this.redraw)
+		redraws.add(checkCallback(this.redraw, false, "context.redraw"))
 		return opts.pending && opts.pending()
 	}
 	var init = async () => {

src/std/router.js

@@ -1,6 +1,8 @@
 /* global window: false */
 import m from "../core.js"
 
+import {checkCallback} from "../util.js"
+
 var Route = function ({p: prefix}) {
 	var href = this.href
 	var mustReplace, redraw, currentParsedHref
@@ -60,7 +62,7 @@ var Route = function ({p: prefix}) {
 	updateRouteWithHref()
 
 	return function ({v: view}) {
-		redraw = this.redraw
+		redraw = checkCallback(this.redraw, false, "context.redraw")
 
 		return [
 			m.remove(() => window.removeEventListener("popstate", updateRoute)),
@@ -74,11 +76,7 @@ export var route = (prefix, view) => {
 		throw new TypeError("The route prefix must be a string")
 	}
 
-	if (typeof view !== "function") {
-		throw new TypeError("Router view must be a function.")
-	}
-
-	return m(Route, {v: view, p: prefix})
+	return m(Route, {v: checkCallback(view, false, "view"), p: prefix})
 }
 
 // Let's provide a *right* way to manage a route link, rather than letting people screw up

src/std/tracked.js

@@ -45,6 +45,8 @@ why that was removed in favor of this:
    work around it, you'd have to do something like this anyways.
 */
 
+import {checkCallback} from "../util.js"
+
 /**
  * @template K, V
  * @typedef TrackedHandle
@@ -75,6 +77,8 @@ why that was removed in favor of this:
  * @returns {Tracked<K, V>}
  */
 var tracked = (redraw, initial) => {
+	checkCallback(redraw, false, "redraw")
+
 	/** @type {Map<K, TrackedHandle<K, V> & {_: AbortController}>} */ var state = new Map()
 	/** @type {Set<TrackedHandle<K, V>>} */ var live = new Set()
 

src/std/with-progress.js

@@ -1,8 +1,12 @@
+import {checkCallback} from "../util.js"
+
 /**
  * @param {ReadableStream<Uint8Array> | null} source
  * @param {(current: number) => void} notify
  */
 export default (source, notify) => {
+	checkCallback(notify, false, "notify")
+
 	var reader = source && source.getReader()
 	var current = 0
 

src/util.js

@@ -1 +1,18 @@
 export var hasOwn = {}.hasOwnProperty
+
+export var invokeRedrawable = async (redraw, fn, thisValue, ...args) => {
+	if (typeof fn === "function") {
+		thisValue = Reflect.apply(fn, thisValue, args)
+		if (thisValue === "skip-redraw") return
+		if (thisValue && typeof thisValue.then === "function" && (await thisValue) === "skip-redraw") return
+		redraw()
+	}
+}
+
+export var checkCallback = (callback, allowNull, label = "callback") => {
+	if (allowNull && callback == null || typeof callback === "function") {
+		return callback
+	}
+
+	throw new TypeError(`\`${label}\` must be a function${allowNull ? " if provided." : "."}`)
+}

tests/core/event.js

@@ -85,8 +85,8 @@ o.spec("event", function() {
 		})
 	})
 
-	o("handles onclick returning false", function() {
-		var spyDiv = eventSpy((e) => { m.capture(e); return false })
+	o("handles onclick returning `\"skip-redraw\"`", function() {
+		var spyDiv = eventSpy((e) => { m.capture(e); return "skip-redraw" })
 		var spyParent = eventSpy()
 		var div = m("div", {onclick: spyDiv})
 		var parent = m("div", {onclick: spyParent}, div)
@@ -106,9 +106,9 @@ o.spec("event", function() {
 		o(e.defaultPrevented).equals(true)
 	})
 
-	o("handles onclick asynchronously returning false", function() {
+	o("handles onclick asynchronously returning `\"skip-redraw\"`", function() {
 		var promise
-		var spyDiv = eventSpy((e) => { m.capture(e); return promise = Promise.resolve(false) })
+		var spyDiv = eventSpy((e) => { m.capture(e); return promise = Promise.resolve("skip-redraw") })
 		var spyParent = eventSpy()
 		var div = m("div", {onclick: spyDiv})
 		var parent = m("div", {onclick: spyParent}, div)
@@ -132,8 +132,8 @@ o.spec("event", function() {
 		})
 	})
 
-	o("handles onclick returning false in child then bubbling to parent and not returning false", function() {
-		var spyDiv = eventSpy(() => false)
+	o("handles onclick returning `\"skip-redraw\"` in child then bubbling to parent and not returning `\"skip-redraw\"`", function() {
+		var spyDiv = eventSpy(() => "skip-redraw")
 		var spyParent = eventSpy()
 		var div = m("div", {onclick: spyDiv})
 		var parent = m("div", {onclick: spyParent}, div)

tests/core/hyperscript.js

@@ -732,7 +732,7 @@ o.spec("hyperscript", function() {
 			// Only doing this for the sake of initializing the required fields in the mock.
 			G.root.dispatchEvent(e)
 
-			o(m.capture(e)).equals(false)
+			o(m.capture(e)).equals("skip-redraw")
 			o(e.defaultPrevented).equals(true)
 			o(e.cancelBubble).equals(true)
 		})

tests/core/mountRedraw.js

@@ -445,7 +445,7 @@ o.spec("mount/redraw", function() {
 		e.initEvent("click", true, true)
 
 		m.mount(G.root, () => m("div", {
-			onclick: () => false,
+			onclick: () => "skip-redraw",
 		}, m.layout(layout)))
 
 		G.root.firstChild.dispatchEvent(e)