Skip to content

Allow partial declaration of routes #193

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

Closed
abergs opened this issue Aug 12, 2014 · 24 comments · Fixed by #200
Closed

Allow partial declaration of routes #193

abergs opened this issue Aug 12, 2014 · 24 comments · Fixed by #200

Comments

@abergs
Copy link
Contributor

abergs commented Aug 12, 2014

Very useful for when you want to split your routes into different sections:

var ContactsRoutes = require("contactRoutes");
...
<Routes>
    <Route path="/" handler={home}>
        {ContactsRoutes}        
        {BookingRoutes}
        {ItemsRoutes}
    </Route>    
  </Routes>

Currently, the above solution works, but only as long as you only have one <Route> with nested child routes within your required Routesmodules (e.g. ContactsRoutes).

It doesn't work if I have multiple routes inside of ContactsRoutes, which are not nested, because of JSX not allowing multiple adjacent components.

My suggestion is to reuse <Routes> as a container (with no additional behavior).

@mtscout6
Copy link
Contributor

+1 As my initial stab at #190 showed I originally assumed that I could build up a component that only housed routes. I'd much rather see something like:

var ContactsRoutes = require("contactRoutes");
...
<Routes>
    <Route path="/" handler={home}>
        <ContactsRoutes />   
        <BookingRoutes />
        <ItemsRoutes />
    </Route>    
  </Routes>

@abergs
Copy link
Contributor Author

abergs commented Aug 12, 2014

Yes that would be even nicer.

@ryanflorence
Copy link
Member

Its just JSX, return an array:

var someRoutes = function() {
  return [<Route/>, <Route/>];
}

@abergs
Copy link
Contributor Author

abergs commented Aug 12, 2014

Cool, didn't know about that trick! Thank you.

Skickat från min Windows Phone


