Skip to content

[added] getAsyncProps for route handlers #396

New issue

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

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

Already on GitHub? Sign in to your account

Closed
wants to merge 16 commits into from
Closed

Conversation

mjackson
Copy link
Member

Stuff to do for server rending integration:

  • don't replace the HTML (253bc1a)
  • get data from multiple promises (db8298a)
  • add some nested routes and make sure they work
  • get activeRouteHandler to work again
  • wait for async props to load before changing the previous screen by default (avoid flickers, assume data fetching is fast)
  • add option to <Route/> to not wait for data to load (opt in when you expect it to be slow)
  • add some indication to route handler being transitioned from that it is on the outs while data loads for the next route

This commit adds the ability for route handlers to load props
they need for a given route, optionally asynchronously. Props
loaded in this manner cascade from route handlers further up
the hierarchy to children, such that props loaded by children
take precedence.

getRouteProps should return a "hash" of props, the values of
which may either be immediate or deferred using promises. As
promises resolve, forceUpdate is used to re-render the <Routes>.
If no props are promises, the operation is fully synchronous
and route handlers will have all the props they need on the
initial render.

As implemented, this work should satisfy the use cases in #319,
#261, #314, #374, and (indirectly) #262.

This commit adds the ability for route handlers to load props
they need for a given route, optionally asynchronously. Props
loaded in this manner cascade from route handlers further up
the hierarchy to children, such that props loaded by children
take precedence.

getRouteProps should return a "hash" of props, the values of
which may either be immediate or deferred using promises. As
promises resolve, forceUpdate is used to re-render the <Routes>.
If no props are promises, the operation is fully synchronous
and route handlers will have all the props they need on the
initial render.

As implemented, this work should satisfy the use cases in #319,
 #261, #314, #374, and (indirectly) #262.
@ryanflorence ryanflorence added this to the props Props PROPS milestone Oct 13, 2014
@ryanflorence
Copy link
Member

Alright, I've been screwing around with this over at https://github.com/rackt/react-router-mega-demo.

It works great!

Except, one thing, it can only resolve one promise and then it calls back. But I must get some rest. I'm going to spend some time making a release instead of working on this tomorrow morning, not sure when I'll get back to it, but hopefully we can get this all working this week :D

@ryanflorence
Copy link
Member

Also, the HTML is getting completely blown away, but I really barely know much about server-side rendering in react, so maybe I'm just not doing something I'm supposed to.

http://react-router-mega-demo.herokuapp.com/

@mjackson
Copy link
Member Author

it can only resolve one promise and then it calls back

Not sure what you mean by this.

@ryanflorence
Copy link
Member

In order to not blow away the HTML, all the props to the routes have to be the same. This changes our approach a bit.

@ryanflorence
Copy link
Member

Or change the approach completely ...

In order to not blow away the DOM when the client code lands, we have to have the props before creating our route config:

<!-- must be the same props on the server and client -->
Route(props);

In a nutshell, on the server we need to

  1. gather up all the props
  2. add the props to the server-side render of Routes
  3. render to html
  4. dynamically generate the JS for the client with the gathered props on the Route components directly.

You define your routes in one place, but then we have to go update the ones that are actually used for the client to have the same data-checksum.

@ryanflorence
Copy link
Member

I'm unclear if we will be able to get the proper checksum w/o putting the properties directly on the Route components. Or maybe we can just put all the propData on Routes and then the handlers get it from the context.

that javascript scope will get you EVERY TIME
if you let it
@appsforartists
Copy link

I just skimmed this PR, so I might be missing something, but I don't think this is a substitute for #374.

#374 allows the Routes object to be treated as a black box: You give me a Routes and I can inject a set of props to all its handlers without knowing anything about how it's architected or it knowing anything about me (except what I will name the props I inject). This appears to make each Route responsible for looking up its own props, which makes it harder (perhaps impossible) to write dynamic, loosely-coupled code on the same level as #374.

Do you expect Routes to have its own getRouteProps prop? If so, I might be able to emulate #374 like this:

var oldGetRouteProps = routes.props.getRouteProps;
var getRouteProps = function () {
    var result = oldGetRouteProps();
    result["settings"] = settings;

    return result;
};

routes = React.addons.cloneWithProps(
    routes,
    {
        "getRouteProps":   getRouteProps
    }
);

Otherwise, I don't think this can replace #374.

@sophiebits
Copy link
Contributor

