Higher order function leads to circular dependencies

[ashtonsix]

This is causing problems throughout my app and I can't think of an easy fix, routes depends on store and because of the recent API changes the store now depends on routes. DISASTER!!!!, to show by example here is my store:

import {createStore, compose} from 'redux';
import {reduxReactRouter} from 'redux-react-router';
import {devTools, persistState} from 'redux-devtools';
import createHistory from 'history/lib/createBrowserHistory';
import mgCookies from 'lib/mgCookies';

import createReducers from 'app/store/reducers';
import middleware from 'app/store/middleware';
import routes from 'app/router/routes';

const initialState = ((
  mgCookies.getItem('sso') &&
  JSON.parse(sessionStorage.getItem('applicationState'))
) || undefined);

export default (
  compose.apply(null, [
    middleware(),
    reduxReactRouter({
      routes,
      createHistory,
    }),
    __DEVELOPMENT__ && devTools(),
    __DEVELOPMENT__ && persistState(
      window.location.href.match(/[?&]debug_session=([^&]+)\b/)
    ),
  ].filter(v => v))(createStore)(
    createReducers(),
    initialState
  )
);

Fairly standard. I'm also dynamically generating action constants:

import {createActionConstants} from 'lib/mgCollection';

...

const actions = [
  'AUTH_LOGIN',
  'AUTH_LOGIN_SUCCESS',
  'AUTH_LOGIN_FAIL',
  'AUTH_LOGOUT',

  ...

  ...createActionConstants('campaign'),
  ...createActionConstants('campaignEmail'),
  ...createActionConstants('survey'),
  ...createActionConstants('surveyQuestion'),
  ...createActionConstants('surveyTemplate'),

  ...

  'SIDEBAR_SET_PROPERTIES',
];

const o = {
  'ROUTER_LOCATION_CHANGED': '@@reduxReactRouter/locationDidChange',
  'APPLICATION_INIT': '@@redux/INIT',
};

actions.forEach((action) => {
  Object.defineProperty(o, action, {
    value: action,
    writable: false,
  });
});

export default o;

mgCollection also contains action generators that connect to the API and reducer generators which depend on the store, thus createActionConstants loads async. I can use it within a setTimeout for instance but not the module body. For another example accessing store.getState().auth on a Route's onEnter hook requires async stuff going on. There are probably better ways to implement all of this - but I don't want to be forced into rewriting a mass of code for the sake of using a higher-order-store.

[gaearon]

cc @acdlite

Never mind me, I thought I'm in RR issues..

[acdlite]

Circular dependencies are known issue. The solution is usually a closure that returns the value of the dependency on demand.

Another option is, instead of using routes, you pass a function getRoutes(dispatch, getState).

If you show me your route config I can try to offer further help. This topic definitely needs to be in the docs.

[gaearon]

I feel uneasy about routes being a part of history configuration instead of being a prop on the component.

Is there absolutely no way we can move routes to the component? Of course assuming there is code to handle componentWillReceiveProps. Would also enable hot reloading of routes.

[acdlite]

@gaearon The only way I can think of is by internally wrapping the route configuration in a top-level route so we can use getRoutes() and asynchronously return the route config during component initialization. It would require adding a replaceRoutes() function to the store a la replaceReducer() (since that's essentially what we're doing, in the context of routing). Perhaps that's not so bad, though.

[gaearon]

That's assuming RR API is locked right? If we could change it too would that be doable in a cleaner way? If so, how?

[acdlite]

@gaearon React Router already supports dynamic route config, but not for the top-level route, only for child routes via getChildRoutes(). We could update matchRoutes() to accept an asynchronous function.

But now that I think about it, wrapping a route config in one more route doesn't seem so bad, or an abuse of the existing API. matchRoutes() is recursive, anyway, so we're just adding one extra level.

[gaearon]

But now that I think about it, wrapping a route config in one more route doesn't seem so bad, or an abuse of the existing API. matchRoutes() is recursive, anyway, so we're just adding one extra level.

Sounds sensible.

[ashtonsix]

As you asked here's what routes.js looks like:

import React from 'react';
import {Route, IndexRoute, Redirect} from 'react-router';
import {
  containsSidebar,
  requiresAuth,
  mergeHooks,
} from 'app/router/transitionHooks';

import {
  App, Auth,
  Error404,
  LaunchpadDashboard,
  ActionsContainer, ActionsDashboard,
  SurveysDashboard, SurveysContainer, SurveysEdit,
  CampaignsContainer, CampaignsList, CampaignsEdit,
  EmailsContainer, EmailsDetail, EmailsList, EmailsEdit,
  ReportsContainer, ReportsDashboard, ReportsPopularity, ReportsPoints,
} from 'app/views';

export default (
  <Route path='rewards' component={App}>
    <IndexRoute component={LaunchpadDashboard} {...requiresAuth}/>
    <Route path='auth' component={Auth}/>
    <Route path='actions' component={ActionsContainer} {...mergeHooks([requiresAuth, containsSidebar])}>
      <IndexRoute component={ActionsDashboard}/>
      ...

And here's transitionHooks.js, notice I've used setTimeout's to ensure store is defined when these hooks are invoked:

import _ from 'lodash';
import store from 'app/store';
import {setSidebarProps} from 'actions/navigationActions';
import {logout} from 'actions/authActions';

const containsSidebar = {
  onEnter: () => setTimeout(() => store.dispatch(setSidebarProps({hideOnTransition: false}))),
  onLeave: () => setTimeout(() => store.dispatch(setSidebarProps({hideOnTransition: true}))),
};
const requiresAuth = {
  onEnter: nextState => setTimeout(() => (
    _.get(nextState, 'location.pathname') !== '/rewards/auth' &&
    !store.getState().auth.claims.length
  ) ? store.dispatch(logout()) : null),
};

function mergeHooks(handles) {
  return handles.reduce((pv, handle) => {
    return _.merge(pv, handle, (f1, f2) => (f1 && f2) ? _.flow(f1, f2) : f1 || f2);
  }, {});
}

export {containsSidebar, requiresAuth, mergeHooks};

My other issue was w/ using createActionConstants a function inside mgCollection.js before store was defined. mgCollection.js imported the store for use in another function called createActions. This was resolved simply by splitting up the file.

My issue isn't w/ overcoming these issues, it's that I don't know what other issues exist because of the circular dependency that haven't been observed, i.e. edge cases. I'm also concerned that this circular dependency has caused and could cause further global consequences in the codebase design.

Sounds like you've got a solution.

[acdlite]

Closed by #47. Now you can specify routes as children (or routes prop) of <ReduxRouter>.