Mithril 2 update

[dsevillamartin]

Supersedes #2126.

Some minor TypeScript changes will be here because it makes the development easier. Don't yell at us.

Decisions made thus far:

• mithril 2.0 does not rerun oninit hooks for page components when a change in route results in the same page. There is general agreement that we want to keep / allow the old behavior. The current implementation is a well documented patch in patchMithril.js, but we are happy to consider alternatives.
• Children is no longer passed to AlertManagerState through attrs; it is now the first argument (attrs is second, componentClass isthird). We might want to switch the order to have attrs before children, but I feel like children is more likely to be provided.
• onunload methods have been merged with context.onunload (from config) into onremove. I am not sure if this is the right way to do this, but it seems to make sense and hasn't broken anything so far
• Instead of returning a retain vnode, subtree retainer now returns a boolean value, which can be used in onbeforeupdate.
• in many cases, we pass a "config" attr to some html element instead of creating a custom component for it. Since there are now multiple life cycle hooks, not just config, passing multiple of those would be inconvenient. If multiple hooks, or logic more than a few lines, needs to be passed as attrs to a component, I am creating a new component for it (e.g. CommentPostPreview, ReplyPlaceholderPreview)
• despite the consensus in the previous team meeting to not store this.attrs, on implementation, that has turned out more difficult than expected, as we'd need to pass attrs through long chains of sometimes more than 5 methods. We have reverted to using this.attrs, and are also keeping initAttrs (although thst is NOT static anymore)
• we are no longer using attrs.children to pass in children. We've even added a warning error message to deter against this.
• In some cases, attrs would be passed to an html vnode, which are distinct from the attrs passed to the component. Previously, these were managed via an attrs method. However, since we now use this.attrs, that will not work. We are renaming those methods to elementAttrs.
• mapRoutes no longer sets a routeName attr on components
• All frontend deprecated code has been removed, except for various warnings/errors designed to prevent wrong inputs.

References to props in comments have not been changed; they will be removed when we introduce attr typing in the typescript part of the rewrite.

Some issues that need fixes

• e0fce1c is messy... Do we want to abstractify these components, and bring them out of this file?

Extracted fixes:

• Extract a few changes from the Mithril 2 upgrade #2262
• Fix opening modals from other modals. #2263
• Revert to using state.needsScroll for triggering scrolls with PostStream #2264

[askvortsov1]

Decisions made thus far:

• mithril 2.0 does not rerun oninit hooks for page components when a change in route results in the same page. There is general agreement that we want to keep / allow the old behavior. The current implementation is a well documented patch in patchMithril.js, but we are happy to consider alternatives.
• Children is no longer passed to AlertManagerState through attrs; it is now the first argument (attrs is second, componentClass isthird). We might want to switch the order to have attrs before children, but I feel like children is more likely to be provided.
• onunload methods have been merged with context.onunload (from config) into onremove. I am not sure if this is the right way to do this, but it seems to make sense and hasn't broken anything so far
• Instead of returning a retain vnode, subtree retainer now returns a boolean value, which can be used in onbeforeupdate.
• in many cases, we pass a "config" attr to some html element instead of creating a custom component for it. Since there are now multiple life cycle hooks, not just config, passing multiple of those would be inconvenient. If multiple hooks, or logic more than a few lines, needs to be passed as attrs to a component, I am creating a new component for it (e.g. CommentPostPreview, ReplyPlaceholderPreview)
• despite the consensus in the previous team meeting to not store this.attrs, on implementation, that has turned out more difficult than expected, as we'd need to pass attrs through long chains of sometimes more than 5 methods. We have reverted to using this.attrs, and are also keeping initAttrs (although thst is NOT static anymore)
• we are no longer using attrs.children to pass in children. We've even added a warning error message to deter against this.
• In some cases, attrs would be passed to an html vnode, which are distinct from the attrs passed to the component. Previously, these were managed via an attrs method. However, since we now use this.attrs, that will not work. We are renaming those methods to elementAttrs.
• affixSidebar util has been removed in favor of an AffixedSidebar component
• mapRoutes no longer sets a routeName attr on components
• All frontend deprecated code has been removed, except for various warnings/errors designed to prevent wrong inputs.

References to props in comments have not been changed; they will be removed when we introduce attr typing in the typescript part of the rewrite.

Some issues that need fixes

• Notifications linking to external sites might not link to them properly. Can we use external URLs with m.route.link?
• e0fce1c is messy... Do we want to abstractify these components, and bring them out of this file?

[franzliedke]

Found a bunch of stuff already. Haven't looked at all files yet, though.

[clarkwinkelmann]

I'd like to suggest we create a util method or Mithril patch for m.route.set('/', null, { state: { key: Date.now() } });. It's not obvious what it does and it would be much simpler to have it inside of a named method instead of adding a comment everywhere. Something like setRouteWithForcedRefresh()

I have read through the changes in this PR last week and again now and I think it's still not an exhaustive review but I think I agree with the vast majority of changes.

[askvortsov1]

I'd like to suggest we create a util method or Mithril patch for m.route.set('/', null, { state: { key: Date.now() } });. It's not obvious what it does and it would be much simpler to have it inside of a named method instead of adding a comment everywhere. Something like setRouteWithForcedRefresh()

That's a great idea! Also allows us to document this centrally. Done!

[clarkwinkelmann]

All good regarding my previous comments. Not sure if I should do another read-through before the end of the week or if it's enough to wait on others to complete their own review?

[askvortsov1]

Ooo 2 approving reviews, does that mean we can merge 🤣
(just to clarify, no merge until everyone agrees or Franz overrides. This approval is largely symbolic in that I think thisr implementation is sufficient to not diminish extensibility, and does not introduce bugs (that I have been able to find))

[dsevillamartin]

We still have some lazyRedraws left behind - see https://github.com/flarum/core/blob/4c4c148c6e3fe6c951941762a685ab42f2329453/js/src/common/Model.js#L183

[franzliedke]

A few post stream questions for @askvortsov1

[franzliedke]

Congrats! 🎉