(The props don't have to be the same; the generated HTML does.)

@ryanflorence
Copy link
Member

thanks @spicyj, 253bc1a seems to prove we're doing it right, my demo must be doing something wrong.

@ryanflorence
Copy link
Member

@appsforartists instead of <Route prop1="foo"/> you use a static getRouteProps on your handlers.

@appsforartists
Copy link

@rpflorence I feared that. I don't think this is a replacement for #374.

@ryanflorence
Copy link
Member

Same effect, different location. Is it simply the API that is unappealing
to you? Or is there a technical difference?

On Monday, October 13, 2014, Brenton Simpson [email protected]
wrote:

@rpflorence https://github.com/rpflorence I feared that. I don't think
this is a replacement for #374
#374.


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

@appsforartists
Copy link

@rpflorence,

Technical difference:

#374 enables a server to serve an arbitrary set of routes that it knows nothing about, but it can pass data down for them to consume if they need to. The handlers don't have to be written any differently or implement any magic static methods. In theory, someone could compose a more opinionated library on top of #374 that would interact with handlers the same way as ReactRouter (by passing specially named props down into them).

In my particular use case, I've put together a server (that I plan on open-sourcing when I get the go-ahead from legal) that will take a set of routes and turn them into an application that can render on both the client and server (and be live-edited with react-hot-loader). The consuming app just needs to pass in a set of Routes, and I pass environmental data down into them as props.

I have a prototype of this written on top of #374, and I wouldn't be able to port it over if this lands and #374 doesn't.

@ryanflorence
Copy link
Member

@appsforartists I've been having a thought that smells like what you're saying but how does your server know what data to go get if it knows nothing about your routes?

@appsforartists
Copy link

@rpflorence, in this case, it's all dynamic.

My app is structured {demoA, demoB, demoC}:DemoGenerator:Ambidex, where Ambidex is the server.

Some of the settings are generated in the individual demo instances and some are generated within their constructor (here called DemoGenerator). The Routes object lives in the generator, so it wouldn't be able to statically look anywhere to find the right settings at runtime, but through the magic of inheritance/composition/instantiation, I can create a settings object and pass it down the chain to where it needs to be.

All three demos are being served by a single Node instance because I only have access to a single load balancer; the server itself routes based on the request's Host domain.

@appsforartists
Copy link

The settings that Ambidex passes down the tree are indeed passed into it, but there's no guarantee that there's a single place in its lineage where those settings exist wholesale; in my case, they're all composed in-memory, so there's nothing I can require to look one up.

@ryanflorence
Copy link
Member

@appsforartists if you simply had <Routes handlerProps={someStuff}/> and that stuff went down to all your handlers props, would that be enough?

@appsforartists
Copy link

@rpflorence Yup. That's #374.

@ryanflorence
Copy link
Member

@mjackson I don't think we should merge the props from the parent into the child (and not just because it breaks everything) but instead implement handlerProps from #374.

One of my primary goals with the router is to be able to move view hierarchy around w/o having conventional data flow fall apart. If props get merged from parent to child, you could move a handler and then not have the props you thought you did anymore.

If you need to pass props from a handler to a child, you can do that easily and more idiomatically with <this.props.activeRouteHandler my={extra} props={here} />. Then when refactoring you will see immediately what data needs to be replumbed.

Finally, if we implement handlerProps from #374, then every handler gets all the same global props, which satisfies a lot of use-cases people are asking about. Additionally, it still keeps UI refactors from getting yucky because there is no implicit coupling of view hierarchy and data.

@tobice
Copy link

tobice commented Oct 19, 2014

How is what I proposed storing the state outside of the stores? The router fetches from the hook, but > the store keeps the data.

Sorry, maybe I am a little lost. You're right that in a running application, the state is kept by the stores. But let's move to what gets dehydrated/rehydrated on the way from server to client. Because if I understand it correctly, you suggest that on the server I dehydrate the return value of getAsyncProps and on the client side I use the rehydrated data to populate my store. Okay, why not, but for me it feels much more straightforward to dehydrate the stores. The stores are the state. I start the work at the server, do all I can, the application reaches certain state, at that point, I freeze the state, pack it/dehydrate it/serialize it/whatever, send it to the client and transparently pick up where I left. It's like when moving a virtual machine from one real server to another. This way I can write 100% isomorphic code. I don't have to use any hacks like checking Router.getAsyncPropsFor('root') whether there is data available from the server.

Anyway, it's just one way to do things. The important thing is that your proposed API will let me do that. But why wouldn't willTransitionTo work? I was thinking about something like this:

willTransitionTo: function(transition, params) {
  var store = context.getStore('ArticleStore'); // context from closure
  if (!store.hasArticle(params.id)) {
    transition.wait(context.callAction(loadArticle, params.id)); 
  }
}

This should just wait until loadArticle resolves and only then let the transition finish. Or am I wrong?

@stevoland
Copy link

@tobice 👍 This is the API I would love for server rendering.

@th0r
Copy link

th0r commented Oct 20, 2014

@tobice I use completely same approach as you: I use willTransitionTo to call action (returning promise), that makes request for page-data and then uses multiple dispatches to distribute data between stores. So, when promise resolves, all stores, needed to render the page, already contains all necessary data.
The only thing needed for this approach to work server-side is possibility to pass current request's Flux instance (stores with actions) to the willTransitionTo hook and to have ability to access it from the renderRoutesToString callback to dump current stores state.

@tobice
Copy link

tobice commented Oct 20, 2014

@th0r And what about that closure trick @rpflorence suggested? It's not nice but that should do the job right?

@th0r
Copy link

th0r commented Oct 20, 2014

And another one nice thing about using willTransitionTo hook to fetch route data: you can use them not only on the final route handler, but on any parent handlers to get common data for the set of routes.
What I mean is you can use this hook on the App component, for example, to get common data for the whole app (current user, notification counters etc.), save it in corresponding stores and router won't refetch it during page transitions! I think it's a great trick and I successfully use it in my app!

@th0r
Copy link

th0r commented Oct 20, 2014

I think there is a huge overhead in this "closure" approach: for each request you are building the whole routes tree and creating new copies of component classes!

@tobice
Copy link

tobice commented Oct 20, 2014

So you are worried about performance? I cannot judge how demanding this recreating would be but it's pure JavaScript so it should be fast in general. I would guess that in a real-life application the bottleneck would be something else, like any I/O operation. But I would definitely prefer if I could just get a context instance to willTransitionTo method. Maybe a fork for us, the 100% Flux positive guys? :)

@appsforartists
Copy link

@th0r I think you misunderstand closures. If you look at how @rpflorence's example is written:

<Route name="root" path="/" handler={rootHandler(context)}/>

rootHandler is being called immediately. So, you can logically replace it with its return value:

<Route name="root" path="/" handler={RootComponentCurriedWithContext}/>

That component is what's being used on every request. rootHandler is only run once, when the routes are parsed.

@tobice
Copy link

tobice commented Oct 20, 2014

@appsforartists actually I need every request to have its own context. Therefore I have to recreate all routes with the new context in each request (call createRoutesWithContext). So in every request React.createClass({ ... }) will be called for each handler defined in the tree.

@appsforartists
Copy link

@tobice Maybe not. There's an experimental feature in React, also called contexts. It's a way to pass quasi-global (e.g. request-level data) down through the component tree. It's not yet documented because there are some hard-edges that need to be polished, but it could be helpful in your case.

You would create a root handler whose only job would be to close-over the Dispatchr context. It would then put that into a React context to be passed down to its children.

Here's an example of using contexts.

@appsforartists
Copy link

Something like this:

/**
 * @jsx React.DOM
 */

var Home = React.createClass(
  {

    "contextTypes":       {
                            "dispatchrData":      React.PropTypes.object.isRequired
                          },  


    "render":             function () {
                            return  <div>
                                      <h1>
                                        Hi from the home screen!
                                      </h1>
                                      { /* use this.context.dispatchrData however you normally would */ }
                                    </div>;
                          }
  }
);

var curryRootWithDispatchrData = function (dispatchrData) {
  var Root = React.createClass(
    {
      "childContextTypes":  {
                            "dispatchrData":      React.PropTypes.object.isRequired
                            },

      "getChildContext":    function () {
                              return {
                                "dispatchrData":  dispatchrData
                              }
                            },

      "render":             function () {
                              return  <this.props.activeRouteHandler />;
                            }
    }
  );

  return Root;
};

var routes = (
  <Routes location = "history">
    <Route handler = { curryRootWithDispatchrData(dispatchrData) } >
      <Route handler = { Home } />
    </Route>
  </Routes>
);

React.renderComponent( 
  routes,
  document.body
);

@appsforartists
Copy link

Notice how Home (and any nested components) are created/instantiated once. All your per-request data routing is abstracted into curryRootWithDispatchrData. All it does is wrap your normal route tree so it can pass down the request-level data in context.

@tobice
Copy link

tobice commented Oct 20, 2014

I've heard of this React context but I also remember something about it being experimental and not implemented after all. Anyway, the thing is that it doesn't really solve my problem because it's tight to an instance whereas the willTransitionTo (or getAsyncProps) are static functions that are called before the component is even instantiated. So to have the context available in this static method, it has to be either passed as an argument or available from the closure.

@th0r
Copy link

th0r commented Oct 20, 2014

@appsforartists, believe me, I know what closures are =)
@rpflorence wrote:

