Fix lifecyle method workarounds

[w-4]

Fixes #2376, Fixes #2368

Edit: Using a view component instead of the render route resolver, allows for page components to unload properly instead of diffing.

Using the Link component with the key state (force) attribute would allow for same route name components to unload again like in beta.13. So it would be possible to use oninit and oncreate for those without workarounds.

Edit2: New approach uses now again route resolvers, and removes the need for setRouteWithForcedRefresh and the Link force attribute.

Confirmed

• Frontend changes: tested on a local Flarum installation.

[askvortsov1]

When you say key state, do you mean forcing full rerenders for all m.route.set? This was discussed internally, but we decided against it to be closer to Mithril's standards, and due to UI issues with DiscussionPage.

[w-4]

No, I was talking about the Link component force option, but on a second check I couldn't see it make a difference. The IndexPage seems to work without that though. It unloads the page component when going from one tag to another. For the DiscussionPage, there was an issue when scrolling from the discussion route to the discussion.near route. It unloaded and reinitalized the discussion page. Will take a closer look later.

[w-4]

Ready for review. I removed a bunch of stuff in discussion page to make it work, but couldn't find any bugs or difference in behavior resulting from that.

[askvortsov1]

I'll reply more in depth tomorrow, but I have some concerns about this approach:

• It strays from Mithrils decision not to always rerender on route change. Not a deal breaker for me, but still something to consider.
• It doesn't use route resolvers, which we need for the errors in frontend issue. Although I suppose we could wrap these components in route resolvers but that might be messy
• It requires links to user pages, discussion pages, and index pages to use the force attr. That's quite a few links, and more places to change code than if the cases were coded into pages

That being said, I don't love what we have now either, and there's a lot of issues with the PR for a cleaner API for the current system (the oncreate thing, for instance. We'd need to have another overridable method for oncreate stuff that should be executed onNewRoute.

[w-4]

Regarding the links, I was thinking about making the force attribute the default if we're fully re-rendering the routes either way.
The only thing that we cannot get rid of is the setRouteWithForcedRefresh. For the previous approach with route resolvers, I think we can remove it and it also doesn't seem to work. Here's the excerpt from the mithril docs:

Note that the key parameter works only for component routes. If you're using a route resolver, you'll need to use a single-child keyed fragment, passing key: m.route.param("key"), to accomplish the same.

https://mithril.js.org/route.html#key-parameter

Based on that it seems to be also possible to have route resolvers fully re-render the page. Let me check.

[w-4]

New approach uses now again route resolvers, and removes the need for setRouteWithForcedRefresh and the Link force attribute.

[askvortsov1]

Thank you so much for working on this, it's a LOT nicer than the current implementation. I still need to try it out locally, but if it works, I think we should include this in beta 14.

Are we leaving the force parameter and setRouteWithForcedRefresh so that a full rerender can be done while on the same page? (e.g. for LinkButtons on IndexPage or the logo/home link in the navbar?) Also, if we're switching to this system, perhaps LinkButtons should have force set to true by default (I don't think that would break anything). That further minimizes changes in behavior from beta 13

[w-4]

I think we could remove the setRouteWithForcedRefresh function and also the force attribute, to not cause confusion in beta.14. When I tested, it didn't have to have any effect when used together with the route resolvers.

[askvortsov1]

We'll want to keep those so that we can force rerender index page by clicking the logo/forum name. We might also want to add force=True to LinkButtons by default, that way we keep the behavior of refreshing the index page by clicking LinkButtons in nav.

[w-4]

Yes, what I wanted to say is that I think we might need a different solution to fix that. If I go https://nightly.flarum.site and click the home link it doesn't refresh the page for me, when I'm already on the '/' route.

[askvortsov1]

Tested locally, haven't encountered any issues. Also, this implementation has turned out to be very clean, thanks for working on this!

[askvortsov1]

