Separate <Routes> component from <Route> spec

[sophiebits]

Fixes #112.

Test Plan: Loaded each of the examples and clicked around. npm test too.

[sophiebits]

(partial-app-loading is as broken as before; the other examples work.)

[sophiebits]

This test was previously not asserting anything useful, as far as I can tell.

[ryanflorence]

is <Routes>{routes}</Routes> too much typing? I'm not excited about convenience properties in JSX #109

[ryanflorence]

Awesome work @spicyj :)

I wonder if having to specify the path / is a bummer or not. We could do it automatically by identifying only one Route child of Routes.

[mjackson]

@rpflorence My preferred usage would be:

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

Otherwise, <Routes>{routes}</Routes> also works. It should be trivial to add a

componentDidMount: function () {
  setupRoutes(this.props.routes || this.props.children);
}

to Routes.

[ryanflorence]

I know its easy for us to add, but you know me and documenting things. I'd rather it was "just JSX" and we dont' document anything than "also, you can pass the children in as a property instead of as children". I just don't want to start making shortcut properties all over the place when there is a "normal" JSX way of doing something.

[ryanflorence]

Come to think of it ... <Routes children={routes}/>

Does that work?

[2 minutes later...]

Yes it does.

[mjackson]

@rpflorence Ah, yes. I should've seen that! :) Good catch.

[sophiebits]

I wonder if having to specify the path / is a bummer or not.

Personally, I like it. Every other routable path is has a path (or name) listed; it seems fitting that the root should too.

[ryanflorence]

README needs an update but I could do that in a different commit I suppose. I haven't looked too much at the code but I'm 👍 on the API

[mjackson]

I'd like to be able to leave out the path prop. Other than that this looks great.

[ryanflorence]

We can only default the path to '/' if there is only one Route under Routes.

[sophiebits]

In Flask you write @app.route("/") on your handler; in Rails you write root to: 'pages#main'; in Django you write url(r'^$', 'apps.main.views.homepage'),. I don't see why we should assign it implicitly here; I think doing so only makes this more confusing. You should also be able to write something like

<Routes>
  <Route handler={App}>
    <Route name="home" path="/" handler={Home} />
    <Route name="about" handler={About} />
  </Route>
</Routes>

without the App route getting an implicit path="/".

[mjackson]

I'd like to be able to do this:

<Routes>
  <Route handler={App}>
    <Route name="home" handler={Home} />
    <Route name="about" handler={About} />
  </Route>
</Routes>

and leave paths out of the tree entirely if I choose. This gives me 3 URLs:

1. / which is just App
2. /home which is Home inside App's UI
3. /about which is About inside App's UI

I don't think that's confusing at all.

[ryanflorence]

@spicy, currently we have default '/' for the root route, but <Route name="home" path="/" handler={Home} /> also works as you'd expect.

[ryanflorence]

Because there is no required single root route, having an automatic / path does make the API less predictable:

<Routes>
  <Route handler={App}>
    <!-- more routes -->
  </Route>
</Routes>

Everything works great, and my app route has an implicit path of /.

But then I add an admin section to the app:

<Routes>
  <Route handler={App}>
    <!-- more routes -->
  </Route>

  <Route handler={Admin}>
    <!-- more routes -->
  </Route>
</Routes>

:trombone: Which one gets the / path? The Router code can certainly handle this, but this is confusing and requires too much knowledge about the router for the everyday user. Adding a sibling route should not cause these kinds of problems and questions.

I wish we still had a required root handler with an implicit path of /... These questions go away. Since I'm the only one rooting for that (get it?), and I lost, then I think there should be no implicit / path anywhere.

[mjackson]

this is confusing and requires too much knowledge about the router

Adding path="/" does not make it any less confusing.

<Routes>
  <Route path="/" handler={App}>
    <!-- more routes -->
  </Route>

  <Route path="/" handler={Admin}>
    <!-- more routes -->
  </Route>
</Routes>

The predictability doesn't come from putting explicit paths on every route. Instead, it comes from how you order them, top to bottom. This is exactly how it's done in a lot of web frameworks, e.g. Flask, Sinatra, etc. You scan your routes top to bottom.

Predictability is already one of the router's greatest strengths. The most deeply nested route that matches the URL is the one that renders. Simple.

[sophiebits]

Your last example looks obviously wrong to me because you have two routes with path="/". Having two root routes without a path, on the other hand, does not look nearly as wrong.

Can you help me understand when you would want this hierarchy?

<Routes>
  <Route handler={App}>
    <Route name="home" handler={Home} />
    <Route name="about" handler={About} />
  </Route>
</Routes>

What does App render when you visit /? Does it notice that this.props.activeRouteHandler == null and render some home-like content (or I suppose it could redirect to /home)? If you were to render any actual content on the home page then it seems you'd want an actual (nested) route for it and would need to specify path="/" on it anyway.

[ryanflorence]

@spicyj exactly. this.prop.activeHandler() || this.renderIndex() or if (!this.props.activeRouteHandler()) Router.replaceWith('dashboard');

I can't stop on the root route ... it gives you a place to deal with a visit from the user no matter what, whether that's to redirect in a transition hook, or render some empty "home" or "not found" template if nothing else matches, all in the same nested structure devs are used to.

[mjackson]

It helps when you're thinking about things in terms of UI instead of paths.

<Routes>
  <Route handler={LoggedOutUI}>
    <Route name="login" handler={Login} />
    <Route name="forgot-password" handler={ForgotPassword} />
  </Route>
  <Route handler={LoggedInUI}>
    <Route name="home" handler={Home} />
    <Route name="about" handler={About} />
  </Route>
</Routes>

In this example, I'm totally focused on UI. I don't care what the path is because it's not important. The only thing I care about is that the LoggedOutUI appears around stuff when the user is logged out and LoggedInUI appears around stuff when the user is logged in, paths be damned! :)

[ryanflorence]

@mjackson so what happens when I just visit / and not one of those matched paths? Wich gets the default /? The first? That seems confusing to me, I get the "first to match" but "first to match a default path because its a root route" is a bigger step. I agree with @spicyj that having path="/" on two is more obvious you've caused a problem.

[mjackson]

The first?

Yes. It didn't take you long to reason it out. I don't understand why you think it will be difficult for others.

In LoggedOutUI I'd probably do:

willTransitionTo: function (transition) {
  if (transition.path === '/') transition.redirect('login');
}

[ryanflorence]

I don't understand why you think it will be difficult for others

Because you and I are used to MAGIC

Alright, I will concede. I'm fine with no root routes and the first "root" route getting a default /.

That means transitioning from 0.4 -> 0.5

0.4.x

<Route location="history" handler={App}>
  <Route name="dashboard" handler={Dashboard} />
</Route>

0.5.x

<Routes location="history">
  <Route handler={App}>
    <Route name="dashboard" handler={Dashboard} />
  </Route>
</Routes>

Seems reasonable. If people start getting confused about where Router.transitionTo('/') is going to take them, then we can revisit 1/3 of this conversation.

[mjackson]

😆

Sounds good.

[ryanflorence]

So are we just saying "routes without a path or name default path to /"?

[ryanflorence]

Re: last comment, yes, yes we are.

@spicyj, please update the PR to put "/" back as the default path and lets merge this sucker. Unless you've still got your boxing gloves on, then by all means ...

[mjackson]

Last thing I'll say: if you really want to put path="/" in to make things more explicit for you and your team, by all means go for it! Just please don't force me to. :)

[ryanflorence]

we're still very early in this project, it'll be easy for us and others to update to a version that doesn't default route paths to /, but the current version does, lets just change one thing at a time, and worry about default paths if we start getting issues opened around it.

[sophiebits]

Rebased and changed to make path="/" the default, as before.

[ryanflorence]

sweet, I'm going to amend this commit with some README updates and commit it manually

don't click the merge button @mjackson

[sophiebits]

5af49d4

[ryanflorence]

queue final fantasy level up riff, welcome to the project @spicyj :D

[mjackson]

Thanks @spicyj! Great work!