React-router: Revisit code splitting API

[taion]

The current code splitting API can be somewhat difficult to use correctly. Consider the huge-apps example, specifically: https://github.com/reactjs/react-router/blob/v2.0.1/examples/huge-apps/routes/Course/index.js.

Because we split code with getChildRoutes (as this is what plays most nicely with the easy webpack support), the route implementation above means that whenever we load any child route under course/:courseId, we end up loading all the child routes.

In practice, the impact is somewhat mitigated in the example by also using require.ensure for getComponent, but that's actually suboptimal in a different way – we end up potentially making separate round-trips to the server to fetch the bundles for the route configuration, and for the route handler component.

What is to be done here? The most obvious approach is to replace the current dynamic route configuration hooks (getChildRoutes, getIndexRoute, getComponent, getComponents) with a single getRouteConfig (or better name). Instead of the above route configuration, we'd configure things as e.g.

import Course from 'components/Course'

module.exports = {
  // path is defined upstream
  component: Course,
  childRoutes: [
    {
      path: 'announcements',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Announcements'))
      }),
    },
    {
      path: 'assignments',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Assignments'))
      }),
    },
    {
      path: 'grades',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Grades'))
      }),
    }
  ]
}

[timdorr]

Question because I don't know enough about webpack's behaviors: Will the getChildRoutes's require.ensure of multiple requires end up grouping those into a single chunk?

The one bad thing I see with this is it pushes up the tree, which may not be desirable in some cases. Say you've got a team working on a big PM solution and one group is working on a calendar sub-project. Now they have to put their top-level routes (relative to the calendar components) into the main route config, rather than delegating to their own file namespace.

Also, what if I want to chunk with larger groups of files? It would seem to me that loading up new chunks on every new path kind of defeats the purpose of a single page app. Sure, they're small chunks, but it's death by a thousand cuts.

I don't think this is the wrong direction, but I think more use cases need to be mentally explored so we don't end up making the API more restrictive by making it simpler.

[taion]

I'll give a more concrete illustration of what's going on. It'll make the most sense if you pull up the examples and look at the requests being made.

Start at http://localhost:8080/huge-apps/course/0, then navigate to http://localhost:8080/huge-apps/course/0/announcements.

What should happen? I would like to see a single round-trip that fetches:

• routes/Course/routes/Announcements/index.js
• routes/Course/routes/Announcements/components/Announcements.js
• routes/Course/routes/Announcements/components/Sidebar.js

What does happen right now?

First we make one round-trip that fetches:

• routes/Course/routes/Announcements/index.js
• routes/Course/routes/Assignments/index.js
• routes/Course/routes/Grades/index.js

Then we make a _second_ round-trip that fetches:

• routes/Course/routes/Announcements/components/Announcements.js
• routes/Course/routes/Announcements/components/Sidebar.js

The first round-trip corresponds to https://github.com/reactjs/react-router/blob/master/examples/huge-apps/routes/Course/index.js#L5-L11. The second round-trip corresponds to https://github.com/reactjs/react-router/blob/master/examples/huge-apps/routes/Course/routes/Announcements/index.js#L13-L18.

You could take out the require.ensure in the latter, in which case everything would be one round-trip, but then you'd end up fetching the components for the other routes as well.

You can work around this with pathless routes in getChildRoutes, but it's a little bit messy. By default, you'll get suboptimal behavior with the current code splitting API.

Thus, we should clean up the API so that the straightforward approach is the correct one. It's not good when even our bundled examples show a suboptimal route splitting setup.

[taion]

The takeaway is that, as set up, the code splitting API does not allow all of:

• Straightforward route configuration
• No overfetching (of components – route definitions always get overfetched)
• Single round-trip for new data

Moving instead to getRouteConfig() or similar would allow resolving this trilemma.

[sethkinast]

On a related note, does getChildRoutes only work with non-JSX routes? I see the function getting invoked when I try to use it in a <Route/>, but the child routes don't seem to actually be used.

[taion]

It works just fine with JSX routes. You need to be a bit careful with how you use it, though, and you can't mix it with normal child routes.

Let's focus this issue on the API discussion, though – if you have questions, please use Stack Overflow or Reactiflux. I think there's enough to talk about here with respect to making the API as good as possible.

[sethkinast]

For sure-- my purpose in asking was to make sure a new API would work with
jsx if the current one didn't, not to get help with it. But it sounds like
a documentation gap and not a feature gap so all good.
On Apr 3, 2016 6:01 PM, "Jimmy Jia" [email protected] wrote:

It works just fine with JSX routes. You need to be a bit careful with how
you use it, though, and you can't mix it with normal child routes.

Let's focus this issue on the API discussion, though – if you have
questions, please use Stack Overflow or Reactiflux. I think there's enough
to talk about here with respect to making the API as good as possible.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/reactjs/react-router/issues/3232#issuecomment-205091533

[ryanflorence]

Then you're going to end up with all your route config up front again though won't you? Announcements has child routes too, requiring a round trip when you getRouteConfig there, right? how is this any different?

[taion]

You'd define that path as:

module.exports = {
  components: {
    sidebar: require('./components/Announcements'),
    main: require('./components/Sidebar'),
  },
  childRoutes: [{
    path: ':announcementId',
    getRouteConfig: (location, cb) => require.ensure([], require => {
      cb(null, require('./routes/Grades'))
    }),
  }],
};

In other words, this route stub by itself has enough data that the matcher can tell that it won't need to trace down any further child routes, assuming the path is /course/0/announcements.

Assuming the path were /course/0/announcements/0 or something, then yes, it would need to make an additional round-trip (for a total of 2), but as currently set up, you'd actually need 2 additional round trips (for a total of 4).

[ryanflorence]

So half the route config is in the parent. I don't like it. If you want to save round trips, define child routes synchronously, the user is in complete control of that right now.

[taion]

I don't think that's the right way to look at it. You already have to list out all of the child routes in the parent route object anyway, synchronous or asynchronous.

The only difference is that for the purposes of optimizing network requests, it's a lot better to also specify just the path to the child routes in the parent, instead of just some pointers to the route object. Imagine if we configured child routes as an object (to go full trie) instead of as a list. It'd be totally natural.

