Skip to content
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

Bring back <Route name> #1840

Closed
mjackson opened this issue Sep 7, 2015 · 61 comments
Closed

Bring back <Route name> #1840

mjackson opened this issue Sep 7, 2015 · 61 comments

Comments

@mjackson
Copy link
Member

mjackson commented Sep 7, 2015

There has been a lot of discussion about this already in #1514 and #1791.

We removed <Route name> in 1.0 for a few important reasons:

  • Dynamically loading route config (i.e. getChildRoutes) means that we may not actually be able to build real URLs for <Link>s whose route config has not yet loaded
  • We believe that using real URL paths in <Link to> doesn't take any extra time–you have to look up the route path or the name, which are usually pretty reflective of each other
  • You don't have to know the parameter names to create links now
  • We want to encourage users to NOT change their URLs–use a <Redirect> instead

Given the above reasons, it hardly seems worth it to bring back <Route name> as an API that we prescribe for everyone. However, some users believe that <Route name> can still be useful to specify a name for a route that can be used during development.

If we did bring it back, it probably work something like this:

// We can build up this map dynamically from your route config
const RouteNames = {
  home: '/path/to/home'
}

class NamedLink extends React.Component {
  render() {
    return (
      <Link to={RouteNames[this.props.name]} />
    )
  }
}

Please, please refrain from "+1" comments here and keep the discussion to the possible benefits/drawbacks to adding this feature back in. Thanks :)

@rickharrison
Copy link

One major benefit of bringing this feature back will be less upgrade pain. Throughout the multiple apps I have in production, there are hundreds of Link references. However, I agree with your thoughts on providing the correct primitives and letting the community create higher level APIs around them. I know that I was planning on writing a named link component like you have above when it was time to upgrade my apps.

@fabiosussetto
Copy link

Just my 2 cents having been following the discussion: I agree with the reasoning behind this decision (e.g. offer a set of primitive APIs so that if you need a more advanced solution, you can implement it yourself). It would be very nice indeed though if the equivalent of a component would be implemented by the core developers and maintained as an add-on for the router.

@everdimension
Copy link

I see it's a tough decision to make. Dynamic route loading is a pretty valid and strong reason though.
Introduction of es6 template strings, of course, also helps to justify this decision.

But still I'm... reluctant to visualize a large app with all urls hardcoded. Kind of decreases component reusability, right?

It also decreases the readability of the paths.

Would anyone want to have to look at paths like these? —

render() {
    let trip = this.props.trip;
    let solution = this.props.solution;
    return (
        <Link to={`/booking/${booking.id}/${solution.getId(this.props.trip.id)}?partner=${partner.id}`} />
    );
}

Yes, this can be lightened up if you cache enough object lookups and function calls and maybe restructure your props a little, but still it's not really nice to look at, although the compiled url is going to be relatively simple.

I propose something intermediate.
How about we leave just the rest-path builder functionality? What I mean is something like this:

render() {
    let bookingParams = {
        id: this.props.id,
        solution_id: this.props.solution.getId(this.props.trip.id),
        partner: partner.id
    };

    return (
        <Link to="/booking/:id/:solution_id?partner" params={bookingParams} />
    );
}

This way, all hrefs are easily searchable and readable. And we still encourage users not to change their urls, although refactoring can become much simpler.

@ghost
Copy link

ghost commented Sep 8, 2015

I might be one of the odd ones, but I found that named routes confusing at times due to there being two ways to reference a route. In the codebase, there were some references by name, some by path, and (the worst...) some names that looked like paths but weren't. While this could be remedied by more thorough code review, it could be avoided entirely by having this functionality isolated to an addon component. Having an addon component reduces the api surface of the core library and makes the decision to use named routes an explicit choice, but would still provide a way forward for those that desire/already extensively use named routes.

@knowbody knowbody added the 1.0 label Sep 9, 2015
@jedwards1211
Copy link

Like @danmartinez101 I found it easy to create confusingly ambiguous route names and paths. But I think it would be great to still have builtin support for named routes as long as Links have to explicitly specify that they're linking to a name rather than a (relative) path, e.g. <Link toName="about"> for <Route name="about"/>. I prefer to use relative paths in many cases so that if I were to refactor the parent path for any reason, I wouldn't have to change Links in the children.

@ChristopherBiscardi
Copy link

