salsita / prism

React / Redux action composition made simple http://salsita.github.io/prism/
496 stars 24 forks source link

Nested routes with react-router #38

Closed jmarceli closed 8 years ago

jmarceli commented 8 years ago

Hi, During my app development I found that dot restriction in (forwardTo)[https://github.com/salsita/redux-elm/blob/master/src/forwardTo.js#L13-L15] causes some trouble if you want to define nested routes which are also nested in Elm Architecture. Are there any reasons to restrict dot in Action names besides possible naming bugs?

I've made custom deepForwardTo() without that restriction and it seems to be working fine.

Here is sample code from my app:

const deepForwardTo = (dispatch, ...types) => {
  if (types.length === 0) {
    return dispatch;
  } else {
    return action => dispatch(wrapAction(action, ...types));
  }
};

const connectView = (View, modelKey, ...nesting) =>
  // lodash _.get() used for nested model matching
  connect(appState => ({ model: _.get(appState.root, modelKey) }))(
    props => <View {...props} dispatch={deepForwardTo(props.dispatch, ...nesting)} />);

const MainLayout = connectView(Main, 'mainLayout', 'MainLayout');
const OrdersPage = connectView(Orders, 'orders', 'OrdersPage');
const OrderPage = connectView(Order, 'orders.order', 'OrdersPage.Order');

const routerRoutes = [{
  component: MainLayout,
  onEnter: checkLogin,
  childRoutes: [
    {
      path: '/orders'
      indexRoute: { component: OrdersPage },
      childRoutes: [
        { path: '/orders/:id', component: OrderPage },
      ]
    },
  ]
}];

// Exports routes defined in routing file
export default view(({ history }) => (
  <Router history={history} routes={routerRoutes} />
));

It is nice because this way I can define all the routes (even nested ones) in one routing file.

Maybe it won't be bad if we add kind of deepForwardTo to the redux-elm base? This way beginners will get nice error reporting when they use forwardTo and more experienced will have an option to use dots.

tomkis commented 8 years ago

Hey,

I am wondering, why don't you just:

const OrderPage = connectView(Order, 'orders.order', 'OrdersPage', 'Order');

I probably miss something fundamental here.

Anyway:

Are there any reasons to restrict dot in Action names besides possible naming bugs?

No, it's just about the naming restriction. I assume it should not throw an exception but warn instead, that it's generally anti-pattern to use . in action types.

On the other hand your use case is perfectly valid and should be treated as nested actions.

jmarceli commented 8 years ago

Hi, Thanks for your reply. Nested action name is required if you want to handle subcomponent action from the wrapper component. In my case it is updating orders[data] list after order[data] is saved. See picture below: image So after saving order action 'OrdersPage.Order.Save' is dispatched and then it is handled by orders component thanks to endsWithMatcher. Without action name prefixing I won't be able to handle order saving from orders list updater.

I think that in more complex applications such situation might be more common. So if defining all routes in one place is recommended way to go nested action names should be available out of the box.

I might be wrong in some point but that's just an issue which I encountered during development.

tomkis commented 8 years ago

I totally understand your use case, but please can you elaborate why you can't do

const OrderPage = connectView(Order, 'orders.order', 'OrdersPage', 'Order');

instead of

const OrderPage = connectView(Order, 'orders.order', 'OrdersPage.Order');

Since those two should be equal.

jmarceli commented 8 years ago

You are totally right. I don't know why I didn't see this in your previous example. That's exactly what I was looking for. Thanks for enlightening me :)