Either way, there's no way to correctly use getChildRoutes for async routes – either you don't use async components and you end up way overfetching, or you do use async components and you end up making two server round trips just to load a new route.

[taion]

The most drastic but possibly best solution is to just kill getChildRoutes and getIndexRoute entirely in favor of just using getComponent(s).

Using only the latter will absolutely prevent request waterfalls; no matter how well you set things up with getChildRoutes, you'll still potentially waterfall on nested split routes.

The benefit of using code splitting for your routes per se (as opposed to just putting them in different modules, which is very possible right now) is pretty minimal from a size perspective.

[taion]

The above is my original preferred solution. The proposal in OP was supposed to not change the API quite so much, but the more I think about it, the more I think that route-based splitting right now is unavoidable broken because of the unavoidable waterfall problem.

[ryanflorence]

The most drastic but possibly best solution is to just kill getChildRoutes and getIndexRoute entirely in favor of just using getComponent(s).

Exactly. The current API allows exactly that, so if you don't want waterfall route matching, don't use getChildRoutes.

No app should ever use getChildRoutes on every parent route, but it would make sense in a few, smart places to do it.

Nothing is "unavoidably broken". But there are, certainly, unavoidable tradeoffs. You either make the user download more route config up front, or you pick a few places you're okay with a little bit of waterfall requests.

[ryanflorence]

I expect an app with thousands of routes to do getChildRoutes in just a handful of "top-level app" routes.

[taion]

Can you elaborate on what you mean by "top-level app" route? Alternatively, what gets solved by getChildRoutes that doesn't get solved by getComponent per-route?

My concern here really is exposing an API that should generally not be used. Looking at the huge-apps example, it seems to be used quite extensively in the Course sub-route, and in such a way looks like it's just to the detriment of UX.

[taion]

For example, you wouldn't want to do:

const rootRoute = {
  getChildRoutes: (location, cb) => {
    require.ensure([], require => {
      cb(null, [
        require('./routes/SubApp1'),
        require('./routes/SubApp2'),
        require('./routes/SubApp3'),
      ]);
    }),
  },
};

You'd have to nest the async getChildRoutes into each of those sub-app route definitions.

This is quite unintuitive to me – even in the case where you want to split the route definition into sub-apps at the top level, you need to move the async handling one level down.

The natural way I think you'd want this kind of route-level splitting to work is that you only load the chunk for the route if you end up on the path for that route. The issue with the current API isn't that it's not possible to do that, it's that it's inconvenient, and that the "default" way of setting things up that looks like it will give you this in fact does not.

In other words, the API as set up right now, if used in an obvious way, leads to failure (defined as suboptimal behavior). That's why we need to rethink it.

[taion]

Let me summarize that a bit more.

For route level code splitting, you want to load the chunk for a route when you hit that route. If I hit /sub-app-1, I want to load the chunk for /sub-app-1.

With getChildRoutes, you instead load the chunks for all child routes, if you hit any child route. With the above, if I hit /sub-app-1, I load the chunks for /sub-app-1, /sub-app-2, and /sub-app-3.

[mjackson]

First of all, sorry for being late to the party here (and for the super long comment). I just got back yesterday from a long vacation overseas. Bad timing :/

I think the API we currently have for code-splitting is ok, but may not be perfect. It's honestly a bit tricky to understand how it all shakes out in really large deployments until you actually see them and look at the network requests being made. The huge-apps example is just a demo of the functionality working.

Yes, we're over-fetching route config, as @taion has demonstrated. This is due to the way we gradually match paths. Part of the requirement when I built this feature was that we wanted to make it possible for someone to add some route config to an app without changing the entry bundle. This is important for a few reasons:

• Facilitates caching of entry bundles
• Keeps entry bundle size to a minimum

With that in mind, let's see how webpack handles the huge-apps example situation in question here.

At /courses/0 webpack fetches the route config at routes/Course/index.js, which looks like this:

module.exports = {
  path: 'course/:courseId',

  getChildRoutes(location, cb) {
    require.ensure([], (require) => {
      cb(null, [
        require('./routes/Announcements'),
        require('./routes/Assignments'),
        require('./routes/Grades')
      ])
    })
  },

  // ...
}

Now, imagine someone wants to add a course/:courseId/exams route beneath course/:courseId. Using getChildRoutes, they would add an additional require statement inside the require.ensure, like this:

module.exports = {
  path: 'course/:courseId',

  getChildRoutes(location, cb) {
    require.ensure([], (require) => {
      cb(null, [
        require('./routes/Announcements'),
        require('./routes/Assignments'),
        require('./routes/Grades'),
        require('./routes/Exams') // new route config
      ])
    })
  },

  // ...
}

It may not be entirely obvious from this example unless you're familiar with webpack, but require.ensure defines a split point, so anything we do inside the callback could potentially be in a separate bundle. This means that if we ask people adding additional child routes to do it inside require.ensure we can preserve the original bundle. Now I can have people working at every level of my route hierarchy without having to deploy new entry bundles every time we ship a new child route. They just have to operate _inside_ the require.ensure callback.

Now, let's consider getRouteConfig. With this API, all path configs necessarily get pushed up to the parent. So our route config looks something like (copied the OP):

import Course from 'components/Course'

module.exports = {
  // path is defined upstream
  component: Course,
  childRoutes: [
    {
      path: 'announcements',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Announcements'))
      }),
    },
    {
      path: 'assignments',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Assignments'))
      }),
    },
    {
      path: 'grades',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Grades'))
      }),
    }
  ]
}

In this scenario, when I want to add another child route I would do it like this:

import Course from 'components/Course'

module.exports = {
  // path is defined upstream
  component: Course,
  childRoutes: [
    {
      path: 'announcements',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Announcements'))
      }),
    },
    {
      path: 'assignments',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Assignments'))
      }),
    },
    {
      path: 'grades',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Grades'))
      }),
    },

    // new route! but we had to add more config to the parent bundle...
    {
      path: 'exams',
      getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Exams'))
      }),
    }
  ]
}

In this situation we have to add more code to the parent route's config, which means the parent's bundle changes, which we want to avoid.

In the end I think it comes down to where we want to take the hit. We chose to optimize for people who are shipping new features and helping them to preserve their entry bundle sizes instead of optimizing for users who may end up over-fetching some route config they may not need.

I think that a little over-fetching of route config isn't a terrible thing. Most people are probably going to use the router to do code-splitting at the top-level routes in their app, which is probably where their navigation lives. Chances are, you're going to visit some sibling routes. e.g. I haven't even mentioned the fact that with getChildRoutes you don't need to make an additional request for route config when you visit /courses/0/assignments. You only take that hit once when you hit the sibling route.

[taion]

Thanks for your comments here. I think there are a few oversights here.

• I don't think your assumption that the entry bundle can be cached holds in the general case. While it's true that the entry bundle can be cached in the example you give, this is likely to be quite brittle in practice. The use of any optimization plugins like LimitChunkCountPlugin, MinChunkSizePlugin, and OccurenceOrderPlugin will lead to chunk ids changing in a potentially unpredictable way, with that chunk change leading to an invalidation of the entry chunk even if nothing changed.
• As set up right now, to limit complete overfetching, the subroutes under routes/Course all use async getComponent, so even when hitting a sibling route, you still need to make a new server round trip for the component – in fact, when hitting the first child route, you make two dependent round trips: one for the routes, then a second for the component.
• With an API like getRouteConfig, you'd actually move the code-splitting to the top level, versus having it on the second level right now, i.e. you'd put it in huge-apps/app.js instead of in huge-apps/routes/Course. This would give you all the benefits you mention above, and more. The entry chunk would be even smaller than the status quo, as it wouldn't include the bodies of the child routes, and it still would not change unless someone were to add a top-level child route. It wouldn't make a lot of sense to code split with getChildRoutes at the top level, but it would work to code split with top-level route stubs that call into getRouteConfig.

[taion]

In that sense, my example in the OP is not how I'd ultimately use this API. With getRouteConfig, you'd actually modify app.js to define:

const rootRoute = {
  component: 'div',
  childRoutes: [ {
    path: '/',
    component: require('./components/App'),
    childRoutes: [
      {
        path: 'calendar',
        getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Calendar'))
        })
      },
      {
        path: 'courses/:courseId',
        getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Course'))
        })
      },
      {
        path: 'grades',
        getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Grades'))
        })
      },
      {
        path: 'messages',
        getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Messages'))
        })
      },
      {
        path: 'profile',
        getRouteConfig: (location, cb) => require.ensure([], require => {
          cb(null, require('./routes/Profile'))
        })
      }
    ]
  } ]
}

instead of what is currently there. As part of this, you'd then remove the async getComponent calls in the root route definitions to each of those included top-level child routes.

This leads to a smaller entry chunk because it only includes the stubs for top-level children instead of the shallow route definition, and also preserves the pre-optimization contents of the entry chunk against any changes in the top-level child chunks, such as in routes/Course.

[mjackson]

@taion re: the points you made here:

• Occurrence order is on by default in webpack 2. It seems like they've noticed that optimization plugins have the tendency to break hashes and are trying to mitigate the problem. In any case, our job is only to make it possible to not change any code outside the split point.
• Yes, that's right. We make two requests in that example ... but it's only an example! I wouldn't expect anyone to take it to that extreme. huge-apps is divided into way more chunks than it actually needs. In practice, I wouldn't expect people to configure things like that. We just give them the hooks so they can decide where to put the split points.

it still would not change unless someone were to add a top-level child route

Yes, that's what I mean. That's the case we're optimizing for: people adding new routes.

[timdorr]

Maybe it's just me, but getRouteConfig looks an awful lot like getComponent(s)...

[taion]

Regarding getRouteConfig looking like getComponent, that's intentional.

One of the warts around React Router's route matching implementation is that async child routes are in some sense special. Consider e.g. getIndexRoute in matchRoutes – this will check the sync index route, the async index route, and sync child routes, but not any async child routes.

This has potential for confusion – I recall someone wondering why index routes nested under pathless routes didn't work for async child routes. In addition to what I've mentioned above, things end up being a lot more consistent if you have synchronous access to the stubs of the child routes, and per my example above, even in the huge-apps example case, this comes at no cost (actually negative cost) to the size of the entry chunk.

[timdorr]

So we have an example, can we see what's going to be on the other side, inside of something like ./routes/Calendar?

(PS sorry for being pedantic. I just think it will be helpful to have the full picture)

[mjackson]

I recall someone wondering why index routes nested under pathless routes didn't work for async child routes

I think there are some subtle bugs with the way routes work if they are loaded asynchronously; something about how we set them up once they're loaded. I've been meaning to look into it for a while now.

[taion]

In the above case, instead of having

module.exports = {
  path: 'calendar',
  getComponent(location, cb) {
    require.ensure([], (require) => {
      cb(null, require('./components/Calendar'))
    })
  }
}

It could just be

module.exports = {
  component: require('./components/Calendar')
}

No need to use getComponent here, since you've already done the code splitting one level up.

ETA: Fixed typo.

[taion]

The index route handling there is not a bug – it's to avoid traversing down all async child routes just in case some of them happened to be pathless routes with an index route under them. It has to do this as a pragmatic tradeoff – the current API doesn't allow the matcher to know what the paths are until the chunk is loaded.

[timdorr]

module.exports = {
  path: 'calendar',
  getComponent(location, cb) {
    require.ensure([], (require) => {
      cb(null, require('./components/Calendar'))
    })
  }
}

In that case, couldn't you just have this instead?

const rootRoute = {
  component: 'div',
  childRoutes: [ {
    path: '/',
    component: require('./components/App'),
    childRoutes: [
      {
        path: 'calendar',
        getComponent: (location, cb) => require.ensure([], require => {
          cb(null, require('./components/Calendar'))
        })
      } // And so on....

routeConfig.path isn't going to be used anyways. If you need to put modules higher up in the chunk tree, just use require.include. If you need them lower, use getChildRoutes.

I'm not seeing a compelling case yet, but that might be a matter of the examples provided thus far. Keep them coming, as they're useful to the discussion!

[taion]

For calendar, yes. The route that looks different is courses/:courseId, as this lets you redefine the child routes for that route without modifying the entry chunk.

[mjackson]

Combining getChildRoutes and getComponent into a getRouteConfig method puts restrictions on the way people do async code loading. Instead of having two hooks, they now have one. It takes away the decision of what kinds of things people might want to do asynchronously and makes assumptions instead. I think it's too early for us to make those kinds of assumptions.

I'm starting to think this whole discussion is because huge-apps is an extreme example of asynchronously loading everything–both route config _and_ components. At any point in that example you can swap out a getChildRoutes call for a childRoutes property or a getComponent call for a component property and make things sync.

Why take away people's ability to choose?

[taion]

getComponent and getComponents should stay – my proposal in the OP was incorrect, and I am editing it appropriately.

I agree that there should be both route-based splitting and component-based splitting. Component-based splitting allows parallel chunk loading, so it will always offer options that route-based splitting doesn't.

The reason I think getRoute (new, improved, shorter name!) is better than getChildRoutes is because it's more intuitive to split at the level of a particular child route, rather than at the level of all child routes. The idea is that if you go from / to /big-sub-app-1, then that's the most natural place to load a new chunk.

[taion]

More accurate statement. The huge-apps example is mostly correct. The problem is that it's confusing, and I blame this on the API.

It's confusing because it defines the split point for routes/Course at getComponent and getChildRoutes on routes/Course/index.js.

The actual goal here is to split out the details of the "course" route. The clearest way to do this would be to put the split point in app.js, like in https://github.com/reactjs/react-router/issues/3232#issuecomment-209170666. The current API doesn't make that possible.

[taion]

Goal: /courses/:courseId lives in a separate chunk, loaded on demand

Current implementation:

In app.js:

const rootRoute = {
  // ...
  childRoutes: [
    // ...
    require('./routes/Course'),
    // ...
  ],
  // ...
};

In routes/Course/index.js:

module.exports = {
  path: 'courses/:courseId',

  getChildRoutes: (location, cb) => require.ensure([], require => {
    cb(null, [
      require('./routes/Announcements'),
      require('./routes/Assignments'),
      require('./routes/Grades'),
    ]),
  }),

  getComponent: (location, cb) => require.ensure([], require => {
    cb(null, require('./components/Course'));
  },
};

Proposal:

In app.js:

const rootRoute = {
  // ...
  childRoutes: [
    // ...
    {
      path: 'courses/:courseId',
      getRoute: (location, cb) => require.ensure([], require => {
        cb(null, require('./routes/Course');
      }),
    },
    // ...
  ],
  // ...
};

In routes/Course/index.js:

module.exports = {
  childRoutes: [
    require('./routes/Announcements'),
    require('./routes/Assignments'),
    require('./routes/Grades'),
  ],

  component: require('./components/Course'),
};

They do almost the same thing, but I think the latter leads to less head-scratching.

[mjackson]

I don't think it's possible for us to say which approach is categorically _better_ (less-confusing, more efficient, whatever your measuring stick is) right now. As with everything, there are trade-offs.

For the record, I'm not 100% sold on getChildRoutes. We introduced it for the sake of very large sites, but I'm not aware of any large customers that are actually using it right now. I've come very close to nuking it a few times. However, it _does_ have another very interesting use case that I was trying to describe to you the other day, @taion, on Discord. It's the use case from #3098 that you keep bringing up.

Let's say that I'm building GitHub and I want to show a 404 page on private repos when you're not logged in. So, e.g. at /mjackson/secret-repo I want to show you a 404 unless you're logged in as mjackson. Using getChildRoutes, I could do something like this:

getChildRoutes(location, callback) {
  // We have location.state, so we can probably read session info.
  getCurrentUserAndRepo(location, (user, repo) => {
    if (user == null || repo.isPrivate && user.username !== repo.owner.username) {
      callback(null, NotFoundRoute)
    } else {
      callback(null, RepoRoute)
    }
  })
}

which I actually think is pretty cool. I should probably write this up in an example because, although I personally would use a redirect to another URL, sometimes people want to do stuff like this and we should probably support it.

Anyway, that's another potential use case for getChildRoutes. In any case I think that having two async APIs could be confusing. I'd prefer to just settle on one.

[taion]

Right, it's important not to get too caught-up here. This is a long-term roadmap item involving breaking changes to an advanced API. Also, most users should probably use getComponent anyway.

---

That said, your proposal for 404ing with getChildRoutes doesn't work in the general case. Suppose my route definition looks like:

<Route path=":org" component={Org}>
  <Route path=":repo" component={Repo} />
</Route>

This will normally render <Org><Repo /></Org>. Suppose I want it to render <NotFound /> when the repo is private.

As it stands, the logic can't go in the route for :repo, because at this point you've already matched :org, so you can't really avoid rendering <Org>.

You could put this in getChildRoutes on :org, but this has two problems.

• This is fine for a single :repo descendant route, but as the number of child routes increases, this becomes cumbersome and unmanageable – you end up centralizing all dynamic 404 logic for all descendants onto the ancestor route
• At the time you are evaluating getChildRoutes for :org, the matcher hasn't consumed the :repo segment yet, and as such, you don't have the value for params.repo – you'd need to parse the URL yourself

[taion]

That's why https://github.com/reactjs/react-router/pull/3098 is required – the only scalable way to bail out is on the level of the child route, _after_ finishing running pattern matching on that child route so that child route's params are available.

[ryanflorence]

The path is probably the most important part of the route definition, moving it to the parent is just not even computing in my head. What is a route if it doesn't even own it's own path?

[taion]

Certainly – this is still a proposal, and not something I plan to start any implementation work on for a bit.

The benefit is that while you've moved parts of the route config outside of the module, you've also gotten rid of unnecessary async loads inside the module, instead consolidating them in a single place one level up.

The mental model here is to have something like "sub-apps" mounted at certain points in the route tree, which have route definitions that only get loaded as needed.

[taurose]

Part of the requirement when I built this feature was that we wanted to make it possible for someone to add some route config to an app without changing the entry bundle.
...
It may not be entirely obvious from this example unless you're familiar with webpack, but require.ensure defines a split point, so anything we do inside the callback could potentially be in a separate bundle. This means that if we ask people adding additional child routes to do it inside require.ensure we can preserve the original bundle.

I don't think that's the case. Code inside require.ensure will not be moved outside the chunk, see example.js output in this example. Therefore, adding to getChildRoutes will break caching.

Instead you'd have to put the routes in another file to move them to the other chunk:

getChildRoutes: (location, cb) => require.ensure([], require => {
    cb(null, require('./foo/routes.js')(location));
}),

// foo/routes.js
export default location => [
      require('./routes/Announcements'),
      require('./routes/Assignments'),
      require('./routes/Grades'),
];

(while also considering the caveat described here)

Overall, it seems to me getRouteConfig is more flexible/powerful, as you can simulate getChildRoutes with it:

childRoutes: [{ 
  path: '',
  getRouteConfig: (location, cb) => require.ensure([], require => {
    cb(null, {
      childRoutes: [
        require('./routes/Announcements'),
        require('./routes/Assignments'),
        require('./routes/Grades'),
      ],
    });
  }),
}]

The same isn't true the other way around though.

I think in order to achieve both "no overfetching" and long-term caching (the way @mjackson described) you're forced to introduce another async chunk.

childRoutes: [{ 
  path: '',
  getRouteConfig: (location, cb) => require.ensure([], require => {
    cb(null, require('./foo/routes')(location));
  }),
}]

// foo/routes.js
export default location => require.ensure([], require => ({
 childRoutes: [
    require('./routes/Announcements'),
    require('./routes/Assignments'),
    require('./routes/Grades'),
  ],
}));

Personally, I don't find getRouteConfig much less intuitive or maintainable at first glance. With this problem of overfetching, however, getChildRoutes seems kinda deceptive.

[taion]

I think the issue of deceptiveness is the biggest problem with getChildRoutes.

Naïvely, you'd probably just go about and split your code by putting an async getChildRoutes at top level, because that's the most straightforward thing to do. This would split your code, but it'd put _every child route_ in one chunk, which I don't think is what people generally want to do.

That's the core of my rationale for wanting to get rid of getChildRoutes (eventually).

[mjackson]

Let's also please keep in mind that this is a feature that nobody is
actually using, AFAICT. So nothing is sacred here. It's a cool demo, but we
may need to build some real stuff with it before we understand all the
implications.
On Wed, Apr 13, 2016 at 8:11 AM Jimmy Jia [email protected] wrote:

I think the issue of deceptiveness is the biggest problem with
getChildRoutes.

Naïvely, you'd probably just go about and split your code by putting an
async getChildRoutes at top level, because that's the most
straightforward thing to do. This would split your code, but it'd put _every
child route_ in one chunk, which I don't think is what people generally
want to do.

That's the core of my rationale for wanting to get rid of getChildRoutes
(eventually).

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/reactjs/react-router/issues/3232#issuecomment-209499574

[timdorr]

Copying in some comments from @taion that ended up in the wrong place:

@taion

We should consider adding a "far future" label for issues like this that aren't going to be actionable for a while.

@timdorr

Shouldn't that be what the-future is for now? It's an icebox of sorts. If things come into the release schedule for a specific version, we can use milestones.

@taion

I think we should have something with adding a require.ensure shim so that webpack users can send down the chunks required for the initial render without going incurring extra round trips on the client side.

[timdorr]

I'm not sure if we should be doing anything specific to webpack. What about users that want to use System.load/import once that API comes to fruition? Shimming require.ensure to enable chunk tracking (how do you do this exactly, btw?) is really out of scope of Router.

Does webpack have any API to query which chunks have been loaded thus far? One could simply serialize that data to load up in the client before proceeding to render.

This (and #3290) is all eerily close to the problems facing redux-router and issues on react-router-redux regarding serializing router state. I'm only half-thinking about this at the moment (gotta get back to work...), but it seems like there's a tangentially related solution for all of this.

[taion]

It can't be part of core. It'd just be nice to have a pattern/recipe for common use cases.

[mwhite]

Here's some magic (ah, magic...), that if it works, would allow the user to optionally define coupling between the router and the module loader in order to enable a declarative-first API for 99% of the use-cases while still enabling all desired capabilities:

/// asyncLoadOrPassthrough.js
export default (routerModuleURL, debug, loadModule) => {
  if (debug) {
    return (path) => {
      return (cb) => {
        loadModule(path, cb);
      };
    }
  } else {
    return (path) => path;
  }
};

/// requireSystemRoute.js
import asyncLoadOrPassthrough from './asyncLoadOrPassthrough';
export default (routerModuleUrl, debug) => {
  return asyncLoadOrPassthrough(routerModuleUrl, debug, (path, cb) => {
    System.import(path).then(cb);
  });
}

/// requireAmdRoute.js
import asyncLoadOrPassthrough from './asyncLoadOrPassthrough';
export default (routerModuleUrl, debug) => {
  return asyncLoadOrPassthrough(routerModuleUrl, debug, (path, cb) => {
    require([path], (module) => {
      cb(module);
    });
  });
}

/// main.js
import requireSystemRoute from './requireSystemRoute';

// Is __dirname available in all environments or able to be calculated?
export const require = requireSystemRoute('/', DEBUG);

// Bundlers should see require calls as normal requires 
// if you want to bundle into a single file by default without any extra configuration.
//
// The internals of the router would need to be modified in two ways to enable this API:
// When a route is matched, if its component or childRoutes are path strings, 
// call Router.require(path) to get a function.  
// If they are functions, call them asynchronously as with the existing API to get an object, 
// but passing along information about the base module URL of the module
// containing the current route, so that...
// ... the base URL of every route can be set when it is first added to the configuration,
// enabling relative module paths returned from that route's getChildRoutes() or getComponents()
// to be correctly resolved.
<Router require={require}>
  <Route component={require('./components/App')}>
     <Route 
       component={require('./components/Assignments')}
       childRoutes={[
          require('./routes/CurrentAssignments')
        ]}
     />
  </Route>
</Router>

// routes/CurrentAssignments.js
import {require} from '../main';

export default {
  // You can also specify 'component' as a value instead of function but only directly,
  // not using require, since handling require requires module path awareness passed at runtime
  getComponent(location, cb) {
    cb([
      require('../components/CurrentAssignments')
    ]);
  },
  getChildRoutes(location, cb) {
    cb([
       require('./LateAssignments')
    ]);
  }
}

(For the passthrough case, if you change it to return both the path and loadModule callback, it's possible to eliminate the need for <Router require>.)

[mwhite]

I would argue that if this would work, it would be worth adding some coupling between the router and the module loader in order to expose a simple API even in the case of code splitting. It seems reasonable that the right API for code splitting would involve the router knowing about the module loader.

This would make the code you write the same regardless of how you bundle your modules, which is how it should be.

Another property this solution has which none of the others for code splitting do is that, for the simple case not involving location-dependent child routes, it enables using a single configuration flag to switch between loading each component on demand and loading all components at app load time, without having to modify the router to traverse and maintain the tree of location-dependent child routes. This is useful to be able to always load the entire app to verify that a change in a shared dependency didn't break components other than the one you're currently working on. E.g. automated testing can verify static validity of the entire app with debug = true just by loading the router, while with the existing code splitting approach, it requires the user to manually hit every route.

Also, for the most common case of wanting code splitting of routes and components while keeping all routes in a single file, getComponent() makes the JSX significantly noisier.

Unfortunately this approach is not idiomatic for ES6, but if ES6 bundlers don't pick up the requires you can always manually create a separate file importing all components. This points at the need for a deferred import in ES6.

[mwhite]

I'm pretty sure the following API would be possible to implement while providing all desired functionality, particularly code splitting. I would suggest that if no one is using dynamic (as opposed to incidentally asynchronous) child routes then don't include it in the router API.

This would require two small changes to the router:

• recursively track the module path of each route, starting from the Router moduleBasePath, so requires for child components and routes can be resolved correctly when a route is matched
• if a route or component is a string, treat it as a module path, prepend the module path of the parent route to get the full module path, and call Router loadModule to get an object.

// /loadModule.js
// Default implementation
const loadModule = (path, cb) => {
  if (typeof System !== 'undefined') {
    System.import(path).then(cb);
  } else if (typeof require !== 'undefined') {
    require([path], (module) => {
      cb(module);
    });
  } else {
    throw new Error();
  }
};

// react-router/require.js
export default (path) => path

// /main.js
import loadModule from './loadModule';
import {Router, Route, require}  from 'react-router';

<Router 
  moduleBasePath='/'
  loadRoutesOnDemand={false}
  loadComponentsOnDemand={!DEBUG}
  loadModule={loadModule}
>
  <Route childRoutes={[
    require('./routes/Assignments')
  ]} />
</Router>

// /routes/Assignments.js
import {require} from 'react-router';

export default {
  component: require('../components/Assignments')
}

// bundler config:
// bundle 1: module main, exclude main, exclude routes/
// bundle 2: include main, include routes/

// or:
// bundle 1..n - 1: include explicit component modules
// bundle n - 1: module main, exclude main, exclude routes/
// bundle n: include main, include routes/

If ES6 had deferred imports:

import {Router, Route}  from 'react-router';
defer import Assignments from './routes/Assignments';

<Router
  loadRoutesOnDemand={false}
  loadComponentsOnDemand={!DEBUG}
>
  <Route childRoutes={() => [
    Assignments
  ]} />
</Router>

[taion]

I think that handling would better live in userspace.

I have cases where I just put a branch in e.g. getComponent to decide which component to return based on something like nextState.location.query. I feel like this is a useful feature to have.

Likewise for child routes, though it's harder to justify there.

[mwhite]

Do you actually need the asynchronousness though?

{
  component: ConditionalRender(() => {
    return this.state.foo ? FooComponent : BarComponent;
  })
}

(The one thing that wouldn't support is code splitting below the granularity of routes with conditional components.)

If React doesn't support asynchronous rendering, why should react router?

[timdorr]

You need to resolve all async actions before you first render when coming from the server, otherwise the server-provided HTML won't match the client-side DOM on first render and React will complain.

[iotch]

My apologies for intruding on your discussion, but from my experience with our website current "get*" implementation lacks the ability to get the whole route config for the given path.

For example, if we have a website with separate sections, like, 'blog', 'shop', 'personal' and each of this sections is a separate app.
Currently, if we want to load a section on demand, we would need to create two bundles (say, using Webpack 2) - first one for the section's root _component_, and the second - for the section _child routes_:

{
    // ...
    childRoutes: [{
        path: 'blog',
        getComponent: function(nextState, callback) {
            // creates new chunk 'blog-section.js'
            require.ensure([], (require) => {
                callback(null, require('sections/Blog'));
            }, 'blog-section');
        },
        getChildRoutes: function(location, callback) {
            // creates new chunk 'blog-child-routes.js'
            require.ensure([], (require) => {
                callback(null, require('sections/blogChildRoutes'));
            }, 'blog-child-routes');
        }
    }]
}

The problem is that we have to create two bundles for the 'blog' app, because getComponent() expects the _component_, and getChildRoutes() expects _routes definition_. We could load the whole 'blog' bundle inside the getComponent(), but then we have to cache it somehow and pass the children routes definitions to getChildRoutes() when it gets called.

Instead, will be great to have a getRoute() method, to fetch the whole section route with children in one request. And have all the components, and more important, logic (like onEnter() , etc), in a separate 'blog' bundle. Like:

{
    // ...
    childRoutes: [{
        path: 'blog',
        getRoutes: function(nextState, callback) {
            // creates a single chunk 'blog-section-bundle.js'
            require.ensure([], (require) => {
                callback(null, require('sections/Blog'));
            }, 'blog-section-bundle');
        }
    }]
}

//sections/Blog/index.js - the whole section bundle
module.exports = {
    component: require('./Home'),
    onEnter: function() {
        // do something here, inside the current 'blog' bundle, not level above
    }
    childRoutes: [
        {
            path: 'categories',
            component: require('./Categories'),
        },
        {
            path: 'tags',
            component: require('./Tags'),
        }
        // ... and so on
    ]
}

In other words, if there is a method to get a child routes definitions, there should be a method to get a parent route definition, not only the component itself.
getComponent() and getComponents() for components and getRoute() and getChildRoutes() for route definitions, IMO

[taion]

@iotch This is a long thread, but that's exactly the proposal here. See https://github.com/reactjs/react-router/issues/3232#issuecomment-209193985.

[iotch]

@taion, yes, I just tried to support your proposal with another example.

[taion]

Ah, gotcha. Thanks!

[sethkinast]

The app I'm working on currently definitely fits the "large app" scenario-- it has half a dozen huge apps mounted that are each worked on by different teams, etc., similar to what @iotch described.

What I'm currently doing to avoid the problem he mentions of different bundles for the route's component and child routes is to use a pathless route around each "app", as @taion alludes to early in this issue:

export default {
  path: "/",
  component: App,
  indexRoute: {
    getComponent(location, cb) {
      require.ensure([], require => cb(null, require('app-one').default));
    }
  },
  childRoutes: [
    require('app-two').default,
    require('app-three').default
  ]
};

And then AppTwo looks something like

export default {
  getChildRoutes(location, cb) {
    require.ensure([], require => cb(null, [{
      path: "app-two",
      component: require('app-two/app').default,
      indexRoute: {
        component: require('app-two/index').default
      },
      childRoutes: [
        {
          path: "results",
          component: require('app-two/results').default
        }
      ]
    }]));
  }
};

This gives me nice chunk-per-app bundling, but it's definitely very wordy and would be improved with something like a getRoute for sure.

And, from what @taion said earlier I suspect that this means I couldn't load some routes inside an app async, so this approach doesn't scale if I want to chunk at a more granular level.

[taion]

@sethkinast I don't think that does what you want. You have the split out code after the split, so the router had to walk through each possible set of child routes in turn until it finds the one that matches.

[sethkinast]

Ah yeah I simplified / anonymized the code for the sake of example and oversimplified :P makes for an even better argument that the current state of the API is not great.

[taion]

Yup, my point exactly – it's an easy API to mis-use.

That said, this is still some ways away – it's inconvenient to split out sub-apps right now, but still possible.

[mmahalwy]

I am facing a problem currently with an isomorphic app where React throws my server-side rendering does not match my client side. More details:

1. Server renders markup
2. Client kicks in to rerender
3. Client does not have current route since it's a chunk, fetches that. During that time a <noscript /> is returned since the route is not present.
4. Javascript file is fetched and renders
5. React throws error and Router throws setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the Router component.

Thoughts?

[taion]

See the SSR guide or the troubleshooting FAQ.

[mmahalwy]

@taion my SSR works fine if i don't chunk that specific route.

[sethkinast]

For async routes you need to use match on the client too.

[taion]

Right, this specific use case is explicitly covered in those docs.

[mmahalwy]

+1 thanks!

[jedwards1211]

Have you guys considered allowing higher-order components to render routes?

This is something I have wanted for a long time that I think would simplify all of this, because by being able to return a from the HoC's render, you would be able to put all of your logic that picks the route config in there, and that logic would be able to access things like Redux state. For example:

import React, {Component} from 'react'
import {Router, Route} from 'react-router'
import {connect} from 'react-redux'
import ReactDOM from 'react-dom'

import store from './myStore'

import LoadingView from './LoadingView'
import CourseView from './CourseView'
import {loadRoutes} from './CourseView/actions'

class CourseRoute extends Component {
  componentWillMount() {
    this.props.dispatch(loadRoutes(...)) // middleware will put the new routes in redux state
    // (or things could be done a completely different way)
  }
  render() {
    return <Route component={CourseView}>
      {this.props.childRoutes || <IndexRoute component={LoadingView} />}
    </Route>
  }
}

function select(state) {
  return {
    childRoutes: state.getIn(...)
  }
}
const ReduxCourseRoute = connect(select)(CourseRoute)

ReactDOM.render(
  <Provider store={store} />
    <Router>
      <Route path="course/:courseId" delegate={ReduxCourseRoute} />
    </Router>
  </Provider>,
  document.getElementById('root')
)

This approach might need a delegate property separate from component so that Router knows it expects a Route to be returned from the component's render. In this case <Route> would actually have a functioning render method that determines what to render based upon the location and its props.

Note also how easy it would be for the Route delegate to render a loading banner while waiting for its child routes to load. I think if you imagine how this type of API would apply to challenging use cases, you'll find that it's extremely flexible.

[jedwards1211]

@mjackson I'm trying to refactor my app to use getChildRoutes for production, and I'm very enthusiastic about this feature. I've created some tools that allow me to put all the routes, components, reducers, and middleware for a given feature of my app into a code bundle that can be loaded into Redux state:
https://github.com/jcoreio/redux-plugins-immutable
https://github.com/jcoreio/redux-plugins-immutable-react
https://github.com/jcoreio/redux-plugins-immutable-react-router
(unfortunately a lot of docs for my tools are still lacking)

Grouping routes, components, reducers, and middleware together in a feature-oriented programming framework like this can really simplify large application development, and especially prevent separate teams working on separate features from stepping on each others' toes.

Note that it's kind of crazy what I do in redux-plugins-immutable-react-router: I deep-clone the route-tree and inject implementations of getChildRoutes, getIndexRoute, getComponent that get those out of my redux state based on some patterns that I've established. For instance, a <Route> with childRoutesKey will get an injected getChildRoutes that gets as child routes (roughly speaking)

store.getState().get('plugins')
  .map(plugin => plugin.getIn('routes', childRoutesKey))
  .flatten()
  .map(decorateRoutes)

My plugins look like the following:

const ConfigViewPlugin = Immutable.fromJS({
  key: PLUGIN_KEY,
  name: PLUGIN_NAME,
  components: {
    AppSettingsMenuItems: (): React.Element => <NavLink to={CONFIG_PATH}>Configuration</NavLink>
  },
  routes: Immutable.Map({
    Root: (
      <Route path={CONFIG_PATH} componentKey="ConfigView" pluginKey={PLUGIN_KEY}>
        <Route path=":machineId" component={DrilldownRoute} childRoutesKey="MachineConfigView">
          <IndexRoute componentKey="MachineConfigView" pluginKey={PLUGIN_KEY} />
        </Route>
      </Route>
    )
  }),
  load(store: Store, callback: (err: ?Error, plugin?: Immutable.Map) => any): void {
    require.ensure([
      './ConfigView.jsx',
      './MachineConfigView.jsx'
    ], require => callback(undefined, ConfigViewPlugin.mergeDeep({
      components: {
        ConfigView: require('./ConfigView.jsx').default,
        MachineConfigView: require('./MachineConfigView.jsx').default
      },
      reducer: require('./configViewReducer').default
    })))
  }
})

const getChildRoutesFromStore = (location, store, cb) => {
  ...
}

const DataPluginsUIPlugin = Immutable.fromJS({
  key: PLUGIN_KEY,
  name: PLUGIN_NAME,
  components: {
    MachineConfigViewContent: (props: Object) =>
      <LoadPluginComponent pluginKey={PLUGIN_KEY} componentKey="DataPluginsView" componentProps={props} />
  },
  routes: Immutable.Map({
    MachineConfigView: <Route path="dataPlugins" getChildRoutesFromStore={getDataPluginRoutes} />
  }),
  load(store: Store, callback: (err: ?Error, plugin?: Immutable.Map) => any): void {
    require.ensure([
      './DataPluginsView.jsx',
      './AddPluginRoute.jsx',
      './dataPluginsViewReducer'
    ], require => callback(undefined, DataPluginsUIPlugin.mergeDeep({
      components: {
        DataPluginsView:  require('./DataPluginsView.jsx').default,
        AddPluginRoute:   require('./AddPluginRoute.jsx').default
      },
      reducer: require('./dataPluginsViewReducer').default
    })))
  }
})

[ericclemmons]

Jumping in here to say that our production apps use getChildRoutes extensively.

[insin]

The proposal for getRoute() above is exactly what an app I'm currently working on needs - it's effectively a bunch of different tools with different target users combined in the same app, and we want a single extra bundle to cover entire subapp paths.

This is a bit like plugging in extra app URL config in Django, and is also where named routes come in handly - a sub-app shouldn't need to know or care about the path it's been mounted at.

[jedwards1211]

@insin you could look at my redux-plugins-immutable framework and its related libraries like redux-plugins-immutable-react-router, I created them to put all code for a feature (reducers, middleware, react comps, and routes) together in a single bundle. Although of course the root route for a given feature can't be async-loaded as there must be something to trigger the loading of the bundle to begin with. There's barely any documentation right now though.

[jedwards1211]

Typically for smaller apps I just _push the async loading out of the route and into a component_ with react-router-loader. For example:

import Course from 'components/Course'

module.exports = {
  // path is defined upstream
  component: Course,
  childRoutes: [
    {
      path: 'announcements',
      component: require('react-router!./routes/Announcements')).default
    },
    {
      path: 'assignments',
      component: require('react-router!./routes/Assignments')).default
    },
    {
      path: 'grades',
      component: require('react-router!./routes/Grades')).default
    }
  ]
}

This is easy to use correctly and the react-router-loader component doesn't load the requested module until it mounts.
This doesn't work for getChildRoutes or getIndexRoute though.

[jedwards1211]

My point is if it were possible to have HoC <Route>s it might not even be necessary to have any async load handling in react-router itself.

[taion]

They're not semantically identical. If you do things that way, you have a hook to render a loading component. getComponent currently doesn't offer you one, but unlike react-router-proxy won't waterfall your loads for no good reason.

[jedwards1211]

I know they're not identical...but what do you mean by waterfall your loads?

[taion]

getComponent loads will happen in parallel. react-router-loader loads will not.

[jedwards1211]

Huh? react-router-loader just returns a component that async-loads the required module when it mounts, so if several react-router-loader components mount at the same time they should load the required modules in parallel, right?

[taion]

Which means that if you have multiple of those nested, the child components don't mount and thus don't start loading until after their parents have _finished_ loading.

[jedwards1211]

Are you talking about how something like this would behave with react-router 2.0.0?

<Route path="account" component="react-router!./AccountShell.js">
   <Route path="profile" component="react-router!./Profile.js" />
</Route>

Because if I understand correctly react-router 2.0.0 at /account/profile would mount the loader comps for the parent and child route simultaneously and they would load the proxied comps simulatneously.

Or are you talking about if a HoC Route component for /account had control over whether or not to render the /profile child? Because I see how then it would definitely waterfall load.

[taion]

You're misunderstanding how React lifecycle hooks work. A child component doesn't mount until its parent renders it.

[jedwards1211]

Sorry, actually, what I had forgotten about was that the react-router component won't render the children passed to it while it's loading the desired module. However, it would be possible to make a variation that invisibly renders them before the desired module finishes loading.

[jedwards1211]

But, that approach would probably have problems, so I see your point.

[WangLarry]

my solution: put 'getComponent' into a Higher Order Function:

function resolveAdminComponent(subpage = null) {
    return (nextState, cb) => {
      require.ensure([], function(require) {
        const {index: indexPage, subIndex: subPages} = require('../../modules/Admin');
        cb(null, subpage ? subPages[subpage] : indexPage);
      }, 'admin')
    }
  }

router:

<Route path="admin" getComponent={resolveAdminComponent()}>
    <Route path="site" getComponent={resolveAdminComponent('site')}>
    </Route>
</Route>

It split code at entry of every top page.
for me, it is simple and enough.

[sandysaders]

Does react-router v4 address this?

[timdorr]

v4 doesn't address code splitting or async stuff at all. But this is intentional. You can do this outside of the router now: https://gist.github.com/acdlite/a68433004f9d6b4cbc83b5cc3990c194

As for this issue, we can still address this within in the 3.x branch, but it will likely come from the community via PRs.

[timdorr]

FWIW, I think the discussion has played out and the consensus is against this change right now. We can reopen if there is a swing in the other direction or a superior option becomes available.

Housekeeping this open issue for now.