pre-1.0, we were dispatching data fetching on the server using an object that keyed on the name of the deepest route to match with a data-fetching action.

{
  'home': fetchDataForHome
}

In 1.0, since the suggestion seems to be moving to sticking data-loading on the Routes themselves, (doc) we've moved/are moving away from names and don't have any issues (with name removal) other than replacing the serverside hash for data fetching.

@rpominov
Copy link

I also think @maspwr making a good points here.

Another thing that worries me every time I do /foo/${bar}/ is that bar may contain "1/baz" although supposed to contain an id or something. Sure there are another ways to fix that, for example create a custom tag that escapes all variables e.g. escape/foo/${bar}/. But providing this safety out of the box is a good idea in my opinion.

@gaearon
Copy link
Contributor

gaearon commented Sep 12, 2015

Would anyone want to have to look at paths like these? —

render() {
   let trip = this.props.trip;
   let solution = this.props.solution;
   return (
       <Link to={`/booking/${booking.id}/${solution.getId(this.props.trip.id)}?partner=${partner.id}`} />
   );
}

Yes, this can be lightened up if you cache enough object lookups and function calls and maybe restructure your props a little, but still it's not really nice to look at, although the compiled url is going to be relatively simple.

I propose something intermediate.
How about we leave just the rest-path builder functionality? What I mean is something like this:

render() {
   let bookingParams = {
       id: this.props.id,
       solution_id: this.props.solution.getId(this.props.trip.id),
       partner: partner.id
   };

   return (
       <Link to="/booking/:id/:solution_id?partner" params={bookingParams} />
   );
}

I'm probably missing the point. Why not just write

render() {
    let { id, solution, trip, partner } = this.props;
    let solutionId = solution.getId(trip.id);
    return (
        <Link to={`/booking/${id}/${solutionId}?${partnerId}`} />
    );
}

?

This is shorter and more direct than both of the versions above.

@gaearon
Copy link
Contributor

gaearon commented Sep 12, 2015

Another thing that worries me every time I do /foo/${bar}/ is that bar may contain "1/baz" although supposed to contain an id or something.

