Frontend Needs Proper State Management

[askvortsov1]

Architecture

Is your feature request related to a problem? Please describe.
Right now, our frontend uses somewhat of a haphazard state management / propogation system. We have:

• Local state (is a page loading or not)
• Global state (all the discussions that have been retrieved from the database

This means that the displayed UI can be modified by:

1. Modifying parts of the component instances directly (which is bad)
2. Modifying the components' local states (which is slightly better, but still bad)
3. Modifying the global state, and having changes propogate during redraw (this is considered the best and is most consistent with unidirectional data flow).

Describe the solution you'd like
This should probably be done gradually, one step at a time. Radical change is probably not needed, however, the following steps should be individually taken:

1. Components should NOT be modified by externally calling methods on an instance of said virtual component. This means getting rid of app.current, app.modal.component, and any other similar systems. Instead, modifications should be done to the global state, and components should be redrawn as appropriate after said state modifications.
2. Adopt a central state management paradigm. Right now, we have a mutable global state, which isn't ideal. We should try to gradually convert to something more robust. Meiosis looks like it could be a simple and elegant solution, although there are plenty of options (Redux and Flux being the big ones)
3. Starting with the most extendable areas, try to convert miscategorized local state to global state when appropriate.

In my opinion, (1) should be included in #1821. (2) should be aimed for before stable. (3) can be done after stable

Benefits
If done properly, this should:

• Make the frontend more extendable
• Increase robustness, as things will be changing only when they should be changing
• Make code easier to update/change without worrying about breaking everything
• Respect better practices
• Make systems more testable

[askvortsov1]

Part 1

In a sequence of PRs, I'm going to convert:

app.current
app.search
app.modal
app.alerts
app.cache.discussionList

away from components. That is, the values stored in app.* will all be states and methods relating to changing states, not component instances. Those changes can be reviewed individually, and will make the frontend rewrite easier actually, as the mechanism for changing state should still work with Mithril v2. Considering that the rewrite would depend on this, I will make this a top priority, and try to get the PRs out within the next 2 days. Considering a general lack of inderdependence, I'm hoping that each of these can be its own PR, which should make reviewing easier.

Just to clarify again why this is necessary: aside from promoting good practice, we're either going to need to go this route, or use hacky workarounds in the rewrite which will come back to bite us later.

[clarkwinkelmann]

I'm a bit confused by the term "state", because each case here is very different. Could we talk about the approach for each in detail without starting the changes? Maybe a separate issue for each one could make sense?

In my opinion:

app.current should store the current page component class instead of component instance, that way we can continue to know which page is currently shown, which I think was the main intent of that accessor.

app.search: having seen the PR and the discussions on the forum, we might have to move more of the state so that other components can modify the current search value.

app.modal: ModalManager should be turned into a non-component class. The view/render method can continue to be used to inject the content into Mithril. Instead of storing a component instance, we should store the component class and props, so that Mithril can handle the internal state of each modal component natively. The onshow and onhide methods are the only thing that required storing app.modal.component it seems. I think those methods could be turned into static methods on the component class, there's no need for those to be called on the component instance.

app.alerts: very similar to modals. The AlertManager should be turned into a simple class. The only thing that required to store the component instance seems to be the ability to hide a specific alert. Maybe we could change this to work similarly to setTimeout/clearTimeout, with the show() method returning an alert ID which you can then use to hide that specific modal. Alternatively, the show() method could return a special object that has a dismiss() method that will close that specific alert.

app.cache.discussionList: this one definitely sounds like the full state should be managed in a simple class, and the DiscussionList component should simply show the data stored in the global state.

For modals and alerts, what I've used in my own v1/v2 Mithril-based applications is a manager that takes the class and props. That way there's no need to call new on a class, and no component states are needed. Instead the manager just injects m(class, props).

I wouldn't call the modal and alert managers "states". I think the name "manager" is perfectly fine, it just stores information on the component currently rendered and crafts the vnodes. It just shouldn't be a component itself.

[dsevillamartin]

For app.modal, fof/components uses app.modal.component.setting() - is it needed? Not necessarily, but it does access the class instance. That's still possible when storing the component class & props if we use oninit and put the vnode.state in the manager class.

If you take at the current rewrite Modal Manager, is that basically what you are suggesting for it?

[askvortsov1]

app.current should store the current page component class instead of component instance
I would agree. Perhaps we could/should also store the route? Same with previous btw.

we might have to move more of the state so that other components can modify the current search value
It seems like most of the Search.js functionality is modifying the HTML of the searching component. I wouldn't be opposed to factoring some of that (especially getCurrentSearch, which is kind of the exception as it falls under app.current/app.previous, which is why it wasn't included in that PR) out into a separate state thing

