React-router: Feature request: 'to' property as a function in Link component

Created on 12 Jul 2017 · 18Comments · Source: ReactTraining/react-router

In RR v3 you could pass a function to 'to' property in Link component. Would you be interested in porting this functionality to RR v4? If yes I would be happy to submit a PR.

Source

smashercosmo

👍3

All 18 comments

What's the use case for this? It sounds like you can achieve the same thing with component composition.

timdorr on 12 Jul 2017

Yep, it's just for convenience and for reducing the amount of API inconsistencies between v3 and v4

smashercosmo on 12 Jul 2017

Well, go ahead and work up a PR. We can discuss the implementation specifics over there.

timdorr on 13 Jul 2017

FWIW I don't see a lot of value in this feature. Seems like you could just as easily call the function yourself instead of asking us to call it.

<Link to={someFunc}/>
<Link to={someFunc(stuff)}/>

In the first implementation we have to decide what arguments we're going to pass to the function you provide. In the second, you just go ahead and call it.

mjackson on 14 Jul 2017

😕2

mmm... but the point is that this function is called with "location" as argument. In RR3 it was very convenient, that you didn't have wrap parent component in withRouter to get the location object.

<Link to={location => {...location, query: {search: 'hello'}}}>

Of course in RR4 you can achieve that by wrapping Link into Route component, but I just thought it would be nice to keep the API consistent between RR3 and RR4.

smashercosmo on 14 Jul 2017

👍1

Ah, I see. That's a great use case, thanks for sharing :). I'd definitely be open to a PR that does this.

mjackson on 14 Jul 2017

I started working on the PR. @timdorr could you give me some guidance? Currently I have a problem with tests. When I run them I get a lot of errors like "Cannot find module 'react-router' from 'BrowserRouter.js'". What could be the problem?

smashercosmo on 16 Jul 2017

You most likely need to run the build script first.

timdorr on 16 Jul 2017

@timdorr thx, it worked. One more question. I noticed, that you're not currently testing clicks on a Link. Should I write these tests using react test utils? And if yes, should I write them only for to-as-a-function case or for all the cases?

smashercosmo on 16 Jul 2017

@timdorr ping

smashercosmo on 17 Jul 2017

@smashercosmo There used to be some tests, but I think that they got lost in the switch to the monorepo. I have a click test in the relative link PR that you can reference: https://github.com/pshrmn/react-router/blob/cd42cf65a2a7394bd6b234238f370df8d274ca80/packages/react-router-dom/modules/__tests__/Link-test.js#L127-L154

I would probably start with just the test for to-as-a-function. The other tests should probably be brought back, but maybe in another PR?

pshrmn on 17 Jul 2017

@pshrmn ok, thx, got it

smashercosmo on 17 Jul 2017

@timdorr @pshrmn One more question, I'm not quite sure what to do with NavLink, because in NavLink "to" property value gets passed to ''path" property of Route component.

smashercosmo on 23 Jul 2017

@smashercosmo The way that I would approach <NavLink> is to call the to function right away to generate the location object to navigate to. You will also need to add context to the <NavLink> in order to access the current location.

const NavLink = ({ to, ... }, context) => {
  const goTo = typeof to === 'function'
    ? to(props.location || context.router.route.location)
    : to
  return (
    <Route
      ...
      children={(...) => {
        <Link to={goTo} ... />
      }
    />
  )
}

NavLink.contextTypes = {
  router: PropTypes.shape({
    route: PropTypes.shape({
      location: PropTypes.object.isRequired
    }).isRequired
  }).isRequired
}

pshrmn on 23 Jul 2017

Also, feel free to open a PR. You can always rebase later, so it doesn't really hurt to open the PR early.

pshrmn on 23 Jul 2017

@pshrmn Alright, thank you very much

smashercosmo on 23 Jul 2017

Done https://github.com/ReactTraining/react-router/pull/5368

smashercosmo on 23 Jul 2017

It looks like this issue fell through the cracks and didn't get merged. This is causing porting problems for us (LessWrong); we're upgrading react-router-dom because our framework (Vulcan) dragged us along, and there are some existing usages of the function-style links in our code. It's too late for these PRs to help us, but, could one of them get merged reasonably soonish?