// main.js
var context = makeContextHoweverYouDoIt();
var routes = createRoutesWithContext(context);
React.render(routes);

I think he meant, that this code should be executed per-request, not once.
I'll try to explain my vision of server-side rendering using flux architecture with the following pseudo-code:

// react-router-middleware.js
var Router = require('react-router');
var getFluxInstance = require('./flux-factory');
var routes = require('./routes.jsx');

module.exports = function (req, res, next) {
    var flux = getFluxInstance();
    var context = {
        flux: flux
    };

    Router.renderRoutesToString(routes, req.path, context)
        .then(function (result) {
            var statusCode = 200;

            // `transitionTo` calls during server-side rendering should immediately stop it
            // and resolve `renderRoutesToString` promise with the `redirect` flag set
            // and with the new path set as `path`.
            // Same thing should happen if `Redirect` route was matched.
            if (result.redirect) {
                return res.redirect(result.path);
            }

            // if `NotFoundRoute` was matched, `notFound` flag is set.
            // `html` prop contains rendered html string.
            if (result.notFound) {
                statusCode = 404;
            }

            // Wrapping generated html with some layout and sending it to the client
            // with serialized stores state.
            // We can even get `statusCode` from some store if we need to.
            return res
                .status(statusCode)
                .render('layout', {
                    // We can access any data we need from stores
                    title: flux.stores.PageTitle.getTitle(),
                    html: result.html,
                    // Put serialized data in some global variable and
                    // call `flux.fillStores(window._storesData)` on the client
                    // or do some more complex things with it
                    data: flux.serializeStores()
                });
        })
        .catch(function (err) {
            // Some kind of error during rendering
            // (transition aborted, transition promise rejected etc.)
            // You can store any additional info either in `context` object,
            // or in reject error, or in stores and do whatever you want here.
            next(err);
        });

};
// flux-factory.js
var Flux = require('some-flux-implementation');
var stores = require('./stores');
var actions = require('./actions');

module.exports = function getFluxInstance() {
    var stores = {
        PageTitle: new stores.PageTitleStore(),
        CurrentUser: new stores.CurrentUserStore(),
        ViewItem: new stores.ViewItemStore()
    };

    return new Flux(stores, actions);
};
// routes.jsx
var App = require('./handlers/App.jsx');
var ViewItem = require('./handlers/ViewItem.jsx');

module.exports = (
    <Routes location="history">
        <Route handler={App}>
            <Route name="view-item" path="/item/:itemId" handler={ViewItem}/>
        </Route>
    </Routes>
);
// handlers/App.jsx
var App = module.exports = React.createClass({

    statics: {

        willTransitionTo: function (transition, params, query, context) {
            // Fills `CurrentUser` store
            transition.wait(context.flux.actions.getCommonPageData());
        }

    },

    render: function () {
        /* ... */
    }

});
// handlers/ViewItem.jsx
var ViewItem = module.exports = React.createClass({

    // Mixin, that subscribes to stores and gets handler state from them
    mixins: [StoreWatchMixin('ItemView')],

    statics: {

        willTransitionTo: function (transition, params, query, context) {
            // Will run only after App's `willTransitionTo` hook resolve,
            // so we already have `CurrentUser` store filled
            var flux = context.flux;

            if (!flux.store('CurrentUser').user) {
                // Router should stop server-side rendering at this point
                // and call `renderRoutesToString` callback with `redirect` flag set
                return transition.redirect('login');
            }

            transition.wait(context.flux.actions.getDataForViewItemPage(params.itemId));
        }

    },

    getStateFromStores: function () {
        // `context` should be provided by router using react context feature
        var ItemView = this.context.flux.stores.ItemView;

        return {
            /* ... */
        };
    },

    render: function () {
        /* ... */
    }

});

@tobice
Copy link

tobice commented Oct 20, 2014

@th0r 👍 that's it. The question is whether we can convince @rpflorence to implement it for us. In my opinion it's not an unreasonable request.

What actually works right now is this:

getRoutes(context) {
  return(
    <Routes location="history">
        <Route handler={App} context={context}>
            <Route name="view-item" path="/item/:itemId" handler={ViewItem} context={context}/>
        </Route>
    </Routes>
  )
}

Router.renderRoutesToString(getRoutes(context), path, function(...) {..});

Although it might resemble it, this is not the closure workaround. This is simply providing default props for the root components. That's something which is very natural for React and we do it all the time:

