Skip to content

Commit b98ab29

Browse files
dead-claudiadead-claudia
authored
Make errors and their messages more accurate and helpful (#2536)
Also, I normalized them to all be sentences for consistency, and I moved the reentrancy check from `m.mount` to `m.render` to be a little more helpful. The router change during mounting is inconsequential and only to avoid the new modified error, and the change to the update loop is to send the original error if an error occurred while initializing the default route. (This is all around more useful anyways.) And while I was at it, I fixed an obscure bug with sync redraws.
1 parent 4757478 commit b98ab29

File tree

14 files changed

+310
-54
lines changed

14 files changed

+310
-54
lines changed

Diff for: ‎api/mount-redraw.js

+7-8
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,15 @@ var Vnode = require("../render/vnode")
44

55
module.exports = function(render, schedule, console) {
66
var subscriptions = []
7-
var rendering = false
87
var pending = false
8+
var offset = -1
99

1010
function sync() {
11-
if (rendering) throw new Error("Nested m.redraw.sync() call")
12-
rendering = true
13-
for (var i = 0; i < subscriptions.length; i += 2) {
14-
try { render(subscriptions[i], Vnode(subscriptions[i + 1]), redraw) }
11+
for (offset = 0; offset < subscriptions.length; offset += 2) {
12+
try { render(subscriptions[offset], Vnode(subscriptions[offset + 1]), redraw) }
1513
catch (e) { console.error(e) }
1614
}
17-
rendering = false
15+
offset = -1
1816
}
1917

2018
function redraw() {
@@ -31,13 +29,14 @@ module.exports = function(render, schedule, console) {
3129

3230
function mount(root, component) {
3331
if (component != null && component.view == null && typeof component !== "function") {
34-
throw new TypeError("m.mount(element, component) expects a component, not a vnode")
32+
throw new TypeError("m.mount expects a component, not a vnode.")
3533
}
3634

3735
var index = subscriptions.indexOf(root)
3836
if (index >= 0) {
3937
subscriptions.splice(index, 2)
40-
render(root, [], redraw)
38+
if (index <= offset) offset -= 2
39+
render(root, [])
4140
}
4241

4342
if (component != null) {

Diff for: ‎api/router.js

+14-10
Original file line numberDiff line numberDiff line change
@@ -33,16 +33,16 @@ module.exports = function($window, mountRedraw) {
3333
var SKIP = route.SKIP = {}
3434

3535
function route(root, defaultRoute, routes) {
36-
if (root == null) throw new Error("Ensure the DOM element that was passed to `m.route` is not undefined")
36+
if (!root) throw new TypeError("DOM element being rendered to does not exist.")
3737
// 0 = start
3838
// 1 = init
3939
// 2 = ready
4040
var state = 0
4141

4242
var compiled = Object.keys(routes).map(function(route) {
43-
if (route[0] !== "/") throw new SyntaxError("Routes must start with a `/`")
43+
if (route[0] !== "/") throw new SyntaxError("Routes must start with a '/'.")
4444
if ((/:([^\/\.-]+)(\.{3})?:/).test(route)) {
45-
throw new SyntaxError("Route parameter names must be separated with either `/`, `.`, or `-`")
45+
throw new SyntaxError("Route parameter names must be separated with either '/', '.', or '-'.")
4646
}
4747
return {
4848
route: route,
@@ -61,7 +61,7 @@ module.exports = function($window, mountRedraw) {
6161
var defaultData = parsePathname(defaultRoute)
6262

6363
if (!compiled.some(function (i) { return i.check(defaultData) })) {
64-
throw new ReferenceError("Default route doesn't match any known routes")
64+
throw new ReferenceError("Default route doesn't match any known routes.")
6565
}
6666
}
6767

@@ -87,8 +87,8 @@ module.exports = function($window, mountRedraw) {
8787

8888
assign(data.params, $window.history.state)
8989

90-
function fail() {
91-
if (path === defaultRoute) throw new Error("Could not resolve default route " + defaultRoute)
90+
function reject(e) {
91+
console.error(e)
9292
setPath(defaultRoute, null, {replace: true})
9393
}
9494

@@ -123,13 +123,17 @@ module.exports = function($window, mountRedraw) {
123123
else if (payload.onmatch) {
124124
p.then(function () {
125125
return payload.onmatch(data.params, path, matchedRoute)
126-
}).then(update, fail)
126+
}).then(update, path === defaultRoute ? null : reject)
127127
}
128128
else update("div")
129129
return
130130
}
131131
}
132-
fail()
132+
133+
if (path === defaultRoute) {
134+
throw new Error("Could not resolve default route " + defaultRoute + ".")
135+
}
136+
setPath(defaultRoute, null, {replace: true})
133137
}
134138
}
135139

@@ -157,12 +161,11 @@ module.exports = function($window, mountRedraw) {
157161
$window.addEventListener("hashchange", resolveRoute, false)
158162
}
159163

160-
return mountRedraw.mount(root, {
164+
mountRedraw.mount(root, {
161165
onbeforeupdate: function() {
162166
state = state ? 2 : 1
163167
return !(!state || sentinel === currentResolver)
164168
},
165-
oncreate: resolveRoute,
166169
onremove: onremove,
167170
view: function() {
168171
if (!state || sentinel === currentResolver) return
@@ -172,6 +175,7 @@ module.exports = function($window, mountRedraw) {
172175
return vnode
173176
},
174177
})
178+
resolveRoute()
175179
}
176180
route.set = function(path, data, options) {
177181
if (lastUpdate != null) {

Diff for: ‎api/tests/test-mountRedraw.js

+154
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,30 @@ o.spec("mount/redraw", function() {
9191
o(spy3.callCount).equals(2)
9292
})
9393

94+
o("should not redraw when mounting another root", function() {
95+
var el1 = $document.createElement("div")
96+
var el2 = $document.createElement("div")
97+
var el3 = $document.createElement("div")
98+
var spy1 = o.spy()
99+
var spy2 = o.spy()
100+
var spy3 = o.spy()
101+
102+
m.mount(el1, {view: spy1})
103+
o(spy1.callCount).equals(1)
104+
o(spy2.callCount).equals(0)
105+
o(spy3.callCount).equals(0)
106+
107+
m.mount(el2, {view: spy2})
108+
o(spy1.callCount).equals(1)
109+
o(spy2.callCount).equals(1)
110+
o(spy3.callCount).equals(0)
111+
112+
m.mount(el3, {view: spy3})
113+
o(spy1.callCount).equals(1)
114+
o(spy2.callCount).equals(1)
115+
o(spy3.callCount).equals(1)
116+
})
117+
94118
o("should stop running after mount null", function() {
95119
var spy = o.spy()
96120

@@ -204,6 +228,136 @@ o.spec("mount/redraw", function() {
204228
o(function() { m.mount(root, {}) }).throws(TypeError)
205229
})
206230

231+
o("skips roots that were synchronously unsubscribed before they were visited", function() {
232+
var calls = []
233+
var root1 = $document.createElement("div")
234+
var root2 = $document.createElement("div")
235+
var root3 = $document.createElement("div")
236+
237+
m.mount(root1, {
238+
onbeforeupdate: function() {
239+
m.mount(root2, null)
240+
},
241+
view: function() { calls.push("root1") },
242+
})
243+
m.mount(root2, {view: function() { calls.push("root2") }})
244+
m.mount(root3, {view: function() { calls.push("root3") }})
245+
o(calls).deepEquals([
246+
"root1", "root2", "root3",
247+
])
248+
249+
m.redraw.sync()
250+
o(calls).deepEquals([
251+
"root1", "root2", "root3",
252+
"root1", "root3",
253+
])
254+
})
255+
256+
o("keeps its place when synchronously unsubscribing previously visited roots", function() {
257+
var calls = []
258+
var root1 = $document.createElement("div")
259+
var root2 = $document.createElement("div")
260+
var root3 = $document.createElement("div")
261+
262+
m.mount(root1, {view: function() { calls.push("root1") }})
263+
m.mount(root2, {
264+
onbeforeupdate: function() {
265+
m.mount(root1, null)
266+
},
267+
view: function() { calls.push("root2") },
268+
})
269+
m.mount(root3, {view: function() { calls.push("root3") }})
270+
o(calls).deepEquals([
271+
"root1", "root2", "root3",
272+
])
273+
274+
m.redraw.sync()
275+
o(calls).deepEquals([
276+
"root1", "root2", "root3",
277+
"root1", "root2", "root3",
278+
])
279+
})
280+
281+
o("keeps its place when synchronously unsubscribing previously visited roots in the face of errors", function() {
282+
errors = ["fail"]
283+
var calls = []
284+
var root1 = $document.createElement("div")
285+
var root2 = $document.createElement("div")
286+
var root3 = $document.createElement("div")
287+
288+
m.mount(root1, {view: function() { calls.push("root1") }})
289+
m.mount(root2, {
290+
onbeforeupdate: function() {
291+
m.mount(root1, null)
292+
throw "fail"
293+
},
294+
view: function() { calls.push("root2") },
295+
})
296+
m.mount(root3, {view: function() { calls.push("root3") }})
297+
o(calls).deepEquals([
298+
"root1", "root2", "root3",
299+
])
300+
301+
m.redraw.sync()
302+
o(calls).deepEquals([
303+
"root1", "root2", "root3",
304+
"root1", "root3",
305+
])
306+
})
307+
308+
o("keeps its place when synchronously unsubscribing the current root", function() {
309+
var calls = []
310+
var root1 = $document.createElement("div")
311+
var root2 = $document.createElement("div")
312+
var root3 = $document.createElement("div")
313+
314+
m.mount(root1, {view: function() { calls.push("root1") }})
315+
m.mount(root2, {
316+
onbeforeupdate: function() {
317+
try { m.mount(root2, null) } catch (e) { calls.push([e.constructor, e.message]) }
318+
},
319+
view: function() { calls.push("root2") },
320+
})
321+
m.mount(root3, {view: function() { calls.push("root3") }})
322+
o(calls).deepEquals([
323+
"root1", "root2", "root3",
324+
])
325+
326+
m.redraw.sync()
327+
o(calls).deepEquals([
328+
"root1", "root2", "root3",
329+
"root1", [TypeError, "Node is currently being rendered to and thus is locked."], "root2", "root3",
330+
])
331+
})
332+
333+
o("keeps its place when synchronously unsubscribing the current root in the face of an error", function() {
334+
errors = [
335+
[TypeError, "Node is currently being rendered to and thus is locked."],
336+
]
337+
var calls = []
338+
var root1 = $document.createElement("div")
339+
var root2 = $document.createElement("div")
340+
var root3 = $document.createElement("div")
341+
342+
m.mount(root1, {view: function() { calls.push("root1") }})
343+
m.mount(root2, {
344+
onbeforeupdate: function() {
345+
try { m.mount(root2, null) } catch (e) { throw [e.constructor, e.message] }
346+
},
347+
view: function() { calls.push("root2") },
348+
})
349+
m.mount(root3, {view: function() { calls.push("root3") }})
350+
o(calls).deepEquals([
351+
"root1", "root2", "root3",
352+
])
353+
354+
m.redraw.sync()
355+
o(calls).deepEquals([
356+
"root1", "root2", "root3",
357+
"root1", "root3",
358+
])
359+
})
360+
207361
components.forEach(function(cmp){
208362
o.spec(cmp.kind, function(){
209363
var createComponent = cmp.create

Diff for: ‎api/tests/test-router.js

+13-1
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,9 @@ o.spec("route", function() {
6969
}
7070
}
7171

72+
// In case it doesn't get reset
73+
var realError = console.error
74+
7275
o.beforeEach(function() {
7376
currentTest = nextID++
7477
$window = browserMock(env)
@@ -79,11 +82,16 @@ o.spec("route", function() {
7982
mountRedraw = apiMountRedraw(coreRenderer($window), throttleMock.schedule, console)
8083
route = apiRouter($window, mountRedraw)
8184
route.prefix = prefix
85+
console.error = function() {
86+
realError.call(this, new Error("Unexpected `console.error` call"))
87+
realError.apply(this, arguments)
88+
}
8289
})
8390

8491
o.afterEach(function() {
8592
o(throttleMock.queueLength()).equals(0)
8693
currentTest = -1 // doesn't match any test
94+
console.error = realError
8795
})
8896

8997
o("throws on invalid `root` DOM node", function() {
@@ -1082,11 +1090,13 @@ o.spec("route", function() {
10821090
var matchCount = 0
10831091
var renderCount = 0
10841092
var spy = o.spy()
1093+
var error = new Error("error")
1094+
var errorSpy = console.error = o.spy()
10851095

10861096
var resolver = {
10871097
onmatch: lock(function() {
10881098
matchCount++
1089-
return Promise.reject(new Error("error"))
1099+
return Promise.reject(error)
10901100
}),
10911101
render: lock(function(vnode) {
10921102
renderCount++
@@ -1104,6 +1114,8 @@ o.spec("route", function() {
11041114
o(matchCount).equals(1)
11051115
o(renderCount).equals(0)
11061116
o(spy.callCount).equals(1)
1117+
o(errorSpy.callCount).equals(1)
1118+
o(errorSpy.args[0]).equals(error)
11071119
})
11081120
})
11091121

Diff for: ‎docs/change-log.md

+8
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,14 @@
1717
1818
### Upcoming...
1919
20+
*Note for later: release as semver-minor.*
21+
22+
- Improved error messages in multiple places. ([#2536](https://github.com/MithrilJS/mithril.js/pull/2536) [@isiahmeadows](https://github.com/isiahmeadows))
23+
- The redraw reentrancy check was moved from `m.mount` to `m.render` and its error message was updated accordingly. ([#2536](https://github.com/MithrilJS/mithril.js/pull/2536) [@isiahmeadows](https://github.com/isiahmeadows))
24+
- This is unlikely to break people because if you were to do it with `m.render` directly before now, you'd corrupt Mithril's internal representation and internal errors could occur as a result. Now, it just warns you.
25+
- For a better debugging experience with `m.route` route resolvers, errors on `onmatch` in the default route are left unhandled and errors in `onmatch` in other routes are logged to the console before redirecting. ([#2536](https://github.com/MithrilJS/mithril.js/pull/2536) [@isiahmeadows](https://github.com/isiahmeadows))
26+
- Bug fix with `m.redraw` where if you removed a root that was previously visited in the current redraw pass, it would lose its place and skip the next root.
27+
2028
-->
2129

2230
### v2.0.4

0 commit comments

Comments
 (0)