This is a great point. (And it also applies to the query: you don't want ryan&michael to be part of your parameter unless you escape it.)

However this point is not really about named routes: it is solvable with

// <a href="/foo/1%2Fbaz?q=ryan%26michael

<Link to={{
  path: ['foo', '1/baz'], // every segment is escaped
  query: {
    search: 'ryan&michael' // every query parameter is escaped too
  }
}} />

Is allowing an object to a bad idea? A named route would then be like

export function userProfile(id, page) {
  return {
    path: ['users', id],
    query: { page }
  };
}

(in userland)

@rpominov
Copy link

+1 on supporting objects {path, query} as to values, good idea!

@rpominov
Copy link

Another use case when named routes can be handy, is when we build a reusable module that supposed to have several pages and links between them, but it doesn't know the base url. A module like that could provide only a subtree of router config, but the end user decides where to put that subtree in the whole router config tree. So the problem is that such module wouldn't be able to build a full path for a link to one of it's pages. But with named routes it could just specify the name.

@idolize
Copy link
Contributor

idolize commented Sep 12, 2015

Yeah, and another advantage of using route names is that it made checking for active routes relatively simple.

For example, if you have a parent route users with a child route specificUser, then you could check if the Users page is loaded, but an individual user isn't selected with something like this:

function isActiveRoute(activeRoutes, routeName) {
  return activeRoutes.some(route => route.name === routeName);
}

if (isActiveRoute(router.getActiveRoutes(), 'users') &&
    !isActiveRoute(router.getActiveRoutes(), 'specificUser') {
  // ...
}

The only way I've found to do this via path is something like:

const { location, params } = this.props;
const { pathname } = location;
if (pathname.indexOf('/fen' === 0) && // this "indexOf" check seems very brittle..
    !params.id) {
  // ...
}

@nkt
Copy link

nkt commented Sep 13, 2015

Please, bring it back. As extension or into the core, but you should to do this. Almost every paragraph in your migration guide starts with "As we removed naming routes, so...". Except a lot of breaking API, it became horrible. Named routes are the most important feature of any router, and every good router implement this feature.
You said:

you no longer need to know the names of the parameters

But instead of this, I have to know how path looks like, /profile or /dashboard, and where parameters should be: /users/${id} or /${id}/users.

@gaearon
Copy link
Contributor

gaearon commented Sep 13, 2015

Please, bring it back. As extension or into the core, but you should to do this.

You can help by taking a few hours to figure out an API for a <Link> with named routes and sharing it with the community as a separate package. If everybody loves it, we can bring it in, or even direct people to your package. Problem solved?

@gaearon
Copy link
Contributor

gaearon commented Sep 13, 2015

For example you can take https://github.com/rcs/route-parser and write a <Link> that works with it.

@gaearon
Copy link
Contributor

gaearon commented Sep 13, 2015

Seriously, route matching is not an easy problem. I can't speak for Michael or Ryan but from what I saw, there was plenty of effort spent on fixing different bugs with it. Add to this the burden of maintaining a custom arbitrary syntax, documentation for it, and receiving “let's add this feature to matcher!” requests.

One library can't do many things well. There are limited people working on it, and limited effort. The focus of React Router is on matching history changes to updating React component tree. This was the reason history was separated: until it was a separate project with its own set of tests and a public API, it was difficult to focus on making it correctly handle all the edge cases.

Route matching is another problem that seems simple, but is in fact difficult to get right, and is not really related to router's main purpose: matching URLs to component trees. While traditionally it has been part of routers, I think it's not a valid argument, because traditionally routers are much simpler and route matching is all they do.

React Router 1.0 API makes it easy for you to build your own thing. Go for it. Named routes are a good use case, but they're not central to what React Router does, and can live as a plugin just fine if somebody actually cares about them enough to take time to implement them instead of being concerned on Github. ;-)

@ryanflorence
Copy link
Member

Everything everybody wants here is easily implemented in user land with your own history middleware and Link component. Let's start seeing some :)

@ryanflorence
Copy link
Member

In my experience with an app with several dozen routes, the names and the paths were identical, except the intermediary dynamic segments were replaced with some delimiter like ., the names could have been auto-generated from the paths, so the only thing link was doing was interpolating the params into the path. If you think that's useful, it would be really simple to wrap link with this:

export default class extends React.Component {
  render () {
    return <Link {...this.props} to={interpolate(this.props.path, this.props.params)}/>
  }
}

@arnihermann
Copy link

I just spent an entire night migrating a fairly large app to 1.0.0-rc1. At first I missed the named routes a lot, especially since we were doing magical things with it in multiple webpack bundles, e.g. could reuse <Link to="profile"> links everywhere in the app and change url in the route definition for the admin bundle which mounted the pages at /admin/profile instead of /profile. It was kind of abusing the router and bound to break someday so I fixed that. The 0.13 -> 1.0 road was quite a lot of changes but I'm pleased with the result so far, my routes are now dynamic and the app feels much snappier.

1.0 is looking out to be a great release! Hats off to @mjackson, @ryanflorence and all contributors, you've done an amazing job!

@jquense
Copy link
Contributor

jquense commented Sep 14, 2015

Well for the sake of the conversation, and for figuring out where the limitations if userland implementations here is one. It implements names and "areas" in a .NET mvc-esque fashion (more closely mirrors our backend). Names can map to more than one route, and you may say that is crazy, but it allows helpful url name patterns at the expense of some ambiguity, and that is a fine trade off for us.

https://gist.github.com/jquense/109f86a6b443345bfd76

There are a few problems, the biggest is that it doesn't work with server rendering, which is fine for us we don't do that.

My big question is: is there an easier/better way to extend the router context?

The other issue is that I feel like I had to reach into lib and utils a lot, which makes me uneasy that I may be relying on private API's to accomplish a lot of this

@jquense
Copy link
Contributor

jquense commented Sep 14, 2015

Side point to the issue of why folks are so adamant about this feature. It think its just that people now have infrastructure built on this feature. Its great that we can implement it, but I think the main reason folks choose a lib in the first place is so they don't need to own that code.

I know that the cost associated with code is very evident to the maintainers here, who have really graciously turned away business to get this router out the door. That is very kind and not taken for granted! The rest of us to a lesser extent are also squaring the some of those concerns with having to backfill something that our chosen library once provided.

I mention this not as a reason or argument but maybe as some context to help us all get along. The React ecosystem doesn't have a lot of robust options for routing aside from RR, so while it is the right choice for RR to refocus its API to provide a better foundation for extensions and platforms. None of those extensions/platforms exist yet, and the choice to build/maintain those new extensions is not as simple as "does RR technically allow for this extension".

all that being said. this is an amazing package, and ya'll are all excellent people for providing it at cost to yourselves, for the sake of the community, so thank you, and the new API is awesome.

@sergeysova
Copy link

+1

@rande
Copy link

rande commented Sep 21, 2015

The benefit of named routes are:

  • easy routing refactoring, no need to lookup into code to update route.
  • easy application decoupling, you can split the code into different sub-modules and rewrite the routing in one file. No need to edit the vendor files to change the routes.
  • Readeable and less error prone, I just see how many errors can be generated by this code: Bring back <Route name> #1840 (comment) . It is easier to manipulate an array than string.
  • Having multiple libraries for handling named routes will not help as third party vendors will try to impose their solution.
  • @idolize comments is valid: Bring back <Route name> #1840 (comment)

@confiks
Copy link

confiks commented Sep 25, 2015

I'm missing the route names a lot too, for all the reasons @rande lists above me. While migrating an app from 0.13 to 1.0.0-rc1, I didn't want to duplicate the paths in a separate lookup, so I've written a quick'n'dirty helper that takes a route definition, and recursively creates a lookup of all name/path combinations, so it can be used in an interpolate helper as mentioned above.

let lookupByName = (route, prefix = route.props.path) => {
  let lookup = {};
  React.Children.forEach(route.props.children, (child) =>
    lookup = {...lookup, ...lookupByName(child, prefix + (prefix.slice(-1) === '/' ? '' : '/') + child.props.path)}
  );

  if(routes.type.displayName === 'Route' && route.props.name)
    lookup[route.props.name] = prefix;

  return lookup;
}

let lookup = lookupByName(<Route path="/" component={...}>...</Route>);

For extra dirtyness points, add the lookup as a property to the history context so it is accessible at any callsite of pushState.

@rpominov
Copy link

rpominov commented Oct 6, 2015

@mjackson useBasename indeed solves this problem better than named routes, or at least as good. Thanks, I didn't know about this feature.

@timdorr
Copy link
Member

timdorr commented Oct 6, 2015

@confiks: Named routes simplify changing your paths.

So do route creator functions, like @gaearon suggested. I think those represent the best pattern here. They are more of a documentation change and less of an API to implement, as they are pure functions that shouldn't be all that complex and require any wrapping.

If you want to get fancy about it, something like route-parser would be a neat, optional addon library to reduce some boilerplate. Or maybe there could be a new project (akin to history) that takes on that specific task. React-router could use that to bring back some of the functionality people are looking for here, but with a more elegant API than simple named routes.

@mjackson
Copy link
Member Author

mjackson commented Oct 6, 2015

@rpominov 👍 :D

@mjackson
Copy link
Member Author

mjackson commented Oct 8, 2015

This thread is tired, and I think we may have a good solution going forward in #2177. Let's follow up there.

@mjackson mjackson closed this as completed Oct 8, 2015
@taion
Copy link
Contributor

taion commented Oct 8, 2015

As an aside, for reasons unclear to me I actually had to add support for naming routes on react-router-relay, because otherwise it was breaking @acdlite somehow.

@StoneCypher
Copy link

Well, this is a pretty huge change blocking our adoption of react 0.14.

Please deprecate in the future.

@taion
Copy link
Contributor

taion commented Apr 6, 2016

They're back. Brand spanking new: https://www.npmjs.com/package/use-named-routes.

@romanlv
Copy link

romanlv commented Apr 6, 2016

library like crossing can be used as well: https://lincolnloop.com/blog/using-crossing-react-router/

@taion
Copy link
Contributor

taion commented Apr 6, 2016

I recommend not bringing in a separate URL resolution library.

The idea behind use-named-routes is to leverage the new location descriptor functionality in History to add drop-in support for named routes. The idea is that it just works without any monkey-patching or hackery involved. You can seamlessly do both:

<Link to="widgets">
<Link to={{ name: 'widget', params: { widgetId: 'foo' } }}>
<Link to="/widgets/foo">

And use the route names and params anywhere where you currently need a path, including e.g. router.push and replace on the onEnter callback, so as above you can do all of:

router.push('widgets');
router.push({ name: 'widget', params: { widgetId: 'foo' } });
router.push('/widgets/foo');

And it all just works.

@th0r
Copy link

th0r commented Apr 7, 2016

They're back.

Incredible work, man!

@mwhite
Copy link

mwhite commented Apr 15, 2016

I was excited to see named routes in core mentioned in the roadmap of @taion's fork blog post, if I remember correctly. Is that still intended?

@everdimension
Copy link

@mwhite what don't you like about the post above? #1840 (comment)

@taion
Copy link
Contributor

taion commented Apr 15, 2016

@mwhite I don't think we plan on adding them to core. I expect that the named route support will live in an extension for the time being.

@mwhite
Copy link

mwhite commented Apr 15, 2016

Allow me to quote from this excellent piece:

Though we allow modules to grow very large, sometimes it becomes apparent that a complex module can be simplified if broken up into ‘submodules’. In other cases, a group of modules may be usefully presented to the rest of the system through the public interface of a single ‘meta-module’. [emphasis added]

And -- gasp -- it's possible for the same package to contain both the core submodule and a meta-module that exposes a user-friendly interface through some combination of wrappers, multiple supported calling styles, and overridable default extensions.

Ergonomics are important. <Link to='widget' params={{widgetId: 'foo'}} /> offers readability advantages over <Link to={{name: 'widget', params: {widgetId: 'foo'}}>, especially if params is large, and possibly also higher-order programming advantages.

Users shouldn't have to import an external package to get named routes, because that's one more package to worry about and whether the extension API changes.

@taion
Copy link
Contributor

taion commented Apr 15, 2016

That's certainly an option we're considering.

However, upstream React does use the "addon package" pattern quite extensively. See https://www.npmjs.com/package/react-addons-css-transition-group and https://www.npmjs.com/package/react-addons-shallow-compare most notably.

We agree that this is a feature that many users will want – otherwise I wouldn't have taken the time to code it up in the first place (as I don't use it) – and that is a factor we will consider, but I don't think there's anything wrong with the "addon package" model.

That also brings up a separate point – I wrote it to demonstrate that it was possible, but I don't use named routes, and I'm not sure any of the other project collaborators do as well. While there's not much code that can break, and I sort of care as a point of pride, it might be best for this feature to be maintained by someone who actually uses it.

@alvelig
Copy link

alvelig commented Jan 11, 2017

@taion If you don't use named routes, what is the best way to archive paths reusage. I mean if I want to type paths in one place and use corresponding variables in places where I need it? Is https://www.npmjs.com/package/use-named-routes the best way to archive it or is there another pattern?

Kind of:

const myRoutePath = "/here/we/go";

<Route path="/here" ... >
  <Route path="we" ...>
    <Route path="go" ... />
  </Route>
</Route>

<Link to="myRoutePath" >MY ROUTE PATH</Link>

@iErik
Copy link

iErik commented Feb 16, 2017

Every single major SPA Router has this feature, the React community itself has been insisting for years now that it is an important feature, who had the idea that Named Routes is an disposable feature? At this point, the only drawback of using React is having to use React Router, a routing library that doesn't even support named routes.

@rande
Copy link

rande commented Feb 16, 2017

@iErik you might be disagree with this choice (I am too) however, there is a way to express opinion... also nothing force you to use this code ... also nothing force you to write a small helper fonction to generate routes.

@timdorr
Copy link
Member

timdorr commented Feb 16, 2017

You can build this with something like the Route Config example from the docs: https://reacttraining.com/react-router/examples/route-config It no longer has to be baked into the core, just wrap some components with a dash of magic and voilà, you've got named routes.

Something like that would make a great addon package for 4.0. I'd definitely be interested in having that live alongside react-router-dom, react-router-native, and some other stuff that's going to live here in the near future. We're looking to build a whole ecosystem of addon packages to the simple core of react-router. No more batteries included, but the batteries will be available nearby 😄

If anyone wants to champion an effort on that, please submit a PR and we can get the ball moving.

@remix-run remix-run locked and limited conversation to collaborators Mar 6, 2017
@ryanflorence
Copy link
Member

Implementing a static route config and named routes on top of this project at this point is like a 20 line ordeal. 😂 If we baked it in we couldn't do recursive, dynamic routing.

SCREW US FOR GIVING YOU A MORE POWERFUL ABSTRACTION

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