---

Agree with everything you said about modals and alerts. However, we do still need to mount HTML/JSX probably (the stuff in the view() methods of each), so not sure where those should go? @datitisev also had the idea of assigning an ID to the 'alert state' props stored in AlertManager, so that whatever component requested it could store that ID and de-activate an alert externally, which I think is a great idea.

this one definitely sounds like the full state should be managed in a simple class, and the DiscussionList component should simply show the data stored in the global state.

This one is done (and largely agreed upon except for one minor issue caused by the change) in the relevant PR, #2150

[clarkwinkelmann]

However, we do still need to mount HTML/JSX probably (the stuff in the view() methods of each), so not sure where those should go?

I suggest replacing

this.modal = m.mount(document.getElementById('modal'), <ModalManager />);

with something like

this.modal = new ModalManager();
m.mount(document.getElementById('modal'), { view: this.modal.view });

EDIT:

or alternatively, create a new component ModalRoot, that takes the ModalManager as a prop, and does what's currently inside of the ModalManager view method. But I don't think it's necessary to create that many classes.

[askvortsov1]

I would probably prefer splitting it up into 2 classes, but I'm fine with the first approach too, as long as ModalManager doesn't need to inherit off of Component (not sure if that would cause any redraw issues?)

[dsevillamartin]

@clarkwinkelmann That won't trigger lifecycle hooks though...?

[clarkwinkelmann]

That won't trigger lifecycle hooks though...?

No, it wouldn't.

That's the approach I used on my private Mithril projects. I have the ModalManager/AlertManager return vnodes. Then I inject those vnodes either via the global Layout component (usually, I have a single mount point), or those vnodes can be injected via an anonymous component like in my code example above.

There's no lifecycle events required, all we need is for the modal manager to return m(component, props) in a view method somewhere.

[askvortsov1]

I'm not sure I like stuff that isn't a component returning JSX. I'd say we should either put the JSX directly into the mount call, or split ModalManager (and AlertManager) into 2 classes each. As long as we later reorganize the classes so that we don't have 60 files in one directory any more, I'd say we should be fine.

[franzliedke]

A general thought: Do we really want to trigger redraws from the states? Or do we rather want a system where states serve as "observables" of sorts that signal to anyone interested (our app, probably) when changes happen and then that place tells Mithril to redraw?

[askvortsov1]

This is a difficult question, but I'm leaning towards yes on this one. My strictest no-no would be including any jsx in state methods (not the biggest fan of storing JSX in states as in #2163, but that's not AS critical). The way I see it, every state method is either involved in:

• Changing state data
• Reacting to changes in state data by setting off animations / redraws.
• Presenting the public API for components to interact with other components (also bad, at no point should any component have access to an instance of any other component (although we can use events/pass bound functions to circumvent some of the limitations))
  So redraws are fine, as is direct DOM manipulation via JQuery (although when we CAN, I'd prefer to still have that in component classes, especially if it's a big method).

One other thing I'll note is that I view states as an intermediate step on the way to a proper state management methodology, at which point the data in states, the public API of manipulating state would be further split off, and the reducers / observers handle animations and redraws and the like.

But right now, our priority is Mithril v2.0, and we need to de-componentize everything regardless. This wave of PRs is only step 1 of the 3 step plan I outlined above. And since there's now a clear defined public API, when we make the changes in step (2), we can "symlink" the State classes API to the new system we design for BC. That's also why I want to avoid people extending / inheriting from states for now: because they're temporary.

[franzliedke]

Okay, they're temporary, but we can still strive for more consistency then (when it comes to who triggers the redraws). I'll comment inline where I hope for more of that.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

This is a lot better now that we don't store component instances, but let's keep this open for now.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[stale]

We are closing this issue as it seems to have grown stale. If you still encounter this problem with the latest version, feel free to re-open it.

[stale]

We are closing this issue as it seems to have grown stale. If you still encounter this problem with the latest version, feel free to re-open it.