For the link, frankly, this looks like another thing (accidentially perhaps) fixed by this PR: locally, setRouteWithForcedRefresh doesn't work, but on your branch it does. Let's keep the setRouteWithForcedRefresh for History.home(). Do we also want to add it to LinkButton as well? If splitting off into another PR, might be worth adding there.

[w-4]

I've split off the Link stuff into a new PR: #2382

[askvortsov1]

Before we merge this in, I played around with the route resolver system a bit, and was able to recover functionality for scrolling from one post to another while on the same discussion page if changing URL via m.route.set. And since this is done with route resolvers, it'll only be triggered when moving from one page to another with m.route.set, so no weird random scrolling while doing something unrelated.

I've put the new mapRoutes in https://hastebin.com/ikidecawuv.js

If we include that in this PR, we fully retain that behavior from beta 13, which means less work for extensions (and also cleaner code imo)

[w-4]

I think if we want to preserve that, we could abstract it into a Page shouldUnload function to keep page-specific logic out of mapRoutes. What would be the issue of replacing m.route.set with app.current.get('stream').goToNumber in extensions, when you want to scroll rather than reload?

[askvortsov1]

I think if we want to preserve that, we could abstract it into a Page shouldUnload function to keep page-specific logic out of mapRoutes.

I agree with that line of thinking, but not sure about placing it in Page. We'd need to use a static method, called from the route resolver. I'm hoping to replace the route resolver in mapRoutes with extensible classes in #2275; key generation could become a method, and pages that need to override (e.g. DiscussionPage) could use their own route resolvers inheriting from the default one to do so.

What would be the issue of replacing m.route.set with app.current.get('stream').goToNumber in extensions, when you want to scroll rather than reload?

I started with that (introducing a util to make things simpler), but it still led to very messy code. In some cases (read: mentions), we just have a post number/ID, and we don't want to unnecessarily load in stuff from the API, so we need duplicate logic there. In other cases (read: notifications), all we have is a url, so we'd need to parse that out and match it. It's small changes, but they're messy code-wise (a lot of preventDefault in link onclicks, which feels off to me), and would be needed for a lot of extensions, whereas this is a fairly universal solution, and is consistent with current behavior in beta 14 without the bugs of adjusting scroll on every redraw (and if rerendering the page is absolutely necessary, force is still an option).

[w-4]

Ok, makes sense, let's split that into a separate PR?

[askvortsov1]

For the hastebin I linked above or the route resolver into class thing?

While the hastebin I linked is sizeable, including it means that this PR doesn't change functionality when it comes to m.route.set for discussion pages, so I think it might be best to include it to keep (closer to) a working master branch. That being said, that's just my opinion.

If you mean route resolvers into classes, then yeah I absolutely agree

[w-4]

The hastebin code might take more work, so I think it's better to have a new PR to keep the diff smaller. If possible maybe we can avoid importing DiscussionPage into maproutes since it's in the common namespace, which would also import it (and as a result also PostStream and other large compents) into the admin.js bundle file.

[askvortsov1]

You bring up a very good point about minimizing bundle size. In that case, we might want to escalate #2275 for this release, so that we can define a custom route resolver for DiscussionPage that would contain this custom logic. If we want to have more internal discussion, we might want to avoid exporting the route resolver classes in compat until beta 15.

[dsevillamartin]

🤔 Not sure what I'm supposed to test with this PR, but nothing seems broken. However, the behavior of links in e.g. profile page vs index page isn't the same - index page sidebar links do not "force" the route (or whatever the terminology we're using is) unlike the home button or the profile page sidebar.

Apart from that, looks like stuff works 🤷

[askvortsov1]

Iirc, that particular piece (setting Link Buttons to force by default) wasn't included here to minimize the diff and avoid unnecessary changes. Goal here is to change the rerendering system, that can be taken care of separately.

[dsevillamartin]

Hm I was just mentioning it because those are the only buttons that don't have that behavior now - the sidebar in user page does. Ok, I'll approve.