Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get Route Handler Components Asynchronously #755

Closed
ryanflorence opened this issue Feb 2, 2015 · 25 comments
Closed

Get Route Handler Components Asynchronously #755

ryanflorence opened this issue Feb 2, 2015 · 25 comments
Labels

Comments

@ryanflorence
Copy link
Member

Right now a route config requires all of the routes to declare their route handle components up front, this is problematic for a couple reasons:

  1. Lazy loading
  2. Indirect circular deps with the router instance (very problematic in flux, router instance requires components which require actions which require the router instance)

While lazy loading is possible by creating thin handlers like in the partial-app-loading example, for cases like Relay's GraphQL queries, we'd still need to be requiring in the child components to get their queries so we're back to having the same problem.

My initial thought is to introduce getHandlerForRoute on the router instance. The default implementation would not require anybody to change their existing code but would give us the ability to let the developer provide a component lazily.

var router = Router.create({
  routes: routes,
  getHandlerForRoute (route, state, cb) {
    // this is the default implementation we use if not defined
    cb(route.handler);
  }
});

// if you're using webpack, route handlers could be lazy bundles
<Route handler={require('bundle?lazy./components/FriendsList')}/>
getHandlerForRoute (route, state, cb) {
  route.handler(cb);
}

// if you have some other loading mechanism, you could use strings
<Route handler="FriendsList" />
getHandlerForRoute (route, state, cb) {
  getComponentByString(route.handler, cb);
}

I like this a lot, but its simply a strawman to get the conversation going.

@ryanflorence
Copy link
Member Author

@geekyme
Copy link

geekyme commented Feb 2, 2015

Looks good and does it require any change of code on server side rendering? Here is a sample that I'm using in yahoo's fluxible architecture https://github.com/geekyme/isomorphic-react/blob/master/server.js#L58-L98

@ryanflorence
Copy link
Member Author

does it require any change of code on server side rendering?

All depends on your implementation, we just provide a function and a callback.

@gaearon
Copy link
Contributor

gaearon commented Feb 2, 2015

This is good. Yes.

What happens before the callback is ready? As you saw in another issue, we ran into a little trouble: if you want transition hooks to work, you can't have a loading UI, because to show the UI, you have to allow the transition.

IMO we need to both allow loading UI and let transition hooks work correctly.

@geekyme
Copy link

geekyme commented Feb 2, 2015

Indirect circular deps with the router instance (very problematic in flux, router instance requires components which require actions which require the router instance)

I didn't know how about that. Please advise. How come actions need the router instance?

On 2 Feb 2015, at 6:35 pm, Dan Abramov notifications@github.com wrote:

This is good. Yes.

What happens before the callback is ready? As you saw in another issue, we ran into a little trouble: if you want transition hooks to work, you can't have a loading UI, because to show the UI, you have to allow the transition.

IMO we need to both allow loading UI and let transition hooks work correctly.


Reply to this email directly or view it on GitHub.

@gaearon
Copy link
Contributor

gaearon commented Feb 2, 2015

I didn't know how about that. Please advise. How come actions need the router instance?

You might want to transitionTo, goBack or replaceWith from a Flux action.

@mjackson
Copy link
Member

mjackson commented Feb 2, 2015

we need to both allow loading UI and let transition hooks work correctly.

Agree. I've done a lot of thinking about this over the weekend. The nice thing about the model we currently have is that we already have a few well-defined points where you get to control 1) how the transition proceeds (the willTransitionTo hook) and 2) exactly when you render (the run callback). So it doesn't seem like we need any more hooks and/or methods. We probably just need to expose the existing functionality in a way that is more friendly for people who are dynamically loading UI.

To put it another way, we already know how to:

  • "Pause" the transition (keep the current UI) until we dynamically load something
  • Render some loading UI while we dynamically load something

Which one you do is up to you, according to your needs.

@gaearon
Copy link
Contributor

gaearon commented Feb 2, 2015

Can route handlers define a static property telling what to show while transition to it is pending?

var ShowNothingUntilImReady = React.createClass({
  statics: {
    willTransitionTo(blabla, callback) {
      setTimeout(callback, 3000);
    },
  }
});

var Spinner = React.createClass({
  render() { return <img src='loading.gif' /> }
});

var ShowSpinnerUntilImReady = React.createClass({
  statics: {
    pendingHandler: Spinner,

    willTransitionTo(blabla, callback) {
      setTimeout(callback, 3000);
    },
  }
});

Or should this be declared in route config instead?

<Route name='not-nice' handler={ShowNothingUntilImReady} />
<Route name='nice' handler={ShowSpinnerUntilImReady} pendingHandler={Spinner} />

How does this work with nesting?

@gaearon
Copy link
Contributor

gaearon commented Feb 2, 2015

<Route handler={App}>
  <Route name='nice' handler={ShowSpinnerUntilImReady} />
  <PendingRoute handler={Spinner} />
</Route>

Until ShowSpinnerUntilImReady calls willTransitionTo callback, App will get Spinner as <RouteHandler />. If App did not have PendingRoute inside it, then the transition would freeze it (like it does now).

This approach probably has problems with more nesting but it's something.

@tcoopman
Copy link

tcoopman commented Feb 2, 2015

