Skip to content

Send params to all active routes #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ryanflorence opened this issue Aug 1, 2014 · 28 comments
Closed

Send params to all active routes #158

ryanflorence opened this issue Aug 1, 2014 · 28 comments

Comments

@ryanflorence
Copy link
Member

Is there any reason not to just have the current params available on this.props.params in every active handler?

Sometimes you want a parent route to nest some UI in that depends on the params to get the data, but is not actually a valid path itself.

For instance, we want to support user/123/details and user/123/activity, and have them share a UI wrapper, but we do not want the url user/123 to match anything (we'd probably redirect somewhere).

<Route handler={User}>
  <Route path="user/:userId/details" handler={Details}/>
  <Route path="user/:userId/activity" handler={Activity}/>
</Route>
var User = React.createClass({
  mixins: [AsyncState],

  getInitialAsyncState: function(params) {    
    // `params.userId` is not sent here
    return { user: this.findUser(params.userId) }
  },

  componentDidMount: function() {
    // `this.props.params.userId` isn't here either
  },

  render: function() {
    return (
      <div>
        <h1>{this.state.user.name}</h1>
        <this.props.activeRouteHandler/>
      </div>
    );
  }
});

To make this work we have to put a path on the user route, and then redirect in componentWillMount if there is no activeRouteHandler.
#149

@mjackson
Copy link
Member

mjackson commented Aug 1, 2014

I'm not sure I agree.

For your use case, you should be doing this:

<Route handler={User}>
  <Route path="user/:userId/details" handler={Details}/>
  <Route path="user/:userId/activity" handler={Activity}/>

  <!-- This route has access to params.userId, and can act accordingly -->
  <Route path="user/:userId" handler={RedirectSomewhere}/>
</Route>

I guess I'd just like to see a better use case. This one seems to be solved nicely by #159.

@bobeagan
Copy link
Contributor

bobeagan commented Aug 1, 2014

We also have an example where we have an account section wrapper that adds a sidebar of account links an a page title of the account page you are viewing. Rather than having to render the title on every account page, we want to show it in the wrapper. But in order for that to happen, we need to access a title from the active route component (think it makes more sense to define in there rather than in the wrapper based on route name) in the wrapper route.

For example: /account/plans might have a title defined of 'Plans and Billing'

@mjackson
Copy link
Member

mjackson commented Aug 1, 2014

I should also point out that I really don't think User should care about the userId at all in this example because it doesn't declare a path. The way you've written it, it's purely just a UI wrapper.

If it did care about userId, it should declare path="user/:userId". But it still doesn't make sense to have User doing a "redirect" in that case because you'd have to do it in componentWillMount. At that point the transition has happened already and you're just doing a manual Router.transitionTo.

@mjackson
Copy link
Member

mjackson commented Aug 1, 2014

I guess the part that doesn't really make sense to me is parent routes that care about segments of the URL they're not even sure exist. I'm not totally opposed to having one set of params per transition (instead of per route). It certainly would simplify the implementation. Just feels a little messy, that's all.

This conversation reminds me of the "Thinking in React" post, particularly the part about identifying where state should live.

@bobeagan
Copy link
Contributor

bobeagan commented Aug 1, 2014

I agree that my UI wrappers don't need paths defined for them. Instead, I'd expect to add another route child inside it for special cases that did redirects (like @mjackson showed). But there are definitely times where it would be nice to have a way to easily access data in the child/active route from the UI wrapper route. Otherwise, I just end up adding a component or mixin to all of the account pages to handle it and make the UI wrapper component do less.

@ryanflorence
Copy link
Member Author

@mjackson three things

  1. The parent route handler needs to be able to fetch the user data to display it in the wrapper UI
  2. The parent route handler is not a valid route, so you don't want to configure a path.
  3. If you add a path, then you have to do the redirect in componentWillMount instead of willTransitionTo because you have to inspect your activeRouteHandler, which feels like a strange suggestion

@ryanflorence
Copy link
Member Author

I wonder if <Redirect/> would help:

<Route name="user" path="user/:userId" handler={User}>
  <Route name="details" path="user/:userId/details" handler={Details}/>
  <Route name="activity" path="user/:userId/activity" handler={Activity}/>
  <Redirect path="user/:userId" to="details"/>
</Route>

Still feels weird configuring a path for user when you don't ever intend for anybody to go there and then having to add a <Redirect/> to prevent it from happening.

@mjackson
Copy link
Member

mjackson commented Aug 2, 2014

@rpflorence I agree with you; User shouldn't declare a path because you'd still have to do the redirect in componentWillMount, which isn't really a redirect at all. It's just a replaceWith.

For the use case being described here, Redirect should be sufficient.

@bobeagan Your use case sounds interesting, but I'm having a hard time following it from your description. Care to share the relevant portion of your route config?

@ryanflorence
Copy link
Member Author

@mjackson you're forgetting that the User component needs the params to find the user so it can display some UI like this.state.user.name, regardless of its child route.

var User = React.createClass({
  render: function() {
    return (
      <div>
        <h2>{this.state.user.name}</h2>
        <this.props.activeRouteHandler/>
      </div>
    );
  }
});

@bobeagan
Copy link
Contributor

bobeagan commented Aug 2, 2014

I'm also curious about how to handle my example #158 (comment) where the content that the UI wrapper needs to display is not directly linked to the params but rather the component. Maybe there is an existing, preferred approach to this which I'm just unfamiliar with?

@mjackson
Copy link
Member

mjackson commented Aug 2, 2014

@rpflorence Why don't you just have the activity/details components use a shared sidebar component?

var Activity = React.createClass({
  render: function () {
    return (
      <div>
        <Sidebar user={this.state.user}/>
        <p>The activity page.</p>
      </div>
    );
  }
});

User-specific UI doesn't need to live in the User component, does it?

@ryanflorence
Copy link
Member Author

@mjackson why have the router do any UI nesting at all then? Just match urls to handlers w/o nesting and then share all UI with components like that.

shared wrapper UI is the whole point of this router

The params are the params, I can see no reason you'd ever not want the parent route to have them. The only reason is "well, it doesn't have a path that asks for it" which a really dumb reason.

@ryanflorence
Copy link
Member Author

Our original implementation did this, and I didn't do it just because the implementation was easier, I did it because I wanted my parent UI, that never matches a path, to be able to read the params. I was actually surprised last week to find out it didn't work this way anymore.

@JedWatson
Copy link
Contributor

@rpflorence nailed it. We're getting two things that should be separate (path matching w/ params, and UI nesting) and mixing them.

Stepping through the guidelines in "Thinking in React":

For each piece of state in your application:

  • Identify every component that renders something based on that state.

I've got my User Route, and its child Routes. They need to know :userId to render UI.

  • Find a common owner component (a single component above all the components that need the state in the hierarchy)

Done; User owns it.

  • Either the common owner or another component higher up in the hierarchy should own the state

Can't; User doesn't get it, unless I specify a path for User, but it's not a valid navigation target.

At this point I'm questioning the sanity of having UI nesting and routing bound so closely together, but I'm tired, so it may just be that.

In fact; what's the point of routes that have no path? They're not really routes, just objects that wrap routes. That's more like layout inheritance than routes.

Here's a crazy idea:

<Layout name="user" path="user/:userId" handler={User}>
  <Route name="details" path="user/:userId" handler={Details}/>
  <Route name="activity" path="user/:userId/activity" handler={Activity}/>
</Layout>

If a Layout contained a Route with a path that didn't match, it would throw an error. This would provide a more semantic structure, because I can say this Layout requires these params, without actually expecting it to be a matchable Route.

Rolling back on my crazy idea, this would also get closer to a workable solution:

<Route name="user" path="user/:userId" handler={User}>
  <Route name="details" path="user/:userId" handler={Details}/>
  <Route name="activity" path="user/:userId/activity" handler={Activity}/>
</Route>

... in this case, User detects there's a child route with the same path, and automatically sets this.props.activeRouteHandler to {Details} when its path matches.

@ryanflorence
Copy link
Member Author

Ember doesn't have this ambiguity because every route has a path and is a target. It also introduces "index routes" that are child routes that render when a parent has no active children. Though its confusing originally to most people, it makes sense eventually. This also only really works well when paths are inherited from parent to child.

I still maintain we should just pass params to every active handler, but there is definitely a conversation here about routes w/o paths and why they are called "routes" anyway. A thought I'm not sure I'm loving yet, but seems to solve this problem, is to require all routes to have a path, and introduce "Default" routes. They are just like routes except they render when no child matches the parent's path.

<Routes>
  <Route name="user" path="user/:userId" handler={User}>
    <DefaultRoute handler={Details}/>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
  </Route>
</Routes>

// or if you really want /details in the url
<Routes>
  <Route name="user" path="user/:userId" handler={User}>
    <DefaultRoute handler={RedirectToDetails}/>
    <Route name="details" path="user/:userId/details" handler={Details}/>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
  </Route>
</Routes>

// and maybe if we keep going down the `<Redirect/>` path
<DefaultRedirect to="details" />

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

Thanks for the explanations @rpflorence and @JedWatson. Let's go ahead and make the change to pass the active params to all handlers.

@rpflorence Let's continue the convo about routes w/o paths in another issue.

@JedWatson
Copy link
Contributor

@rpflorence that looks like a more explicit version of my second idea, where we detect the DefaultRoute based on a common path (honestly, this is still my favourite API, it's pretty clear that if the path matches, all matching child Routes are activated).

I generally don't want the /details part in the URL, that was a workaround, your suggestion's more explicit in its definition and allows for this; I'd say if you want that, use a <Redirect>.

If we're being explicit, I'm not sure it's worth another type; wouldn't it be cleaner to specify a defaultChild in the parent Route than use a <DefaultRoute />, or have a defaultRoute=true attribute?

My $0.02 is the first is cleaner; it saves validating that there should only be one default child per Route, the API is clearer because it doesn't allow for misconfiguration.

Also, maybe important to note that the solution should behave recursively, so it can scale to multiple levels of nested default routes, for more complicated use-cases than our example.

@ryanflorence
Copy link
Member Author

@JedWatson were I do build this UI it would look something like this:

<Routes>
  <Route name="user" path="user/:userId" handler={User}>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
  </Route>
</Routes>
var User = React.createClass({
  // code to fetch the user, probably with `AsyncState`
  render: function() {
    return (
      <div>
        <h1>{this.state.user.name}</h1>
        {this.props.activeRouteHandler() || Details({user: this.state.user})}
      </div>
    );
  }
});

The constraint that made things more complicated was having the /details url, which introduces redirecting, which introduced the indirection.

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

I started digging into the implementation of this feature this morning and realized there's a problem. It has to do with transition hooks.

The way the router works currently, we know exactly which params each route is interested in. They are declared in the path prop. So if my config looks like this:

<Route handler={User}>
  <Route path="users/:userID/activity" handler={Activity}/>
  <Route path="users/:userID/details" handler={Details}/>
</Route>

I know that Activity and Details are interested in the userID param, but User doesn't care.

Now, let's say the user clicks on a link from users/123/details to users/456/activity. That means we need to make a transition. If we know that User isn't concerned about the userID, we can trigger willTransitionFrom on Details and willTransitionTo on Activity and we're done. However, if we have no idea what params User is concerned with we need to trigger willTransitionFrom/To on it as well. This behavior extends all the way up the route hierarchy to the root route.

On the other hand, if we know what params User needs to do its job and we get a transition from users/123/details to users/123/activity then we know we don't need to send transition hooks to User.

tl;dr If we pass all params to every route handler then every transition triggers willTransitionFrom/To all the way up the route hierarchy.

@rpflorence Turns out "well, it doesn't have a path that asks for it" isn't "a really dumb reason" after all. :P

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

Anyway, my recommendation would be that if you need a param, declare it in your path. It's that simple, and it fits very well with the explicit nature of React.

This config solves the transition hooks issue:

<Route path="users/:userID" handler={User}>
  <Route path="users/:userID/activity" handler={Activity}/>
  <Route path="users/:userID/details" handler={Details}/>
</Route>

It also eliminates the need for DefaultRoute entirely by letting you do return this.props.activeRouteHandler() || this.defaultRouteHandler() in User's render method.

Edit: Or use <Redirect> as @JedWatson suggested if you really want /details in the URL.

@ryanflorence
Copy link
Member Author

@mjackson and I had a quick chat in IRC. Only sending params to routes that care is important for transitions so I'm closing this issue.

Though you can solve the problem already as we've shown in the last few commnets, we'd like it to be more obvious how to meet this use-case. So, we'll be adding <DefaultRoute/>.

It will look something like this, and remove the need to check if there's a valid activeRouteHandler.

<!-- render details at `/users/123` -->
<Routes>
  <Route name="user" path="user/:userId" handler={User}>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
    <DefaultRoute handler={Details}/>
  </Route>
</Routes>

<!-- redirect to `/users/123/details` from `/users/123` -->
<Routes>
  <Route name="user" path="user/:userId" handler={User}>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
    <Route name="details" path="user/:userId/details" handler={Details}/>
    <DefaultRoute handler={RedirectToDetails}/>
  </Route>
</Routes>

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

Just thinking about this a little more.

Here's what these APIs would look like without DefaultRoute:

<!-- render details at `/users/123` -->
<Routes>
  <!-- use this.props.activeRouteHandler() || Details() inside User#render -->
  <Route name="user" path="user/:userId" handler={User}>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
  </Route>
</Routes>

<!-- redirect to `/users/123/details` from `/users/123` -->
<Routes>
  <Route name="user" path="user/:userId" handler={User}>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
    <Route name="details" path="user/:userId/details" handler={Details}/>
    <!-- Use <Redirect> and avoid need to create RedirectToDetails -->
    <Redirect path="user/:userId" to="details"/>
  </Route>
</Routes>

Of these two, the first is definitely the stronger use case IMO. We could solve it using a <DefaultRoute> config that magically inherits its parent's path, but we could also use something like a <Route defaultRouteHandler> prop.

<!-- render details at `/users/123` -->
<Routes>
  <Route name="user" path="user/:userId" handler={User} defaultRouteHandler={Details}>
    <Route name="activity" path="user/:userId/activity" handler={Activity}/>
  </Route>
</Routes>

In this case, we would automatically set this.props.activeRouteHandler = this.props.defaultRouteHandler when the path is /user/123. This would let you omit the || logic in User#render so you could just

render: function () {
  return this.props.activeRouteHandler();
}

regardless of whether the path is /users/123 or /users/123/activity.

@ryanflorence
Copy link
Member Author

hmm ... what if I want to send "static" props to the defaultRouteHandler?

(I like that the path only lives in one place and there's no explaining to do about inheriting paths of parents)

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

Then ignore defaultRouteHandler and use this.props.activeRouteHandler(staticProps) || Details(staticProps). But that's the edge case I'd say.

@ryanflorence
Copy link
Member Author

I LIKE THIS

@ryanflorence
Copy link
Member Author

except ... that it is rendered as a sibling to the other routes but doesn't look like them ...

@ryanflorence
Copy link
Member Author

(I still like it) however ... as we keep talking about making Route more flexible via a mixin, DefaultRoute could also be a mixin, allowing people to extend it for their application's conventions. Because of this I'm still leaning toward DefaultRoute.

@mjackson
Copy link
Member

mjackson commented Aug 4, 2014

🎆 #164

oh ... wait.

Edit: Let's keep this convo going in #164.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants