Consider not polluting component props with router properties

[magicmaaaaan]

Version: 4.0.0-beta.3

Currently, Route and withRouter merges all router properties with component props. This includes 11 fairly generically named properties: action, block, createHref, go, goBack, goForward, length, listen, location, push, replace. It seems unnecessary to pollute props like this.

I propose router properties should remain under a single router context rather than spreading all the properties into component props.

[mjackson]

I'm not opposed to this idea, but what's the real benefit to keeping everything under a router prop? Seems like it just means you have to write more code.

const MyComponent = ({ location, match }) => {
  // ...
}

vs.

const MyComponent = ({ router: { location, match } }) => (
  // ...
)

Are you worried about name clashes with other props you might want to pass to your component?

[TomiS]

For example, prop location already collides in my app. So it's really possible it happens. Location is also quite easy to collide with because it's also a typical domain concept related to physical world, geolocation etc.. So I'd guess it's used in many other apps than ours too.

Furthermore, I'd say it's just good manners to not pollute props. What if all 25-100 libs in every app would do the same? They would probably collide with each other even without adding any application specific props :)

ps. Generally the version 4 is shaping up really nicely. I'm almost through implementing it in our app.

[timdorr]

Are you using connect on your route components? That (or another HoC) is the only way a prop would collide for the component prop on a Route. For the function child form, you have full control over the props you pass.

But regardless, that represents a bad pattern of overloading your components with too much responsibility. I see a lot of people using Redux connected components as their route components. This puts a lot of code in one place and makes for really brittle systems. Components are cheap, so make separate ones for routing, state connection, etc. You not only avoid the problems this issue brings up, but you also reduce complexity and chances of creating bugs.

[TomiS]

@timdorr That's a solid point. And to be perfectly honest, I haven't yet had any actual collision issues. I just see the danger since we have Location as domain object in our app. So conceptually they collide, but not technically.

I also cheer for not changing the api all the time, so I guess having many props here might not be too bad after all. Hard to say.

[mjackson]

I'm interested to hear @ryanflorence' opinion on this. Honestly, I'm indifferent. I think @timdorr has a great point. Feels like this complaint is more about how people currently think about structuring their apps instead of an actual issue with our code.

[pshrmn]

I can provide an instance where this was an issue, but it isn't something most people would run into.

If a component that has a namespace collision with one of context.router's properties is wrapped in withRouter, then the component's property will be lost because of the order in which the props are spread in withRouter. Unfortunately, reversing the order is not a great fix because then context.router properties with collisions could then be lost.

const WrappedLink = withRouter(Link)

// this.props.replace will be a function instead of the expected boolean
<WrappedLink replace to='/some/page'>Some Page</WrappedLink>

I ran into this issue when I was writing a whenActive HOC factory (think connect but to make any component <NavLink>-like, source).

There were also other issues with this, such as needing to remove any context.router properties from props before spreading them to the wrapped component in case the wrapped component was a built in (<div>, etc.).

I am sure @ryanflorence would be glad to know that my solution ended up being to render a <Route>, but I would have preferred to use withRouter.

[xzilja]

Supporting this, in usecases where you just need to pass router props down, you will also pass unnecessary ones that come from withProps, having them all under router object would be welcome 👍

---

Right now I need to pass these one by one in order to only include router related props

[billyjanitsch]

@timdorr's point is certainly true for route components, but I think the original point is well-taken for any component wrapped in withRouter. Here's an example of the kind of issue this could cause:

const Vacation = props =>
  <div>
    Take a sunny vacation to {props.location}!
  </div>

render(<Vacation location='California' />)

Later, I want to add some routing functionality:

let Vacation = props =>
  <div>
    Take a sunny vacation to {props.location}!
    <button onClick={() => props.push('/flights')}>book flight now</button>
  </div>

Vacation = withRouter(Vacation)

render(<Vacation location='California' />)
// oops, location gets rewritten by withRouter :(

As @magicmaaaaan pointed out, this is fairly likely to happen with 11 generically named props. Passing just router leaves less overall surface area, with a less generic name.

[atoiv]

Components passing a load of props to the components that they render, without knowing what props those components expect, is generally a bad practice imho. When there's need to reserve/hijack props of the rendered component, it's better to be 1) explicit about that, and 2) try to minimize the amount of reserved props.

It's essentially about negotiating a deal between parent and a child component. In general case, when a parent component is rendering a specific child component, it's the parent component that is responsible for passing the right props to that child component, because it can adjust to the props required by the child component. But in situations like this case, when roles are reversed, and a library provides the parent component, and a developer is left to choose the child to pass to it, it is just courteous for the parent component to try to minimize the reserved footprint on the child components props.

In the latter case, this kind of forced props should also be part of the api documentation.

That being said, separating routed components from HOCed components, like @timdorr said, sounds like a good practice.

[ryanflorence]

I'm willing to discuss the tradeoffs here. Here's why we did it this way:

1. It's easy to avoid collisions with container components:

// the component with a naming collision
const Facility = ({ location }) => <div>{location.city}</div>

// instead of
<Route component={Facility}/>
// do:
<Route render={(router) => (
  <Facility location={someLocation} routerLocation={router.location} />
)}/>

// instead of
const FacilityWithRouter = withRouter(Facility)
// do:
const FacilityWithRouter = withRouter((router) => (
  <Facility location={someLocation} routerLocation={router.location} />
))

2. It's pretty simple to get the API we're talking about with composition

For collisions with Route, you can unwrap it a bit:

<Route render={(router) => <YourThing router={router}/>}/>

For collisions in withRouter, that's just a higher-order component problem, They "compose" but they have the exact same problem as context regarding naming collisions--which is one of the reasons I find component composition less problematic.

However, you can make a namespaced HOC to get the api you want pretty simply using the same composition:

const withNamespacedRouter = (Comp) => (props) => (
  <Route render={(router) => (
    <Comp router={router} {...props}/>
  )}/>
)

3. We did it for simpler identity checks in the lifecycle

In many cases, especially for generic data loading, animations, and "pure render", it's useful to check the identity of match or location to know if the location changed.

componentDidUpdate(prevProps) {
  if (prevProps.match !== this.props.match) {
     this.doTheStuff()
  }
}

Because the router object we're talking about is currently mutated our implementation is much simpler. If we passed in a router namespace to route components, then we'd need to construct a new object every render to avoid identity checks on router.match or router.location always returning true.

Additionally, we'd have to be mindful of PureRender implementations by not sending a new router object every render unless the location actually changed. And that's weird stuff to document and talk about:

The router object will have a new identity when [x], [y], or [z] change, otherwise it is the same object so that shouldComponentUpdate will hopefully work as you'd expect it too...

Bleh. Know what I mean?

Then as the developer, if I haven't read that piece of the docs, I'm running a comparison on props.router.match !== this.props.router.match and wondering if I'm just taking advantage of an implementation detail or if this is part of the contract, where I think prevProps.match !== this.props.match feels more trustworthy to me.

I don't want to have to document how the identity of a nested object and its properties work, it's confusing stuff. However, our non-namespaced props work the way most people expect (I think, anyway).

4. We also did it for convenience

This is ugly no matter how you slice it:

const Thing = ({ router: { match : { params : { userId } } } }) => (
  <div>{userId}</div>
)
const Thing = (props) => (
  <div>{props.router.match.params.userId}</div>
)

It's convenient to have one less level of nesting.

Summing up

I'm not totally opposed to this, I just don't think the collision problem is a big deal since it's easily composed around. And, correct me if I'm wrong, that's the only problem. So I guess I'd need more convincing that these props names would really lead to confusing situations.

I think of these route components (whether <Router component/> or withRouter) as exactly that, "a route component" that has a bag of props about routing. I like documenting and using components more than objects, cause you get all the normal expectations (like simpler propTypes, lifecycles and prev/next prop comparison, etc.)

[ryanflorence]

Sorry about that ^ clicked "close and comment" because I intuitively thought it was "cancel" 😆

[mjackson]

Great explanation @ryanflorence, thanks. I'm pretty sure the potential naming collision was the only real issue, and, as I already said, that's not really an issue with our code but just more about how people think about using our code. I, for one, like the separate props. It's just less typing and generally easier to deal with.

Thanks for the discussion everyone. I think we'll leave it as-is for now. :)

[ryanflorence]

FYI, we didn't circle back to this issue. For those following, the final API is going to be { match, location, history } instead of { ...history, match }.

It gets passed to withRouter, <Route render>, <Route children>, <Route component>