-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Revisit code splitting API #3232
Comments
Question because I don't know enough about webpack's behaviors: Will the 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. |
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:
What does happen right now? First we make one round-trip that fetches:
Then we make a second round-trip that fetches:
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 You can work around this with pathless routes in 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. |
The takeaway is that, as set up, the code splitting API does not allow all of:
Moving instead to |
On a related note, does |
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. |
For sure-- my purpose in asking was to make sure a new API would work with
|
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 |
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 Assuming the path were |
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. |
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 |
The most drastic but possibly best solution is to just kill Using only the latter will absolutely prevent request waterfalls; no matter how well you set things up with 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. |
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. |
Exactly. The current API allows exactly that, so if you don't want waterfall route matching, don't use No app should ever use 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. |
I expect an app with thousands of routes to do |
Can you elaborate on what you mean by "top-level app" route? Alternatively, what gets solved by 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 |
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 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. |
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 With |
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:
With that in mind, let's see how webpack handles the huge-apps example situation in question here. At 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 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 Now, let's consider 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 |
Thanks for your comments here. I think there are a few oversights here.
|
In that sense, my example in the OP is not how I'd ultimately use this API. With 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 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 |
@taion re: the points you made here:
Yes, that's what I mean. That's the case we're optimizing for: people adding new routes. |
Maybe it's just me, but |
See the SSR guide or the troubleshooting FAQ. |
@taion my SSR works fine if i don't chunk that specific route. |
For async routes you need to use |
Right, this specific use case is explicitly covered in those docs. |
+1 thanks! |
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 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. |
@mjackson I'm trying to refactor my app to use 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 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
})))
}
}) |
Jumping in here to say that our production apps use getChildRoutes extensively. |
The proposal for 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. |
@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. |
Typically for smaller apps I just push the async loading out of the route and into a component with 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 |
My point is if it were possible to have HoC |
They're not semantically identical. If you do things that way, you have a hook to render a loading component. |
I know they're not identical...but what do you mean by waterfall your loads? |
|
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? |
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. |
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. |
You're misunderstanding how React lifecycle hooks work. A child component doesn't mount until its parent renders it. |
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. |
But, that approach would probably have problems, so I see your point. |
my solution: put 'getComponent' into a Higher Order Function:
router:
It split code at entry of every top page. |
Does react-router v4 address this? |
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. |
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. |
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 undercourse/:courseId
, we end up loading all the child routes.In practice, the impact is somewhat mitigated in the example by also using
require.ensure
forgetComponent
, 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
,) with a singlegetComponent
,getComponents
getRouteConfig
(or better name). Instead of the above route configuration, we'd configure things as e.g.The text was updated successfully, but these errors were encountered: