Allow extensions to use route resolvers

[askvortsov1]

Depends on #2378.

See #2266.

Mithril Route Resolvers are new features in Mithril 2 that allow some logic to be inserted before a route is handled. Essentially, a quick layer before the component: https://mithril.js.org/route.html#routeresolver. Essentially, its 2 methods allow for hacking in:

• Before the page is first rendered (so we could run authentication or use a different component, for instance)
• Before every redraw while on a page

Currently, Route Resolvers are used to pass in the Flarum route name as an attr to the component handling the route. We will also be using them to process 403 and 404 errors in the frontend (instead of having dedicated blade pages).

The current implementation of mapRoutes automatically wraps all components in a route resolver. This is nice, but it doesn't allow extensions to use custom route resolvers in their routes. As such, this PR provides the following options for mapRoute:

• the resolver key can be used to provide an INSTANCE of a route resolver. This instance should define which component should be used, and hardcode the route name to be passed into it. This instance will be used without any modifications by Flarum.
• The resolverClass key AND component key can be used to provide a CLASS that will be used to instantiate a route resolver, to be used instead of Flarum's default one, as well as the component to use. It's constructor should take 2 arguments: (component, routeName). We recommend extending the flarum/utils/DefaultResolver class.
• The component key can be used alone to provide a component. This will result in the default behavior.

We also use a custom route resolver for DiscussionPage so that routing from one post to another in the same discussion results in a scroll, not a full page rerender.

[clarkwinkelmann]

My suggestion would actually be that we add validation for route.component and only accept components there.

Then we can add a second property, like route.resolver which will be checked to be a valid resolver.

I see there's an interface for RouteResolver we could use https://github.com/MithrilJS/mithril.d.ts/blob/84a87fefe18868c1b56a0a9a09efc7443a1fbd0f/index.d.ts#L58

While it might be helpful to provide an extendable base resolver, we need to also make sure devs can register resolvers that don't extend that base resolver, since there would otherwise be no way to have only onmatch or only render

[dsevillamartin]

@clarkwinkelmann Those are TS typings, we can't use them to check in JS. They most likely don't translate to anything in JS.

[clarkwinkelmann]

Yes, I know. I was just saying we should use the typings since that code is TS already.

The part about checking the parameters will have to use basic javascript and not interfaces.

[askvortsov1]

My suggestion would actually be that we add validation for route.component and only accept components there.

Then we can add a second property, like route.resolver which will be checked to be a valid resolver.

I would be alright with that. The only question is, how do we want to handle providing routeName to a custom resolver? I'd say we should expect a setRouteName method on all custom resolvers then?

[clarkwinkelmann]

We don't necessarily need to pass the routeName to the resolver. The resolver could also hard-code its routeName in the case of custom resolvers.

I'd say we add a check in the Page component to log a warning if no attrs.routeName is passed, and let extensions do as they please to pass it in their custom resolvers. If they extend our base resolver, they could re-use most of the logic.

Something else we could do is check whether the route.resolver parameter is a class. If it's a class (or alternatively, that it extends our class), we call new and pass the same parameters as we do to our DefaultResolver. If the parameter isn't a class but just a POJO, we pass it as-it, assuming the routeName is already hard-coded or stored in there.

[askvortsov1]

@clarkwinkelmann how's this?

[clarkwinkelmann]

I'm not sure that resolverClass feature is really needed, but otherwise looking good.

Functionnaly I guess it's the same but why use onmatch and not return m(this.component, [...]) in render() like previously?

[askvortsov1]

Functionnaly I guess it's the same but why use onmatch and not return m(this.component, [...]) in render() like previously?

By doing that, we'd be effectively killing off the ability to use/modify onmatch. It's a useful feature, and I'm planning to use it in #2249 to show error pages when relevant (passing the error details in through the payload).

[franzliedke]

Could the PR description get a few more details on the actual use-case here? What is the intention here? It's a bit difficult sifting through so much discussion about temporary, now-obsolete changes in the huge Mithril branch.

[askvortsov1]

Could the PR description get a few more details on the actual use-case here? What is the intention here? It's a bit difficult sifting through so much discussion about temporary, now-obsolete changes in the huge Mithril branch.

Done, and rebased

[franzliedke]

I still find it hard to see actual concrete use-cases, to be honest. The only concrete ones seems to be the "frontend error handling" one (#2249), and I've raised my concerns about that in the relevant issue and PR.

That said, I would first recommend to spend time on actual abstractions for pages / routes, to get hold of the hacks that were introduced around routing in the upgrade to Mithril 2 and then revisit this if there are actual extension needs - we might not need the resolvers in core anymore, or have better extension points to offer at that time.

[askvortsov1]

Resolvers are a mithril 2 feature, and even if we can't come up with use cases now, I'm not sure why we shouldn't try to support the feature itself, especially when it's not difficult to do so

[dsevillamartin]

I think this is good (???)

[askvortsov1]

I still find it hard to see actual concrete use-cases, to be honest. The only concrete ones seems to be the "frontend error handling" one (#2249), and I've raised my concerns about that in the relevant issue and PR.

That said, I would first recommend to spend time on actual abstractions for pages / routes, to get hold of the hacks that were introduced around routing in the upgrade to Mithril 2 and then revisit this if there are actual extension needs - we might not need the resolvers in core anymore, or have better extension points to offer at that time.

@franzliedke Could you go a bit more into detail about your vision for page/route abstraction? We need to decide whether we want to include this in b14

Some ideas I've had include:

• Instead of onbeforeupdate, having extensions extend the onNewRoute method, to add whatever logic needs to execute. That being said, this could cause a problem with UserPage, since we load in the active user differently from the other UserPage(s)
• Adding a method called newRouteCheck, which would run in onbeforeupdate and return whether we've switched to a new page. This would support custom logic (in IndexPage and DiscussionPage we check the URL, but in UserPage, we check the username, since we don't need a change between multiple UserPages for the same user) (Although that being said, we could switch that one too, since it is actually a route change) (I'm actually starting to think that might be a good idea while I'm typing this out
• In onbeforeupdate, if newRouteCheck, run onNewRoute

This would be a more pleasant API for dealing with this stuff,

[clarkwinkelmann]

I don't think this is an urgent matter, I'd recommend keeping this for the next release cycle.

It would be nice to have at least one (community?) extension making use of this feature to validate its implementation.

[dsevillamartin]

The code looks clean and organized, and nothing else is added apart from the resolver for Discussion so 👍

Not tested locally.