remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.06k stars 10.3k forks source link

Bring back <Route name> #1840

Closed mjackson closed 9 years ago

mjackson commented 9 years ago

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:

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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.

jedwards1211 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

rpominov commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

gaearon commented 9 years ago

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 commented 9 years ago

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

ryanflorence commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

+1

rande commented 9 years ago

The benefit of named routes are:

confiks commented 9 years ago

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.

nkt commented 9 years ago

What if you will bring back named routes, like they have been, but in the more extensible way. You can introduce RouteMatcher interface and accept matcher in Router.create().

interface RouteMatcher {
  add(name : string, path : string);

  format(name : string, params : object, query : object) : string;

  match(path : string) : {name : string, params : object};
}

So you can implement your own standard matcher and if it's not enough for someone, he can use external matcher.

rande commented 9 years ago

@nkt In the php world, the Symfony's router has 2 mains services: the generator and the matcher. It is also possible with the CMF bundle to have a chain router so you can encapsulate different generators and matchers.

Vincz commented 9 years ago

+1 to bring them back. One more reason is the ability to translate route paths. As @rande said, you should get inspiration from the Symfony's router.

dminkovsky commented 9 years ago

Deleted previously posted comment here because: what is the point?

confiks commented 9 years ago

@dminkovsky The point is that you can make your voice heard, and we can say: hear hear (or: nay)

We cannot now.

jquense commented 9 years ago

@mjackson @ryanflorence above there was a bunch of talk about how we can implement this stuff ourselves. However I haven't heard any response to some of the issues we've posted about the challenges with that approach.

The real big one for us being that the provided public api's aren't actually sufficient, we need to reach in an use private ones like PatternUtils, in order to effectively reverse RR's path matching

th0r commented 9 years ago

@dminkovsky Haha! You rock!))

SpainTrain commented 9 years ago

Remember, Cool URIs Don't Change. :sunglasses:

confiks commented 9 years ago

In addition to my comment above, here's a gist with a more complete hack on 1.0.0-rc1 to nastily bolt on named routes again. Expect corner-case issues.

dminkovsky commented 9 years ago

@confiks true, but my criticism wasn't constructive. I meant what's the point of posting if you're not constructive and it's RC1 already?

I just have a visceral dislike for propagating

<Link to={`/booking/${id}/${solutionId}?${partnerId}`} />

through code. Hard to stomach. And who cares if "Cool URIs Don't Change"? Who says I'm trying to be cool?

But obviously it is what it is (RC1), so it makes sense to get with the program given that I can't afford to spend programming time on a router when this project exists. For which, despite any or all criticism, I am very grateful.

@th0r thanks :D

SpainTrain commented 9 years ago

And who cares if "Cool URIs Don't Change"? Who says I'm trying to be cool?

Hah, well I suspect that title is just special brand Berners-Lee style humor. The article itself discusses why W3C recommends not changing URIs. While many of the examples are amusingly out of date, it's thematically similar to this thread.

I'd assumed folks would already be familiar with it, so intended as a relevant-yet-tongue-in-cheek reference.

anru commented 9 years ago

Named routes should be implemented. Lack of this feature leads to harder upgrade plan to migrate to React 0.14

mjackson commented 9 years ago

Eh, ok. Thanks to everyone who is voicing gratitude and thank-yous :) I really appreciate it. And to everyone else, no worries. I'm not offended in the least.

To sum it all up, it seems like the major arguments for bringing back <Route name> are:

  1. People don't like manually building URLs. Point taken. I think we can figure out some better patterns for building URLs than just bare string interpolation all over the place. For one, I like @gaearon's suggestion to create a named function that just returns an object you can use in <Link to>. I think that's a great idea that could solve the heartache of manually typing out URLs. Let's discuss further in #2177.
  2. Using named routes made it easy to check which ones are active. Well, @idolize, there's technically nothing stopping you from putting a name prop on your <Route> and using your same isActiveRoute function. Just because we don't do anything with that prop doesn't mean you can't :) As for routes without names, we have a isActive API for those cases.

Am I missing anything? Or can we close this?

rickharrison commented 9 years ago

The only thing we are missing is how to solve the upgrade pain problem. On one of these nights or weekends, I was going to investigate writing something as suggested above to help with the loss of named routes. I just want to mention it again for other people like me who have large apps in production already and will need to figure out this migration process.

confiks commented 9 years ago

In my view there are still multiple major issue:

  1. You are breaking the API in a big, big way, for benefits that seem rather trivial or absent. You name four "important issues" that brought about this change at the top of this issue. Of all the issues brought up, actually only the first one is not an ideology-based issue. And even that one seems rather mundane and easy to fix.
  2. Named routes simplify changing your paths. You take the position that you shouldn't change your paths at all, but this isn't at all practical. Many people dislike regex'ing all components for all Link component which reference some path you changed. Refactoring code become cumbersome. You argue that you can bolt on lookups in development, but this isn't practical because you can't easily switch paradigms: names or paths, choose one. You basically have to build a complete abstraction on top. This should be built-in.
  3. Non-named routes can be confusing. Naming your routes may relieve some of the mental burden of remembering all paths, and specifically all edge-cases inside those paths. You may choose a path name that documents it's own usage. In a lot of cases, you can't document paths because your user/customer will see those paths.
  4. Named routes made it completely trivial to bolt on i18n (i.e. vary path names for the same route). I don't quite see how to do it now.

On a personal level: react-router has had a lot of breaking API changes in the last year, and I've spent >50% of my refactoring-because-of-lib-upgrades time on react-router alone, among many other libraries that were upgraded. That's okay; it's open source progress, and I appreciate your effort on it. I do regret that this change seems so utterly useless to me.

rpominov commented 9 years ago

Am I missing anything? Or can we close this?

What about the case I tried to describe in https://github.com/rackt/react-router/issues/1840#issuecomment-139819503

It probably wasn't very clear what I meant without an example. So here is an example:

Suppose I want to create some reusable module that has several pages. It will contain following files (pseudocode):

// PageA.js

export default React.createClass({
  render() {
    return <div>
      <Link name="pageB">Go to B</Link>
    </div>
  }
})
// PageB.js

export default React.createClass({
  render() {
    return <div>
      <Link name="pageA">Go to A</Link>
    </div>
  }
})
// index.js

export default [
  <Route name="pageA" path="/a" component={PageA} />,
  <Route name="pageB" path="/b" component={PageB} />,
]

The main file that this module exports is index.js, it contains only subtree of router config. The consumer of the module is free to insert that subtree wherever in their router config. For instance:

// appCode.js

import pageABModule from 'pageABModule'

<Router>
  <Route path="/foo">
    {pageABModule}
  </Route>
</Router>

As the result the PageA will be available at /foo/a, and the link to page B from page A will have href="/foo/b".

What this example is trying to illustrate, is that without named routes, we wouldn't be able to construct path for the link to PageB from PageA (we wouldn't know that we should prefix it with /foo)

Update

Actually I am not sure the above will work with named routes, I didn't tried to do this with previous version of react-router (that has named routes). But still this seems like a use case worth consideration.

mjackson commented 9 years ago

@confiks I'm sorry for the upgrade pain you've had over the past year. We made our semver policy very clear at the top of our README in 0.13.x, and we've maintained a detailed upgrade guide every step of the way. Not sure how else we could've built this thing and gotten it right without making a few breaking changes along the way. I believe all of your other concerns are already addressed in this issue.

@rpominov Base URLs are handled transparently in 1.0 using the useBasename history enhancer. Essentially, you just need to make all of your <Route path>s relative and let whoever uses your library decide what base URL to use.

rpominov commented 9 years ago

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

timdorr commented 9 years ago

@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 commented 9 years ago

@rpominov :+1: :D

mjackson commented 9 years ago

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

taion commented 9 years ago

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 commented 9 years ago

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

Please deprecate in the future.

taion commented 8 years ago

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

romanlv commented 8 years ago

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

taion commented 8 years ago

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.