[Frontend Rewrite] Bring back current routeName

[w-4]

Often times it's useful to get the route name of the current Page. (index, tag, ..)
I don't think it's possible anymore after #2156.
Could be easily re-implemented:
For mithril v.0.2 inside Page init():

  app.current = new PageState(this.constructor, { routeName: this.props.routeName })

Would make it possible to get it through app.current.get('routeName').

[askvortsov1]

Hmm, I don't know if this exact solution will work, since the attr being set was removed in ccc77dd. Although my choice of commit title was definitely poor (since this isn't obsolete, really), we now provide component classes, not component instances to mithil v2. I'm not sure that we can set this attr on the page components directly... The first thing that comes to mind is keeping track of routeNames somewhere parallel, but that doesn't seem particularly robust. Any thoughts @clarkwinkelmann @datitisev

[dsevillamartin]

I feel like it'd make sense if Application knew the current route name, as it has the current and previous route data. If not, perhaps app.history ?

[askvortsov1]

Problem is, once we generate a route with app.route, there's nothing tying that to the route name. We could, of course, use a hashmap to store routes generated by app.route and map them back to their original route name, but that could grow unweildy pretty quickly.

[clarkwinkelmann]

Can we maybe use a route render method to achieve that?

  for (const key in routes) {
    const route = routes[key];

    map[basePath + route.path] = {
      render() {
        return m(route.component, {routeName: key});
      }
    };
  }

EDIT: but it won't be available from the global scope though... we could use the page's oninit method to then set that value on the global app object

[askvortsov1]

Oooo I like that! Not sure if there's any detriment to always using route resolvers though.... But then again, that was my plan for #2249 too!

EDIT: but it won't be available from the global scope though...

Couldn't we just add that to PageState in oninit? Or somewhere?

[clarkwinkelmann]

Yep, I've edited myself multiple times... I think it would be perfectly reasonable to set that value in the pagestate or elsewhere on the app object from Page.oninit, and the value can be cascaded from a route resolver like shown in my snippet.

[askvortsov1]

@clarkwinkelmann @datitisev is that something we want in the main PR, or as a separate one on top of that?

[clarkwinkelmann]

I'm getting lost with the commits spread across multiple non-merged branches 😅

Since #2255 seems to show that it removes the old routeName, I think it would make sense to revert it there, that'll make one less breaking change to list inside of that PR.

[askvortsov1]

@clarkwinkelmann @w-4 Done in c579d66. Do we want it to be part of attrs, or it's own prop on PageState? attrs seems simpler.

If we are fine with this approach, we'll need to update the relevant code in subscriptions which checks if we're on the following page.

[askvortsov1]

Just realized: I'm not sure you can have nested route resolvers. If not, this could be problematic, as it'd mean that extension devs can't use route resolvers for their own routes at all.

[dsevillamartin]

@askvortsov1 Yeah I don't think that's a thing. I'm also not sure if adding that route resolver would change the behavior with how the pages themselves are rendered, re-rendered & reinitialized...

[askvortsov1]

c579d66...11b0941 should fix it. From local testing and reading, doesn't seem to change render/re-render behavior.

[clarkwinkelmann]

The way I see it, component should only ever be a page component class, not a resolver, like it was in beta 13. In beta 13 there was no way for extensions to define a route resolver, so I don't think this should be added now.

We should create a new issue to decide how we want to let extensions add/use route matchers/resolvers and how to make sure they update the routeName when doing so.

[askvortsov1]

I'm not inherently opposed to restricting route resolvers in extensions, but I would still prefer to keep a structure similar to what I have now, which is more in compliance with what Mithril looks for in route resolvers. Perhaps, instead of accepting it, we could throw an error for now when an extension tries to use a route resolver?

[clarkwinkelmann]

In the interest of keeping the diff as small as possible, I think c579d66 is perfectly enough for now. It fixes this issues, and doesn't introduce any behavior change from beta 13 regarding the creation of routes (apart from passing the page class instead of an instance of the class)

[askvortsov1]

Alright, I'll split the later two commits into their own PR

[dsevillamartin]

doesn't introduce any behavior change from beta 13 regarding the creation of routes

The other commit that allows the use of route resolvers in extensions doesn't either though? One doesn't have to use a route resolver, but that commit allows the use of one if necessary for the use case. What's the point of updating to Mithril 2 for its benefits if we don't allow the use of some of its features?

[clarkwinkelmann]

I'm all for enabling route resolvers for extensions, but I think this should happen outside of the Mithril v2 PR. It's big enough like it is now.

If we're going to allow route resolvers, I think we need to discuss how extensions will register those while still assuring routes are resolved correctly in Flarum, and in this case particularly the routeName feature. I think we also need to create a different key for those in the route object, as to not re-use component. This feature should also be added to the documentation.

So yeah... subject for another issue and another PR IMO.

[askvortsov1]

Generally speaking, I agree more with David on this one, but given the remaining issues we still need to cover (https://discuss.flarum.org/d/24758-beta-14-m2-frontend-rewrite-tests-logs), I'd prefer to take care of those first, and then return to this if we have time (or at the start of the next release cycle).

I'm going to close this issue since routeName is now available again via app.current.get('routeName)

EDIT: no it's not: individual tag pages show up as "index"......

[askvortsov1]

Fixed in 5502056