Move Routes to a component

[andresmgot]

From: #618 (comment)

Move Routes to a component and get state trough connect. cc/ @prydonius

[migmartri]

I might be missing the point here, but this seems to me like an unnecessary level of indirection for not using the store directly.

Added also some comments wrt the location of some definitions.

[migmartri]

Do these need to be in the container? Why not in the component?

[prydonius]

I guess the reason @andresmgot put this here is because of the need to import all the other containers. It might be weird for the component to import the containers, what do you think @migmartri?

[migmartri]

In my opinion, the container should only include the mapStateToProps and the connect.

Then the component/routes.tsx should be practically the same than it used to be except that now it will pick up the namespace from the props?

[prydonius]

+1, also seems strange to pass it the privateRoutes and routes here since they don't come from the state. wdyt @andresmgot?

EDIT: actually, I think the reason privateRoutes and routes are in the container is because of the importing of other containers.

[andresmgot]

yes, the reason was to import containers from other containers and for the routes to be easier to discover. In any case I can move them to the component.

[andresmgot]

I might be missing the point here, but this seems to me like an unnecessary level of indirection for not using the store directly.

The reasons I understood from @prydonius are for consistency with the current structure and for making the code easier to test without having to work with the storage.

[migmartri]

Thanks @andresmgot

This is good,

Just some food for thought. In my opinion, sometimes, being forced to have both a component and a container in different files does not make sense from the practical POV due to extra indirection, files to maintain and so on.

In this case for example, I agree with you that importing in a component a set of containers is weird (thanks for the explanation) but it is also weird to pass down props that are not redux state dependent.

If we add to this specific case that the RoutesComponent itself does not seem to fully fit in my head a representational component (it does not render any UI), I'd have made all these changes in a single file, in the RoutesContainer one. This is just a matter of preference, your solution is perfectly fine :)

Thanks!

[migmartri]

I believe that this type can also be IRoutesProps

[prydonius]

Hmm @migmartri raises a good point here, so rethinking this what we might want to do is just have the component class inside the container file and have the container connect to that local React component that is defined in the container file. Does that make sense? That would address @migmartri's point about it being an indirection and Routes not being a "real" component, but we still are able to connect to the Redux state correctly and pass in the props we need to the container.

[andresmgot]

... to do is just have the component class inside the container file ...
That would address @migmartri's point about ... not being a "real" component

What I understand from that is to have the component Routes inside the file RoutesContainer. But that would be still a "component", the only thing is that it would be hidden, isn't that weird? Also we would need to export the component class for testing which is a bit weird as well since the component is not meant to be used outside.

[prydonius]

I mean the container is just a concept, right? React doesn't actually have a container type, everything is a component, we just use the separation pattern because it sometimes makes sense to separate the presentational components (components) from components that know more about the state (containers).

The way we implement the container pattern is a bit different because of Redux's connect helpers, but outside of Redux a container would look something like this:

class CommentListContainer extends React.Component {
  state = { comments: [] };
  componentDidMount() {
    fetchSomeComments(comments =>
      this.setState({ comments: comments }));
  }
  render() {
    return <CommentList comments={this.state.comments} />;
  }
}

(taken from https://medium.com/@learnreact/container-components-c0e67432e005)

It's just a component that knows about some state and passes that state to another component as props. With Redux, we are essentially doing the same thing but using the connect and mapState/DispatchToProps helpers.

In this particular case, we don't really see Routes as a presentational component (i.e. in the same way we do AppList), but rather something that is core to configuring the React application. In that sense, I think it doesn't make sense for it to belong in the container. Root.tsx is another example of this, by the way, it's technically a Component, but we see it as a container.

Does that make more sense?

[andresmgot]

okay, as I understand your point is that components under the folder components/ are representational components and containers/ are not (and are used to connect the state). I agree with that. The only minor thing is that I prefer Routes to be a standalone file for simplicity and testing purposes. I hope that's okay, I am updating this PR.

[migmartri]

Thanks for the changes.

[prydonius]

great, thanks!!