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

Route component #40

Merged
merged 9 commits into from
Jun 24, 2014
Merged

Route component #40

merged 9 commits into from
Jun 24, 2014

Conversation

mjackson
Copy link
Member

This PR simplifies things quite a bit.

There are now only two types of React components: Route and Link. Both components are able to be rendered normally using React.renderComponent. Everything that was previously encapsulated in Router is now in Route, but only the top-level routes ever render. All others are used for configuration, as before.

The recommended usage is now:

var routes = (
  <Route handler={App}>
    <Route name="user" path="user/:userId" handler={User}/>
  </Route>
);

React.renderComponent(routes, document.body);

which should be more familiar to React users than our custom Router#renderComponent method.

This work should help with #38 and #34.

@ryanflorence
Copy link
Member

don't merge this yet, we need to reword the commit messages with [changed], [added] or [removed] so they go in the changelog automatically when I do a release

@ryanflorence
Copy link
Member

also, do we want to add a Router export for backwards compat that we'll remove with the 1.0 release?

mjackson and others added 8 commits June 23, 2014 20:35
The Router interface is deprecated in favor of using <Route>
components directly with React.renderComponent.
webpack-karma was bundled everything into every
single spec we had (5 copies of react, 5 copies
of the router, etc).

While having to put every spec we want to require
into the specs/main.js file isn’t ideal, it
works. I imagine there is some configuration
option I’m unaware of that removes the need for
this workaround.

Also, Promise was not being required, some
environments have it, some don’t.
@mjackson
Copy link
Member Author

@rpflorence Good call on keeping the Router interface around. I added it back with a deprecation warning, removed the initialPath thing, and cleaned up the commit history a little. I also updated the commit message to use the [changed] prefix.

mjackson added a commit that referenced this pull request Jun 24, 2014
@mjackson mjackson merged commit 9e7e04f into master Jun 24, 2014
@sophiebits
Copy link
Contributor

I personally find this more confusing because now Route sometimes describes what routes exist and sometimes renders an actual component to the DOM? It would make more sense to me if routes was the same as you have it, but the render call looked something like

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

Then there's an obvious place to support passing in a path for server rendering, etc:

React.renderComponentToString(<Router routes={routes} path="/user/17" />);

(Other props could also be added to that Router, like whether to use window.location.hash or pushState.)

@sophiebits
Copy link
Contributor

Or maybe I'm misunderstanding how nested routes work? It's not clear to me whether the top-level component behaves differently from the others.

@mjackson
Copy link
Member Author

@spicyj Yes, the top-level component acts as the router for all routes underneath it. All other routes are simply used for configuration.

Using the same component class has some nice benefits, including the fact that it makes it easy to split out your routes into multiple vars and then combine them again into a single component declaration at the end. The obvious place to pass in top-level configuration is on the top-level Route that actually gets rendered into the page.

@sophiebits
Copy link
Contributor

I'm not sure what you mean. Even in my idea you can break up the routes and write:

var userRoutes = (
  <Route name="users" handler={Users}>
    <Route name="user" handler={User} path="/user/:id"/>
  </Route>
);
var routes = (
  <Route handler={App}>
    <Route name="about" handler={About}/>
    {userRoutes}
  </Route>
);
React.renderComponent(<Router routes={routes} />, document.body);

Of course it's nice if any level of route can be composted within another, but the actual rendering responsibility seems to be a different concern. You could also imagine that in another world, the route configuration would be plain JSON, and then you would need to have a separate component to render them. Basically, there's no reason that the subroutes need to be React components at all, so it seems confusing to conflate the two concepts.

@mjackson mjackson deleted the route-component branch July 22, 2014 19:59
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 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 this pull request may close these issues.

3 participants