Från: Ryan Florence
Skickat: 2014-08-12 23:10
Till: rackt/react-router
Kopia: Anders Åberg
Ämne: Re: [react-router] Allow partial declaration of routes (#193)

Its just JSX, return an array:

var someRoutes = function() {
  return [<Route/>, <Route/>];
}

Reply to this email directly or view it on GitHub:
#193 (comment)

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

I can't get this to work, perhaps I am missing something? when packing the routes in an array the router throws an exception here:

function findMatches(path, route) {
  var children = route.props.children, matches;

because route is an array.

This is the code:

var someRoutes = function() { return [<Route />, <Route />]; };

var app = (
<Routes>
    <Route path="/" handler={home}>
        <someRoutes />
    </Route>
</Routes>

Either the findMatches could unpack to route component or I guess it would be possible (but not so nice) to write some unpacking code on someRoutes before inserting it into <Routes>

Am I missing something?

@mtscout6
Copy link
Contributor

The problem there is that although JSX would properly transform <someRoutes /> to someRoutes() React is expecting the return result to be a single React component or null.

If you changed it to {someRoutes()} it should work. I know that sounds counter-intuitive, but the difference is where the function is actually getting invoked. When you invoke it you are sending React an array of component constructors, and React knows how to handle that. When you let React do it, it is expecting a React component constructor function, not just any old function.

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

I tried that, and I get the error Cannot read property 'children' of undefined in findMatches

@mtscout6
Copy link
Contributor

Ah, I scrolled down to far and didn't see the beginning of your question. Sorry about that.

@mtscout6
Copy link
Contributor

The way I'm currently doing it is:

var someRoutes = (
  <Route ...>
    <Route ... />
    <Route ... />
  </Route>
);

var app = (
<Routes>
    <Route path="/" handler={home}>
        {someRoutes}
    </Route>
</Routes>

Have you tried removing the function wrapper and just assigning an array. Something like:

var someRoutes = [<Route />, <Route />];

var app = (
<Routes>
    <Route path="/" handler={home}>
        {someRoutes}
    </Route>
</Routes>

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

Yes, that doesn't work either unfortunately.. (the same error message as above)
This is what the transformed JSX looks like:

var someRoutes =  [
        Route({name: "booking", handler: home}, 
            Route({name: "booking/quickEdit", path: "/booking/quickEdit/:bookingId", handler: home, defaultLeaveRoute: "booking"})
        ),
        Route({name: "booking/contacts", handler: home}, 
                Route({name: "booking/contact", path: "/booking/contacts/:contactId", handler: home})
        )
        ];
 application = (
     Routes(null, 
        Route({path: "/", handler: home}, 
            Route({name: "start", handler: Start}), 
            someRoutes
        )
      )
    );

@ryanflorence
Copy link
Member

/** @jsx React.DOM */
var Router = require('react-router');
var Routes = Router.Routes
var Route = Router.Route;

var moreRoutes = function() {
  return [
    <Route name="foo" handler={require('../components/foo')} />,
    <Route name="bar" path="/what/evz" handler={require('../components/bar')} />,
    <Route name="index" path="/" handler={require('../components/index')} />
  ]
}

module.exports = (
  <Routes>
    <Route location="history" handler={require('../components/app')}>
      {moreRoutes()}
    </Route>
  </Routes>
);

Works in the rackt/react-boilerplate app.

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

Aha! So I tried your example @rpflorence and ofcourse it worked. I dig some fast digging and the problem is when you combine "packaged" routes and "local", like this:

<Route location="history" handler={require('../components/app')}>
  {moreRoutes()}
  <Route name="whatever" ... />
</Route>

Which I guess doesn't let react unpack the array because the children is now a sort of nested array. The solution is simply to not mix an array together with a lone <Route>

And just to clear things up, it doesn't matter if it is a function or a array.

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

Perhaps issuing a warning/error if the RouteComponent detects one of it's children to be an array, I can't be the only newcomer to run into this invisible wall, no? 👊

@ryanflorence
Copy link
Member

This is all just JSX, isn't it? Is the router getting in the way?

On Wed, Aug 13, 2014 at 8:21 AM, Anders Åberg [email protected]
wrote:

Perhaps issuing a warning/error if the RouteComponent detects one of it's
children to be an array, I can't be the only newcomer to run into this
invisible wall, no? [image: 👊]


Reply to this email directly or view it on GitHub
#193 (comment).

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

Actually react seems to handle this differently (??)

Because this renders fine:

var application = React.createClass({
    render: function () {
            var other = [
                <button>2</button>,
                <button>3</button>
            ];

        return (
            <div>
            <button>1</button>
            {other}
            </div>
            );
    }
});

@ryanflorence
Copy link
Member

I don't mean to be a grump, but I don't see myself spending any time
getting this use-case to work, I don't like the idea of splitting up my
route configs like this. But if you want to take the time to make it work,
I would happily merge the pull request :)

On Wed, Aug 13, 2014 at 8:41 AM, Anders Åberg [email protected]
wrote:

Actually react seems to handle this differently (??)

Because this renders fine:

var application = React.createClass({
render: function () {
var other = [
2,
3
];

return (
    <div>
    <button>1</button>
    {other}
    </div>
    );

}

});


Reply to this email directly or view it on GitHub
#193 (comment).

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

No problem @rpflorence, sorry for being all over you in the /issues. I will happily create a PR.Thanks for all your time and help tracking this down, @rpflorence and @mtscout6

@mtscout6
Copy link
Contributor

@rpflorence I understand your reluctance to fix this bug yourself, but may I suggest that the issue remain open or open a new issue. It may even make sense to rename it to more adequately reflect this bug. This is now a known issue that should get resolved at some point, and again I'm not saying that you have to do so. My main motivation for this would be that first it is not forgotten about. Second so that follow on users that also have this problem would see that it is already a known issue.

@ryanflorence ryanflorence reopened this Aug 13, 2014
@ryanflorence
Copy link
Member

woops, meant to reopen! thanks.

@mjackson
Copy link
Member

This sounds like something that should be handled by React.Children.forEach, which we're using to iterate over the route config. Seems like if it knew how to handle child arrays, we wouldn't be having this problem.

@spicyj Can you confirm/deny?

@abergs
Copy link
Contributor Author

abergs commented Aug 13, 2014

@mjackson I did an console.log(route) at the top of registerRoute which seemed to identify everything as OK.

The problem is visible first in findMatch, but that seems to be a bad place to do the extra isArray check. Had to cancel the work on this for the evening but will continue tomorrow.

@mjackson
Copy link
Member

@abergs Ah, thank you. Sounds like that's the right place to fix the bug.

@mjackson
Copy link
Member

Just to clarify, the only reason I'm not using React.Children.forEach inside findMatch is because I need it to be cancellable. In other words, I need to break early once the match is found. We need a more robust React.Children.find or something.

mjackson added a commit that referenced this issue Aug 14, 2014
Also, changed behavior of routes with no name, path, or children so
they act as default routes. <DefaultRoute> is essentially just sugar.

Fixes #164
Fixes #193
abergs added a commit to abergs/react-router that referenced this issue Aug 14, 2014
@abergs
Copy link
Contributor Author

abergs commented Aug 14, 2014

Oh, didn't see that you did work as well @mjackson. I did a small change to allow nested arrays of routes, my examples worked good with it. Seems like you did that also, as well as some more work with <DefaultRoute>.

Perhaps we did similar work? :)

mjackson added a commit that referenced this issue Aug 14, 2014
Also, changed behavior of routes with no name, path, or children so
they act as default routes. <DefaultRoute> is essentially just sugar.

Fixes #164
Fixes #193
@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.

4 participants