Extract Alert State, AlertManagerState

[askvortsov1]

See #1821, #2144

Changes proposed in this pull request:

• Extract Alert State for each alert
• Extract AlertManagerState for alerts in general
• Mostly a copy paste of work done by David in the draft mithril rewrite branch.

Reviewers should focus on:

• Anything I missed?
• Should we be concerned that we're storing component instances in the controls? We could take care of that as well (store tuples of classes, props) if needed

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

[clarkwinkelmann]

Here are my observations just looking at the code.

I still think it might be a good idea to give the ability to customize the Alert component. What about adding a component property to AlertState, which defaults to Alert. Your code can already handle passing a custom AlertState to show(), so all someone would need to customize the component, is to create the AlertState object themselves. This wouldn't overload the show() method in any way.

[askvortsov1]

I still think it might be a good idea to give the ability to customize the Alert component. What about adding a component property to AlertState, which defaults to Alert. Your code can already handle passing a custom AlertState to show(), so all someone would need to customize the component, is to create the AlertState object themselves. This wouldn't overload the show() method in any way.

After a few other changes to simplify the code/interface, it doesn't slot in 100% as cleanly, so I just put it in as an optional param to alerts.show(). How's this look?

[dsevillamartin]

Why not use the AlertState as a method for passing the attributes and having the key (like my rewrite commit)? I don't like that alertAttrs and alertKey are saved separately.

