Mithril.js: [rewrite: router] Router API discussion

[pygy]

Edit: The original title of the issue was

"[rewrite: router] Provide a consistent API for canceling the finalization of a route change from the onmatch handler."

---

Two of the tests added in #1466 require returning new Promise(function(){}) for passing. If you don't return that value, the corresponding render fires.

Having different ways to cancel the finalization of a route change depending on the way the redirection is performed seems unoptimal to me.

I'd suggest a providing a forever-pending Promise as m.route.reject over what I had initially proposed in #1462, and getting rid of the m.route.set logic.

[lhorie]

from a discussion w/ @DavidBindloss on gitter last night, I implemented behavior to redirect to defaultRoute on promise rejection (overrideable via .catch)

In that context, I think reject is the wrong term for what this proposal is about. My gut feel is that preventing render is something you're only ever going to do in conjunction w/ redirecting, so I'm having a hard time coming up with realistic scenarios where you would want to skip render and do something else. The case for history.back that you brought up in the other thread strikes me as a case of something that is technically possible but highly unlikely.

Note, I'm not saying there aren't valid use cases, I'm just saying I can't think of one.

Also, imho, returning a magic callback that is provided by the framework isn't really any more discoverable than returning a naked pending promise, so I'm kinda -1 on this

[pygy]

m.route.hold = new Promise(identity) then?

Also, imho, returning a magic callback that is provided by the framework isn't really any more discoverable than returning a naked pending promise

If you're writing an app from scratch while discovering the framework, I agree, but m.route.hold can be searched for more easily than new Promise(function(){}) if someone stumbles on it in an existing code base.

[pygy]

A potential drawback of this approach is memory leaks... Won't the parent promise be forever held somewhere if we return one that is never resolved?

[lhorie]

I'm not 100% sure, but my gut says something would leak.

[pygy]

Another thing that bothers me about the current design is the fact that bail set as a reject handler will swallow diff exceptions. Edit: oh, nevermind, that's wrong, it will only swallow onmatch errors.

Maybe we could mandate a return/resolution value from onmatch and provide m.route.pass, m.route.hold and m.route.bail as special tokens and dispatch on it on resolution?

[pygy]

Another thing that's bugging me is the fact that now the timing of mounting a routed tree differs depending on whether an explicit onmatch is provided. When present, mounting is async (even if onmatch returns a value, not a promise). When absent, mounting is synchronous.

Given how tricky timing is to get right, even when built on top of consistent a API, we don't do users a service by providing an inconsistent one.

Edit: Making it all async of course breaks most of the test suite (336 bad assertions after fixing null refs that were crashing the script). The resulting code is almost identical to #1290 (but with promises).

routeService.defineRoutes(routes, function(payload, params, path) {
    if(payload == null || typeof payload !== 'object') throw new Error('Component or RouteResolver expected as route end point')

    var isComponent = typeof payload.view == 'function'
    var comp = isComponent ? payload : null
    var routeResolver = isComponent ? {} : payload

    var update = lastUpdate = function() {
        if (update !== lastUpdate) return
        component = comp != null && typeof comp.view === "function" ? comp : "div", attrs = params, currentPath = path, lastUpdate = null
        render = (routeResolver.render || identity).bind(routeResolver)
        run()
    }

    Promise.resolve(typeof routeResolver.onmatch === 'function' ?  routeResolver.onmatch(params, path) : comp).then(
        function(resolved) {
            comp = resolved
            update()
        },
        bail
    )
}, bail)

[lhorie]

it will only swallow onmatch errors

It will swallow returned rejected promises (and that's intended behavior), but thrown errors just unwind the stack normally (because only the result of the onmatch call is Promise.resolve'd (as opposed to wrapping the whole call in a promise context)

Given how tricky timing is to get right, even when built on top of consistent a API, we don't do users a service by providing an inconsistent one

Ok, I think framing this as a Zalgo thing is wrong. The thing to keep in mind is that due to the nature of onmatch, there's no guarantee that a route will be resolved after one, two or even a hundred event loop ticks, so whether you write code assuming synchronous resolution or assuming a setTimeout/rAF/Promise.resolve delay, your code would still be guaranteed to break under some conditions.

The only sensible API to get around that would be a callback (since route changes are repeatable events, promises don't cut it). But in that case, you might as well call your code in the onmatch promise chain or in render or in a lifecycle method.

Going back to the original topic: I'm really struggling to see a compelling case for returning a pending promise from onmatch, and I don't think it makes much sense to have an API point that exists solely to return one to cover the unlikely event of someone not resolving an onmatch AND not rerouting. At that point, you might as well just make the promise yourself and comment why you're doing such a weird thing.

[isiahmeadows]

@lhorie I agree with the general sentiment (and your last bit on the original topic itself); just thought I'd point out a little nit:

It will swallow returned rejected promises (and that's intended behavior)

That would kind of suck for anyone using async functions as resolvers (which always return promises, even on synchronous rejection). For similar reasons, this is part of why I suggested making m.route.set return a promise. I'm not too attached to it, though, so ¯_(ツ)_/¯

[lhorie]

That would kind of suck for anyone using async functions

Why? You can .catch and do your own thing before returning if you want to, and if you forget, Mithril bumps you over to the default route. I think that's fairly reasonable behavior.

[isiahmeadows]

@lhorie Async functions don't have .catch, and errors thrown synchronously from async functions translate into synchronous rejections (i.e. they don't bubble up at all). That's why I brought it up.

[barneycarroll]

@isiahmeadows an async function returns a Promise, nothing more or less. That Promise exhibits catch as normal.

[pygy]

The onError channel is supposed to deal with exceptions, as catch blocks would in sync code (it is actually mapped to catch blocks in code that uses the ES7 async syntax).

Would you redirect to the default route if an error was thrown in onmatch? If not, Promise rejection shouldn't IMO have that behavior either.