React.renderComponent({ ...props... }, mountPoint);

The default props provided for the root component are something that originated outside of the React hierarchy (or outside of the route hierarchy in this case). The existence of these props is pretty justifiable. Unless we want React to take care of everything, we need some entry point. After all, path passed to Router.renderRoutesToString is exactly this type of data.

Now quoting the documentation for Route:

Any additional, user-defined, properties will be become properties of the rendered handler.

So at this point, those extra properties are passed to handler instance. Perfect. So why don't we just pass them to hooks (like willTransitionTo etc) as well? They are available at the point when the hooks are called. It makes perfect sense to me. Those properties are just extra parameters that will be available in the instance and the only reason the hooks are static is because they have to be called before the component is actually instantiated.

@tobice
Copy link

tobice commented Oct 20, 2014

Okay, now I really don't want to get pushy or anything because I really do appreciate what you do... but could you consider changing this line to

handler.willTransitionTo(transition, match.params, query, match.route.props);

Because that's all I need to get full server-side rendering just the way I described it in my initial comment (I just positively tested it). And this doesn't seem like a change that could break something (but of course, I'm not that familiar with the code).

@mjackson
Copy link
Member Author

@tobice This is what I suggested in #319. I think we have a better solution for all of this coming soon tho. I'll open a new issue soon to discuss the new API.

@tobice
Copy link

tobice commented Oct 20, 2014

@mjackson Oh yes, you're right. Sorry, it's hard to keep up with all the stuff going on around. So consider this as an argument why to do it this way.

I'm curious about this better solution, but as far as I'm concerned, this solution would be good enough for me and would let me do things my way. Maybe there's only one thing. If I use transition.wait(), it will pause the transition both on the server and the client. Which is okay, but I can't simply implement any kind of a spinner on the client side. Therefore I would really like to have a some kind of a hook that would make the transition wait only on the server side and on the client side, it would let the component render with uncomplete data. Is getAsyncProps supposed to work like that?

Actually, if I think about it, I could implement it just using a smart mixin (assuming that I can define statics in a mixin), so its not a top priority.

@arnihermann
Copy link

Would it be possible to opt out from having scriptTagData(data) rendered as a part of the HTML? I'm using flux and don't have any use for it (at least not right now). I'd much rather be able to render it myself -- maybe the callback in renderRoutesToString should accept some kind of props parameter which users can render to a script tag themselves?

Edit: A minute later I found out that the callback gets the data as the 4th parameter. I guess my comment about opting out still stands since. It's not a mayor annoyance but it's rendering a script tag I (and probably others) don't need.

@arnihermann
Copy link

Another thing, 424bda1 changed behavior of getAsyncProps which I was depending on, which was that it was called in the browser. As of 424bda1, it's only called on the server. I was using it to conditionally load data like so:

getAsyncProps(params) {
  return {
    me: __SERVER__ && context.getApi('ProfileApi').getMe(),
    player: context.getApi('ProfileApi').getById(params.playerId),
  };
}

What would you suggest as an alternative for scenario like this?

@ryanflorence
Copy link
Member

Thanks for messing with this branch everybody.

It has revealed that we've gone a bit too far with our abstraction by hiding the creation of your views from you. @mjackson and I are working on some new code we hope to push up to a branch this week that will solve a lot of use-cases around handler props and server-side rendering. It's done by us getting out of your way, rather than some new API or convention like getAsyncProps.

We aren't saying too much about it right now because the best way to get a thumbs up or thumbs down on an API is to have a working prototype. Can't wait to get everybody's feedback on it, we are going to work hard to get it up quickly :)

@appsforartists
Copy link

It's pretty damned cool that even though we're experimenting on the JavaScript frontier, there are half a dozen people eager to give feedback on all these APIs (and that it's all based on the work of two guys in their spare time).

Cheers all around. I love this project.

@ryanflorence
Copy link
Member

@appsforartists thanks :) You, in particular, are going to love the new stuff. I should just stop talking and write some code.

@natew
Copy link
Contributor

natew commented Oct 21, 2014

👍 agreed, this is a great example of open source right here. Appreciate all your guys' work.

@arnihermann
Copy link

Thanks, can't wait to try it out.

@appsforartists
Copy link

I'll stand guard outside your door to make sure no one disturbs you while you write.

💂‍♂️

@tobice
Copy link

tobice commented Oct 21, 2014

Sounds like a job for the Night's Watch 💂‍♂️ 💂‍♂️

@mjackson mjackson deleted the async-props branch February 24, 2015 23:45
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants