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

Go back to <Routes><Route/><Routes> #112

Closed
ryanflorence opened this issue Jul 24, 2014 · 16 comments
Closed

Go back to <Routes><Route/><Routes> #112

ryanflorence opened this issue Jul 24, 2014 · 16 comments

Comments

@ryanflorence
Copy link
Member

Proposal

Bring back <Routes/> instead of "overloading" <Route/> with two roles. Currently the root Route is really the only thing doing anything interesting, the child Routes are just config.

Justification

  • The root route performs a different role than child routes
  • The root Route accepts props that are meaningless to child routes, like location
  • We will likely get more of these child-route-meaningless-configuration-options
  • Documenting Route, primarily the props, is more confusing than documenting Route and Routes
  • Making Route more extensible seems like it'd be easier since it'll be an empty component with a few propTypes.

Opposition

  • Just using Route all the way down is simpler for consumers
  • (anecdotal) nobody but @spicyj has seemed bothered by this

Questions

Should Routes take a handler

<Routes handler={App}>
  <Route name="users" handler={Users}/>
  <Route name="dashboard" handler={Dashboard}/>
</Routes>

v.

<Routes>
  <Route handler={App}>
    <Route name="users" handler={Users}/>
    <Route name="dashboard" handler={Dashboard}/>
  </Route>
</Routes>

I don't know all the implications of not having a root handler that is always active, and always rendered.

@mjackson
Copy link
Member

Yeah, the more I'm using React the more I've been feeling like we should bring back <Routes>. If <Routes> does have a handler, it shouldn't be called handler and it shouldn't always be rendered. Instead, it should be called defaultHandler or something similar and should be the thing that renders when no other routes are active.

@sophiebits
Copy link
Contributor

👍 from me (obviously).

I think Routes should not take a handler object, mostly because it doesn't need to. Separating the route configuration (paths/names/handlers) from any runtime router flags (like location="history") makes a cleaner API in my opinion. As far as I can tell, there's no reason to require a single root route except that it's less clear what a URL with no matching route should render (though I think that ambiguity is present regardless, and this API change just makes it more obvious).

@tarjei
Copy link

tarjei commented Jul 24, 2014

A Routes component would also be the logical place to add default handlers for errors and not found urls.

@jstayton
Copy link

👍

@mjackson
Copy link
Member

I should add that the default defaultHandler should just return null.

@ryanflorence
Copy link
Member Author

Vote no on proposition notFound!

I am opposed to a "notFound" handler. I still want my not-found handling to be wrapped inside my application chrome. Today, you just check if your root handler has an activeChildHandler or not.

render: function() {
  return (
    <div>
      <h1>My Site!</h1>
      {this.activeRouteHandler() || this.renderNotFound()}
    </div>
  );
}

If we have <Routes notFoundHandler={NotFound}/>, how do I get my application chrome in there? I'd have to resort to Header() and Footer() things, which is not really the way sharing layout UI with this router works. A NotFound handler makes you have to write your code quite differently, and breaks the nesting paradigm a little bit.

Same for errorHandler

Let the handler with an error tell you there was an error. I want to give the user good information about the error and keep them in the UI they were at. If we have some ErrorHandler up at the top, then all of your nested UI gets blown away.

Vote yes to Routes Handler!

Having something rendered all the time is the path to least surprise, if something isn't rendering, its your code you sort through, not stuff in the router. It also gives you an opportunity to do any "notFound" and "error" handling you want, inside of your application layout.

You always have an application, I can't see any benefit in not always having something to render that fits within the nesting paradigm.

@ryanflorence
Copy link
Member Author

Additionally, imagine these docs:

Handlers on the Routes component do not nest, but handlers on the Route component do. Why? [insert reason that eludes me]

@bobeagan
Copy link
Contributor

👍 for Route/Routes and also keeping error and 404 handling out of the routes config

@paulstatezny
Copy link

👍 👍 Completely agree, @rpflorence.

@mjackson
Copy link
Member

Notice I didn't call the defaultHandler a NotFoundHandler or ErrorHandler. It isn't really for either of those use cases. I agree that the API already provides good mechanisms for handling both.

But there is currently a small hole and it is: "what do you render when there's nothing else to render?". Currently we just render null and emit a warning. The defaultHandler would essentially be a hook for the user to handle that case if they choose to. In other words, we already have a default handler in the code. <Routes defaultHandler> just exposes it.

Is it a hole worth plugging? Probably not. That's why I said "if <Routes> has a handler"...

Anyway, the whole reason for separating <Routes> from <Route> is to prevent confusion. One renders into the document, the other is used for configuration. Giving <Routes> a handler just means it doubles as a renderable and config.

@ryanflorence
Copy link
Member Author

What I want to avoid is this in every app:

<Routes>
  <Route handler={App}>
  </Route>
</Routes>

That would be the new required boilerplate for pretty much everything.

Anyway, the whole reason for separating <Routes> from <Route> is to prevent confusion.

I don't think its confusing to document that Routes is just like Route but also contains the config options.

I do think it is confusing (currently) that the positioning in the hierarchy of a Route changes the properties that matter.

I see your point on defaultHandler, if somebody just never provides a handler anywhere its what we do, but I don't see the use case where I'd not just rather provide a handler.

defaultHandler is really just notFoundHandler with a different name.

@ryanflorence
Copy link
Member Author

Here's some code to talk about,

<Routes handler={Root}>
  <Route name="app" path="/" handler={App}/>
  <Route name="admin" handler={Admin}/>
</Routes>

v.

<Routes defaultHandler={NotFound}>
  <Route name="app" path="/" handler={App}/>
  <Route name="admin" handler={Admin}/>
</Routes>

I much prefer the first because the user gets an idiomatic place to deal with the lack of an activeRouteHandler instead of the router doing something about it. They can also omit the handler on Routes if they really want to, no differently than if we don't allow Routes to have a handler.

I realize we already "do something about it" by way of render null, but I think the warning is enough, in either case they have to provide a handler, I'd rather it was one that worked like the others.

@mjackson
Copy link
Member

defaultHandler is really just notFoundHandler with a different name

Fair enough. That's probably what most people will use it for anyway. I can't really think of any other use cases.

Just to clarify, your examples aren't equivalent. They should be:

<Routes handler={Root}>
  <Route name="app" path="/" handler={App}/>
  <Route name="admin" handler={Admin}/>
  <Route handler={NotFound}/>
</Routes>

v.

<Routes defaultHandler={NotFound}>
  <Route handler={Root}>
    <Route name="app" path="/" handler={App}/>
    <Route name="admin" handler={Admin}/>
  </Route>
</Routes>

The second one makes it more obvious to me what's going on. It isn't currently obvious to users where to put NotFound stuff, or that it applies globally. And we've had quite a few questions about it.

Additionally, you could keep the boilerplate to a minimum by just doing:

// Declare your routes just like we do now.
var routes = (
  <Route ... />
);

React.renderComponent(<Routes routes={routes}/>, document.body);

The <Routes routes> prop is same as passing children to <Routes>. This definitely feels more React-y to me.

@mjackson
Copy link
Member

I might also point out that in the first example your NotFound handler is forced to be a child of your Root handler, so they're coupled. This means that if you build your app and then decide sometime later that you want to add a NotFound page you need to either a) make sure it fits within Root's UI or b) refactor things so that Root's UI is really generic (and essentially blank). In any case, you're going to at least have an extra <div> surrounding your NotFound UI, which is annoying. A lot of sites have completely separate pages for not found.

@ryanflorence
Copy link
Member Author

:shipit: May the root route handlers rest in peace 🙏

@mjackson
Copy link
Member

❤️ :)

sophiebits added a commit to sophiebits/react-router that referenced this issue Jul 26, 2014
Fixes remix-run#112.

Test Plan: Loaded each of the examples and clicked around. npm test too.
@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

Successfully merging a pull request may close this issue.

7 participants