FEATURE: improve blank page syndrome on the activity/topics, activity/read and group messages pages (PR #14313)

An example:

GitHub

I think this thing could be a one liner rather than storing these two in a short lived variable

Very minor comment, otherwise LGTM

Hmm I’m not sure I agree that emptyState is a model concern. I’m curious what was the rational behind moving this to concern to the model? I prefer the previous way of having the route pass the emptyState to the controller.

Sure, I’ve changed this.

I’m not sure I agree that emptyState is a model concern

Why? Can you expand this, please? I honestly believe this is a model concern.

Why do you believe this is a model concern? If this was a model concern, the emptyState will remain the same irregardless of the route that fetches the model. The fact that we’re setting a model’s attribute at the router means this state itself is specific to the route. Therefore, it is a concern of the route itself and not the model.

Why do you believe this is a model concern?

Probably it’s worth saying what is the model in this context. This is not the same thing as Model in Ember Data:

Note that Ember Data also has a feature called a Model, but it’s a separate concept from a route’s model hook (from here).

Model that a route model hook returns is data to render on the page on this route. If this is the /topic route, for example, then the model can be:

{
    title: "",
    posts: [...]
}

But if this is the /about route, the model hook can return something like:

{
    aboutTitle: "",
    aboutDescription: "",
    address: "",
    phone: ""
}

In our specific case, we have both parts: a list of topics + a title and description to show in case the list is empty. And every route return different lists of topics and different empty state placeholders.

It’s one of the main responsibilities of the route to prepare and provide the model for templates it’s going to render. And the route can do it in different ways:

  1. By loading data from one or several endpoints
  2. Without calling any endpoint. Data can be hard-coded in the route, or loaded from a client-side resource file.

The last option can work well for an /about page, or for our empty state placeholders. We have support of localization on the client side, there’s no need to store these strings on the server.

From this point of view, the arguments you provided aren’t really applicable to an ember application:

The fact that we’re setting a model’s attribute at the router means this state itself is specific to the route.

The model is always specific to the route. Different routes return different models.

Therefore, it is a concern of the route itself and not the model.

This is always a concern of route to return data for templates it’s going to render. If this data was loaded from the server or from the client-side resource file, it doesn’t matter. Both approaches or their combination is just fine, depending on our needs.

Speaking of having some data defined on a controller, the documentation explains two possible cases, when we may need it. When thinking of them, it is crucial to keep in mind that controllers are singletons.

  1. We need to compute a value based on the results of the model hook. For example, if the model have the name and the secondName properties, the controller can define a computed property - fullName. And since controllers are singletons:

    we should avoid keeping state that does not derive from either the Model or Query Parameters since these would persist in between activations such as when a user leaves the Route and then re-enters it (from here)

  2. We want to store something that isn’t connected to the model at all and needs to be saved between route activations. For example, if we need to remember if some element on the page is expanded or not, we may define the isExpanded property on the controller.

It may look like one of these cases is about our emptyState property. But if we think about it strictly, emptyState isn’t something that isn’t connected to the model at all or something that is deriving from the model. When the list of topics is empty, the empty state title and body is the model itself, just like title, description and phone is the model for an about page.

I’m curious what was the rational behind moving this to concern to the model? I prefer the previous way of having the route pass the emptyState to the controller.

Now, this wasn’t the only reason why I decided to move it to the model. I just wanted to discuss it firstly. Another reason was that the previous implementation is not idiomatic and fragile.

We have many places where we show a list of topics, but this list implemented not as a component as it should be, but as a route + controller. The problem is that controllers are singletons, so all these lists share only one instance of the user-topic-listcontroller. So, when you set the emptyState property (or any other property) on this controller, it persists when you move to another page that also contains a topic list, until you explicitly set it to the new value before moving. It’s super easy to forget about it, or you even may not be aware that you need to do it.

I fell into this trap myself in https://github.com/discourse/discourse/pull/14165. This is reproducible now. Look, I go from the /messages page to the /activity/read page, but I still see text from the /messages page there:

This will be fixed after this PR is merged. I’m pretty sure there exist other bugs when moving between pages with topic lists, since we set many properties in this way:

I hope we’ll refactor this soon. We need to implement user-topic-list as a component, then state won’t be saving between visits of different routes anymore. Routes will be passing the model to that component and that model will be either a list of topics, either a title and description for empty state. Adding the empty state to the model that we’re discussing now is the first step in that direction.

@AndrewPrigorshnev Thank you for the really detailed response here. Please bear with me while I still have some clarifications.

Model that a route model hook returns is data to render on the page on this route.

This is always concern of the route to prepare data for templates it’s going to render. It doesn’t matter If this data was loaded from the server or from the client-side resource file. Both approaches or their combination is just fine, depending on our needs.

I understand this but my concern is why set it on the topicList model instead of returning a route model like so:

return this.store.findFiltered("topicList", { filter }).then((topiclist) => {
  return { topicList, emptyState: this.emptyState() };  
});

The controller can just check for the emptyState property on the model and renders accordingly. In this particular case, we’re returning an object of topicList type as defined in models/topic-list.js. I don’t think it is the router’s concern in this case to alter the attributes on that object. The router can return a different model but mutating the state of an existing topicList type does not feel right to me.

@AndrewPrigorshnev I don’t want you to feel blocked here so feel free to merge even if you disagree with my latest comment. We can continue the discussion internally but I don’t think it should be blocking you on this change.

@eviltrout @jjaffeux @CvX I’m curious if you have any thoughts about the discussion above.

@tgxworld Oh, I see what you mean, I 100% agree with this:

The router can return a different model but mutating the state of an existing topicList type does not feel right to me.

I’m going to fix it as you suggested, like this:

return this.store.findFiltered("topicList", { filter }).then((topiclist) => {
  return { topicList, emptyState: this.emptyState() };  
});

I admit I have been confused in the past when referencing a model.something in a template when that model is not really a “model”, but some piece of data set in the route.

In that case I will often set the data into a different name on the controller than model to clear it up rather than relying on the automatic model name that Ember uses. It looks like your solution does something similar. I think you landed in a good place.

@tgxworld wanted to apply this suggestion of yours:

return this.store.findFiltered("topicList", { filter }).then((topiclist) => {
  return { topicList, emptyState: this.emptyState() };  
});

But unfortunately, we receive the model that already contains topic_list collection:

So if I do it, we’re going to have ugly calls like this.model.topicList.topic_list.topics.

I still want to fix it as you suggested, I agree that we shouldn’t mutate rest models in model hooks. But I’m probably going to need to simplify the server method as well. I added it to my list with low priority and going to revisit it later.

@tgxworld I also added comments in that places, so everyone will see that it’s better to avoid coping this pattern.