This also means that extensions that extend Alert (https://github.com/FriendsOfFlarum/terms/blob/572025438f6795e96ce9e8b854a1c31f69b485d1/js/src/forum/components/UpdateAlert.js) will have to do app.alerts.show({ children: <extended alert>.component() }) which seems like a waste.

Is it because we don't want to have DOM in the states? If so, i think we should move past that because we have other state PRs directly modifying the DOM, so there's no point in differentiating that.

I much prefer keeping the old syntax of app.alerts.show(Alert.component // new AlertState({ ... }). show method can still return the key, but it'd be the one stored in the Alert state (and it can be modified directly there, and because it's a class, the changes propagate, i think).

In short, I like my implementation in 4c8af1c better 🙂.

---

I've got nothing to say about the AlertManagerState, FYI.

[askvortsov1]

I don't like that alertAttrs and alertKey are saved separately.

Why not? The 2 aren't inherently related. IMO, the mechanism of keeping track of the currently active alerts should be in AlertManagerState, which is what is done here.

This also means that extensions that extend Alert (https://github.com/FriendsOfFlarum/terms/blob/572025438f6795e96ce9e8b854a1c31f69b485d1/js/src/forum/components/UpdateAlert.js) will have to do app.alerts.show({ children: <extended alert>.component() }) which seems like a waste.

Not necessarily: you can pass in a class subclassing Alert into show, so the structure remains constant. We might want to switch the order of attrs and Alert around though so that it matches what we have for Modal.

Is it because we don't want to have DOM in the states? If so, i think we should move past that because we have other state PRs directly modifying the DOM, so there's no point in differentiating that.

As of right now, I view the states as 2 things:

• A bucket for data
• A public API for modifying other components.

In the future, when we adopt a proper state changing mechanism such as redux, these roles will be further split up. However, for now, our priority is Mithril 2.0 and not storing components, so this intermediate step is fine. For alerts specifically, ideally I'd like to have a class for each type of alert (like we do with Modals), which is where the children and control elements would be held. However, that's not worth delaying the rewrite over.

My concern isn't avoiding anything to do with DOM in states. My concern is when states start to act like components in terms of containing JSX / business logic. I'd like to restrict states to dealing with data and animating stuff around.

I much prefer keeping the old syntax of app.alerts.show(Alert.component // new AlertState({ ... }).

I don't think that passing the component instance directly was supported in the other PR, just the attrs dict or the state object?

[dsevillamartin]

I don't think that passing the component instance directly was supported in the other PR, just the attrs dict or the state object?

It was supported, just not required (I think). This was to allow classes extending AlertState to be passed.

A bucket for data

If the state is a bucket for data, why wouldn't you pass it as you pass the attribute data? That way you have the whole thing and can store it if necessary (why, I don't know, but in case it's needed for some reason).

The key is referencing a specific alert state. Why not just have the alert state stored with the key, instead of storing them separately when you are saving them in things like requestError?

Why not? The 2 aren't inherently related. IMO, the mechanism of keeping track of the currently active alerts should be in AlertManagerState, which is what is done here.

What do you mean, not inherently related? They both relate to a specific alert state?

[dsevillamartin]

I think we should get the opinions of other extension creators on how the data should be stored/accessible. Might shed some new light on this. I know it's not exactly important, but accessing the alert data should be through requestError.alert and not have two separate keys for references to the same object, essentially.

[askvortsov1]

It was supported, just not required (I think). This was to allow classes extending AlertState to be passed.

What's the benefit of extending AlertState? If we keep it minimal, there's not really a need.

If the state is a bucket for data, why wouldn't you pass it as you pass the attribute data? That way you have the whole thing and can store it if necessary (why, I don't know, but in case it's needed for some reason).

My initial idea was to pass the entire state into Alert, iirc @clarkwinkelmann recommended passing just the state.attrs, as that's the only part of the AlertState that's used.

What do you mean, not inherently related? They both relate to a specific alert state?

My initial thought in separating was to make things simpler, but storing the key in the state is just as good tbh. Any objections about going back to that mechanism?

[askvortsov1]

Here's what I'm thinking in terms of next steps:

• Show accepts an AlertState instance.
• Dismiss accepts a key.
• AlertState stores attrs, Alert class (defaults to Alert), and the key
• AlertStates are stored in AlertManagerState via an object of keys => alertStates

[askvortsov1]

As a side note, what if we required that the children props of Alert be a string (in that case, perhaps rename it to title), and if controls/other children are needed, we created a subclass of Alert? I feel that would be cleaner state-wise. Would add extra overhead though.

[dsevillamartin]

Don't think that's necessary or a good idea. Would create extra classes for basically nothing (like our many confirmation modals 🙃)

[franzliedke]

As of right now, I view the states as 2 things:

• A bucket for data
• A public API for modifying other components.

This part I have my doubts about. AlertState seems to fall into the former category, AlertManagerState into the latter. IMO, naming these concepts differently would help a lot in understanding / teaching these concepts.

The "states", as I see them, should be of the second variant: application-wide containers (with a bit of behavior) that accept events (method calls, really) that then trigger actions / data changes across various places of the UI, followed by a redraw.

That Alert class is just that, an object representing an alert. A POJO (Plain Old Javascript Object). It does get confusing, because we also have an Alert component, but that's kinda already solved by our directory structure (even before we make larger changes to that).

---

One of my favorite programming quotes, somewhat relevant:

So much complexity in software comes from trying to make one thing do two things.
– Ryan Singer

[askvortsov1]

My only concern would be importing Alert and Alert, but I suppose we can just use import aliases? Should POJO alert still be in states directory?

[franzliedke]

I wouldn't see it in the states directory, no. It would be a simple value object, and should not do some of the things (change routes, trigger redraws etc.) that "states" are allowed to.

[franzliedke]

Out of interest: what's the idea behind the latest commit ("Encapsulate ...")? There doesn't seem to be much encapsulation going on, especially considering that you added both getters and setters (in their most trivial form) to these attributes.

[askvortsov1]

Out of interest: what's the idea behind the latest commit ("Encapsulate ...")? There doesn't seem to be much encapsulation going on, especially considering that you added both getters and setters (in their most trivial form) to these attributes.

While I hate to be the introductory college java course where encapsulation is taught as religion, I do think it's warranted here: if we continue as planned, the attributes might not be accessible as attributes in the future.

This has been taken care of.

[clarkwinkelmann]

I like it. Just one comment below.