Separation of Concerns Leading to Implicit Dependencies

I’ve been building a toy app to feel out server-side React, Redux, GraphQL, &c. to explore ways to build my next webapp(s). One thing this has introduced me to is react-router. react-router has good support for SSR because it lets you statically determine the set of Component classes that match a route so that you can load their data before rendering them.

Its route descriptions allow nesting, which translates interestingly to your rendered UI. Here’s my routes.js file:

<Route path="/" >
  <IndexRedirect to="/rails/rails" />
  <Route path=":ownerName/:repoName" component={App}>
    <IndexRoute component={IssueListPage} />
    <Route path=":number" component={IssuePage} />
  </Route>
</Route>

This is used to make pages that look something like these: (When in doubt, make a GitHub viewer.)

The App component renders the header with the repo name, and react-router passes it a child component, which will either be an IssueListPage if we’re requesting just “/ownerName/repoName/” or an IssuePage if we want “/ownerName/repoName/number”. It will pass in a params prop object that has all of the :keys and their values from the URL.

I am currently undecided about whether or not reflecting the visual containment of components in the routes description is a good idea. It may be an easy place to build in layout-like functionality, or it might be a mess that hampers your flexibility to make structural UI changes. You can certainly use react-router with a flatter set of Routes if that becomes a problem.

What I’ve been working through now is the relationship between these components and the params. This is where we start to get into separation of concerns.

It was very tempting for me at first to think of the ownerName and repoName params as the concern of the App component, since they’re defined at that level of the nesting. IssueListPage and IssuePage could have the current repository generalized away from them in some way. It doesn’t have to be their business that those values came out of the location field, which means that it could be easier to re-purpose these components somewhere where the repo is specified in a different way.

You could also look at this on DRY grounds: both IssueListPage and IssuePage would have to look in the params for the same values to execute their fetches. What if we want to change the route variable names? Can we just access the params in one place?

In vanilla React, that might look like passing props — either data or functions that close over the data — down into the children, but that won’t work here. While it is possible with react-router to use cloneElement to have an outer container like App adjust the props of a matched child such as IssueListPage, that adjustment happens inside render, which makes it too late for affecting server-side fetches.

On the server, in order to get the data ready before the render, we need to use methods that operate exclusively on the component class and the router’s values. The code for that looks something like this:

static dispatchForRouteParams(params, dispatch) {
  return dispatch(GitHub.loadIssues(params.ownerName, params.repoName));
}

We could instead turn to Redux to have App set up data for IssueListPage. In its disptachForRouteParams, App can dispatch an action that sets a current owner name and repo name based on params. We could then make the state available when it comes time to load for IssueListPage:

static dispatchForRouteParams(params, dispatch, getState) {
  const { ownerName, repoName } = getState().gitHub;
  return dispatch(GitHub.loadIssues(ownerName, repoName));
}

We’ve now isolated the concern of how our app determines which repo is the current one.

Using redux-thunk, which allows us to write actions that can read in current state, we could even move the concern of there even being an owner name and repo name out of this component:

static dispatchForRouteParams(params, dispatch) {
  return dispatch(GitHub.loadIssues());
}

This means that the loadIssues action creation method, with redux-thunk (and redux-pack for the promise handling), looks like:

export const loadIssues = () => (dispatch, getState) => {
  const { ownerName, repoName } = getState().gitHub;
  return {
    type: 'LOAD_ISSUES',
    promise: fetch(`https://api.github.com/v3/repos/${ownerName}/${repoName}/issues`).then((res) => res.json()),
  };
};

Ta-da! Now you can easily load the issues for the current repo without having any idea how it’s specified. Separation! Isolation! We win, right?

This is where I led myself this afternoon. If you’ve ever programmed with me, you’re probably not surprised. Look at how irrelevant data is hidden away! Look at how there’s no copied boilerplate about how to get the owner name and repo name out of the route props!

This was a bad idea.

By going HAM on separation of concerns, I had introduced an implicit dependency into this system. While it’s true that the caller of loadIssues doesn’t have to know or care about where the ownerName and repoName values come from, it is now a hidden dependency of the IssueListPage that they must come from somewhere. If it’s not nested under App or another component that issues the current-repo-setting action, it will fail.

Instead of loadIssues having an obvious set of parameters that, while possibly tedious to gather, was at least clearly state, I, in the service of cutesy elegance, made it so it only worked if another function was called first, with no connection other than documentation. Fans of the blog can see how this mirrors the abstraction vs. convention discussion from the last post, with clear method parameters being a kind of “checked convention.”

While I am still not 100% satisfied by the way that the params prop seems to tie the data access so directly to the router, I think that can be softened by using Redux’s “container” pattern to confine both params access and Redux state to a specific layer in the component hierarchy.

I also did away with any magic around calling dispatchForRouteParams on the client side and left it up to components to call it as needed from componentDidMount and componentWillReceiveProps, which lets container components establish their own cache invalidation strategies.

Since it’s still poor practice to trigger a fetch on the initial client load, I made a Redux middleware that keeps an eye out for that and logs an error if it happens, in the spirit of checked conventions.