Should it be the responsibility of the component to chose a spinner or of the router/root component?

So: component: I'm not done yet, please render this spinner till I'm done. vs component: I'm not done yet. Router: Ok, while waiting, I will render this spinner?

Both seem to have advantages.

@gaearon
Copy link
Contributor

gaearon commented Feb 2, 2015

I think the parent component (or parent's route config node) should decide that. Only parent knows what is appropriate to nest inside it at the current level of nesting.

@plasticine
Copy link

This is really exciting, and definitely solves a really tricky data/lazy loading use-case that I keep butting into. Awesome.

@plasticine
Copy link

Until ShowSpinnerUntilImReady calls willTransitionTo callback, App will get Spinner as <RouteHandler />. If App did not have PendingRoute inside it, then the transition would freeze it (like it does now).

I really like this.

This would also allow for managing any initial data load that may also occur after the initial lazy-load of the handler to happen behind the same spinner.

@ryanflorence
Copy link
Member Author

@gaearon I know its related, but can we move the discussion around "loading states" to a new thread? We can talk about in the context of waiting on a transition already, I'd like to try to keep this thread dedicated to a solid API for async component loading, and then we can reference this in the other thread.

@nickdima
Copy link

nickdima commented Feb 3, 2015

@ryanflorence your proposal looks good to me. I bumped into this problem just yesterday and I was looking for exactly this implementation.

@jsdf
Copy link

jsdf commented Feb 4, 2015

I think this could be an opportunity to add a lot of flexibility to how matched routes are handled. I'm not even specifically interested for this for the sake of loading handlers asynchronously, but I think this same feature could be really useful anywhere you want to be able to dynamically build a handler given the context of a matched route.

Doing that with the current API is awkward, but if in the suggested getHandlerForRoute the callback were to either take a (Handler, props), or an 'instantiated handler' like (<Handler some="dynamic props" />) (which you could still inspect statically, and then cloneWithProps) then route handlers would become much more expressive.

@pburtchaell
Copy link

This is good. I'm currently building an app that would benefit enormously from this API.

We should start a new thread to discuss how this API would be connected to transition hooks and the loading component idea. IMO that is a very important part of this.

@ofersadgat
Copy link

@ryanflorence An alternate idea is to let each handler declare its own routes, then when a parent dynamically loads a child, it can add it to its own routes. I have a rough prototype of this which only requires one small change: #798 . The way it works is by having a handler add or alter its own routes, then call replaceRoutes in the Router with the root and it will then reload all of the new routes. Not the most efficient implementation, but if you like this path, it can be made more efficient.

@josephsavona
Copy link

@ryanflorence @mjackson this looks good. It would probably be necessary to be able to abort a call to getHandlerForRoute, for example when the user attempts a second transition before the first completes. This would be especially helpful if there's a lot of code for the route and/or you're on a slow connection.

@EvHaus
Copy link

EvHaus commented May 22, 2015

I have another use case for async loading which I'm hoping this issue will be able to address in the future. My app's flow is as such:

 +------------------------------+
 |   GET request to /internal   |
 +------------------------------+
                |
                V
    +------------------------+         // At this point, the only routes
    |   React.Router.run()   |         // the Router knows about are
    +------------------------+         // root (/) and 404.
                |
                V
 +--------------------------------+    // The main app is wrapped in an
 |  willTransition() for AppView  |    // authentication mixin. So we
 +--------------------------------+    // check if the user is logged in.
                |
                V
   +--------------------------+    +----+    +---------------------+
   |  Check if authenticated  |--->| No |--->| redirect to /login  |
   +--------------------------+    +----+    +---------------------+
                |
                V
             +-----+
             | Yes |
             +-----+
                |
                V
   +----------------------------+     // Webpack async bundles are only
   |  Dynamically load modules  |     // loaded only if the user is authenticated.
   +----------------------------+     // The bundles contain additional routes.
                |
                V
 +-------------------------------+    // One of the loaded modules will
 |   Add routes to React-Router  |    // extend the React.Router with the
 +-------------------------------+    // Component for the /internal route
                |
                V
 +-------------------------------+    // Finally, we should load the Component
 | Load Component for /internal  |    // for the /internal route. Currently this
 +-------------------------------+    // fails because there's no way to
                                      // add routes in the middle of a transition
                                      // in the previous step.

So the main issue here is that when React.Router.run() is executed, it doesn't have the route that is being requested. That route needs to be added mid-way through the transition after authentication is completed. However, at the moment, trying to add a route in the middle of a transition causes all sorts of issues. If I use Router.addRoutes() - it simply takes me to a 404 (I'm guessing because adding a route in the middle of a transition is too late) and if I use Router.replaceRoutes() - I get all osrts of issues (primarily that the initial transition is aborted and the wrote route request is resubmitted from the beginning).

@gaearon
Copy link
Contributor

gaearon commented May 22, 2015

I think new API should solve this with async route configuration.

@ryanflorence
Copy link
Member Author

1.0 does all of this and MORE SEND US MONEY!

@EvHaus
Copy link

EvHaus commented Jun 16, 2015

@ryanflorence Cool! Do you have any docs yet for the new async stuff in 1.0?

@goldensunliu
Copy link
Contributor

+1

@Duan112358
Copy link

smell nice

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests