Proposal: Replace AsyncState with getAsyncProps

[mjackson]

Edit: Please read my updated proposal as well

I'd like to propose adding two new transition hooks, didTransitionTo and didTransitionFrom that are designed to replace the AsyncState mixin. Currently, AsyncState has the following quirks/limitations:

• It keeps data in this.state, but data is passed passed to children through props in render
• It's a mixin but it was only ever intended to be used in route handler components, so users may be inclined to use it in other components (which we don't support). Also, it means that users need to declare the mixin

The goals for these two new transition hooks are:

• Keep data in props, where it belongs. this.state should be used for UI state
• Keep the ability for components to subscribe to changes in data, but also have a way to let components know that data only needs to be fetched once (for server rendering)
• Keep the ability for components to fetch data in parallel
• More clearly indicate that data fetching behavior belongs in route handlers, and not other components and eliminate the need for users to declare a mixin in order to use it

I believe the following API satisfies all of the above requirements. Feedback is welcome.

var User = React.createClass({

  statics: {

    // This hook is called after the transition has been validated by all
    // willTransition* hooks. The setProps argument is a function that
    // can be used to set props ad hoc (like AsyncState's setState).
    didTransitionTo: function (params, query, singleRun, setProps) {
      // If you have an immediate value, use it.
      setProps({ user: UserStore.getUserByID(params.userID) });

      // If we're not on the server, subscribe to changes in the data.
      if (!singleRun) {
        this.userListener = function () {
          setProps({ user: UserStore.getUserByID(params.userID) });
        };

        UserStore.addChangeListener(this.userListener);
      }

      // Or, ignore setProps entirely and return a hash of values that should
      // be used as props. Values may be immediate or promises. As they
      // resolve, props are updated.
      return {
        user: fetchUser(params.userID)
      };
    },

    // This hook is called after the transition has been validated by all
    // willTransition* hooks.
    didTransitionFrom: function () {
      UserStore.removeChangeListener(this.userListener);
    }

  },

  // The render method needs to be resilient to the fact that props
  // may not yet be loaded.
  render: function () {
    var user = this.props.user;
    var name = user == null ? 'anonymous' : user.name;

    return <div className="user-component">Hello {name}!</div>;
  }

});

Note that, although this API looks similar to the willTransition* API, it has two key differences:

• handlers are run in parallel, not in sequence, so all data can be fetched at the same time
• the resolution of these handlers does not block the transition from happening

Thanks to @rpflorence for giving me feedback and helping me think through this API earlier today!

Edit: Updated example code to make setProps the last (optional) argument to didTransitionTo.

[ryanflorence]

👍

if (!singleRun) {
  this.userListener = function () {
    setProps({ user: UserStore.getUserByID(params.userID) });
  };

  UserStore.addChangeListener(this.userListener);
}

Looks sorta bleh, but it could easily be wrapped up and made declarative based on the architecture of an individual application, like mapping async props to stores by key with a mixin etc.

// I dunno, something like this would totally be possible
var SomeHandler = React.createClass({
  mixin: [FluxBBQ({
    users: UserStore,
    assignments: AssignmentStore
  })]
});

Good that the router doesn't get too interested in application conventions.

[mjackson]

I almost wrote it like:

        UserStore.addChangeListener(this.userListener = function () {
          setProps({ user: UserStore.getUserByID(params.userID) });
        });

A little more concise :) But yeah, the router needs to be pretty generic so your app can do whatever it wants.

[ewiner]

Do you mean AsyncState? I'm not seeing what this has to do with ActiveState.

[mjackson]

@ewiner Yes, thanks for pointing that out. My brain is fried from thinking about this too much..

[ryanflorence]

I wouldn't mind supporting the use cases described in #176, namely, leaving the previous screen rendered while these transition hooks resolved, via an option on <Route/> perhaps.

[mjackson]

@rpflorence Interesting. So basically just a flag that gives you fine grained control over when you actually want things to update?

The way I see it there are two options:

1. after willTransition* hooks resolve (this is what we currently do)
2. after didTransition* hooks resolve

What's the main use case for the second? To prevent flickering while data loads? What about the delay in navigation?

[sophiebits]

The setProps callback you have is weird to me. State is a fine place to store data that comes from a store – I feel that the biggest difference between props and state is where it comes from, not model state vs. UI state or anything like that.

[KyleAMathews]

I'm indifferent about props vs. state and like avoiding needing to declare a mixin.

[KyleAMathews]

Why is there a didTransitionFrom? How is it different from componentWillUnmount?

[sophiebits]

(setProps sounds confusingly similar to this.setProps which you can't and shouldn't call on yourself.)

[mjackson]

@spicyj setProps is the props equivalent of AsyncState's setState function, which I believe also was a little weird to you when I first introduced it :)

The important thing to note is that you don't actually have to use it. In fact, it should probably be the last argument to didTransitionTo. The only reason it exists is to support the streaming data use case. We could omit the argument entirely if we don't want to support streaming.

The way the router is built, all data is passed in as props (think params, query, etc.). The setProps argument is just a way to add to those.

[mjackson]

@KyleAMathews didTransitionFrom is specific to the router. It doesn't care about mounting. It's really just the logical compliment to didTransitionTo with slightly different semantics from componentWillUnmount.

Up to this point people have been using componentDidMount and componentWillUnmount as a hack for loading/unloading data. This commit addresses that.

[sophiebits]

didTransitionTo also sounds like an odd place to do data-fetching – I would expect it to get called after data is fetched, not before.

[mjackson]

I just picked these names because they correlate directly with the willTransition methods. If you have better suggestions, I'd love to hear them.

[mjackson]

@rpflorence BTW, I should mention that it would be trivial to implement #176 using the architecture I've got going (been hacking on this today). Just kind of seems like a footgun.

[ryanflorence]

I think it's valid. Lots of apps do this and it feels fine. Gmail, google analytics, some parts of Facebook, even some native iPhone apps I use.

Sent from my iPhone

On Sep 3, 2014, at 7:01 PM, Michael Jackson notifications@github.com wrote:

@rpflorence BTW, I should mention that it would be trivial to implement #176 using the architecture I've got going (been hacking on this today). Just kind of seems like a footgun.

—
Reply to this email directly or view it on GitHub.

[mjackson]

@rpflorence ok. Let's discuss in a separate issue.

[ewiner]

In React's core API, the will* and did* methods are intended for doing side effect operations with no return value before or after some built-in action, while the get* methods are intended for you to return some data that the library will then use.

didTransitionFrom() seems like more of the former case, while didTransitionTo() is the latter. I would suggest ditching the parallelism with the willTransition methods and name them following the core API's naming pattern, maybe something like:

• getPropsAsync instead of didTransitionTo
• componentDidTransitionAway instead of didTransitionFrom

I didn't suggest getInitialPropsAsync or getDefaultPropsAsync because the props are neither initial nor default. And I feel much less strongly about the didTransitionFrom name, because like @KyleAMathews I don't quite understand the intended use of it yet.

[ryanflorence]

didTransitionFrom is a spot to remove change listeners on stores.

I like @ewiner's names.

[ryanflorence]

(though, willTransitionFrom is as good a place as any for that ...)

[KyleAMathews]

@rpflorence @mjackson why can't we just continue using componentWillUnmount for removing listeners like we already are...?

[mjackson]

@rpflorence willTransitionFrom is different from didTransitionFrom because the former implies that the transition is still abortable whereas the latter means that it's actually happened. In the first case you don't want to teardown listeners but in the second case you do.

@KyleAMathews componentWillUnmount is an instance-level method, so you should probably use it in conjunction with componentWillMount. If you want to use those methods for loading data and cleaning up, go ahead. But the router can't take advantage of data loaded via that mechanism to render your templates on the server because it doesn't know what components need data until after it renders, which is too late.

@ewiner yeah, something like getPropsAsync could work. componentDidTransitionAway feels like an instance-level method though because it uses the word "component".

didTransitionFrom actually feels fine to me. It's didTransitionTo that's the problem IMO, because it has a return value.

[ryanflorence]

/me casts a vote:

getAsyncProps

didTransitionFrom

[mjackson]

yeah @rpflorence 👍

[karlmikko]

How will this deal with errors in fetching data? You will want a way of passing those errors (from the promises) as props also.

[mjackson]

the singleton stores

@taurose Flux is an anti-pattern when it comes to the server. Stores just don't make sense in a stateless environment. I've been struggling to make it work for a while now, but there are a lot of things that just don't make sense (including actions, dispatchers, etc.)

@appsforartists So, initially we were using state for storing data. But that doesn't work for server-side rendering because the environment is inherently stateless, so we need to put all the data in props.

[ewiner]

@mjackson @taurose I've been using yahoo/dispatchr, which solves the problem of server-side singleton stores by... well.. by not being a singleton. I instantiate a new Flux for each request on the server, call a few actions to set up the data in the right way, then use the dehydrate method to serialize out the right data to send over to the browser.

[ryanflorence]

@mjackson I like it, I've been kind of yearning to go in that direction also.

stuff about flux

This is not a problem the router has to solve. The router is providing an API to:

1. provide async props
2. do work after a transition

It's up to not-the-router to figure out how that works with your data.

[karlmikko]

@mjackson @rpflorence I agree - the router should provide an API not be a data store.

[ryanflorence]

I think this represents a typical use-case with something like flux:

var Courses = React.createClass({
  statics: ENV.SERVER ? {
    // only defined for the server
    getAsyncProps: function() {
      return {
        courses: db.getCourses()
      };
    }
  } : {},

  getInitialState: function() {
    return {
      // slurp up state from props if they are there
      courses: this.props.courses || []
    };
  },

  getStateFromStores: function() {
    return {
      courses: CourseStore.getState()
    };
  },

  // only called on the client
  componentDidMount: function() {
    CourseStore.addChangeListener(this._onchange);
  },

  componentWillUnmount: function() {
    CourseStore.removeChangeListener(this._onchange);
  },

  _onchange: function() {
    this.setState(this.getStateFromStores());
  },

  render: function() {
    return (
      <div>
        <h1>Courses</h1>
        <CourseList course={this.state.courses.length}/>
      </div>
    );
  }
});

We might not even need the setProps stuff. On the client, the props get set as promises resolve, on the server the router knows to wait for everything.

(I don't think I'd care to make my stores work isomorphically, I'd rather just do what that code does)

[agundermann]

Sorry I wasn't clear. I was talking about the stores in react-router which would cause problems when rendering (resolving async props for) more than one route at the same time as far as I can tell. I didn't mean to take this off-topic though 😄

[ryanflorence]

I think we've already refactored that code already for that reason.

Sent from my iPhone

On Sep 23, 2014, at 10:34 PM, Alexander Gundermann notifications@github.com wrote:

Sorry I wasn't clear. I was talking about the stores in react-router which would cause problems when rendering (resolving async props for) more than one route at the same time as far as I can tell. I didn't mean to take this off-topic though

—
Reply to this email directly or view it on GitHub.

[mjackson]

@taurose I've already prepped the PathStore for server-side work, RouteStore still needs some work tho. But after @ewiner's comment, I'm taking a closer look at yahoo/dispatchr which seems like a better fit for server-side work.

[mjackson]

@rpflorence Yeah, I like your example. Let's strip the whole thing down to just getAsyncProps(params, query) and go from there.

[mjackson]

Just realized: we should also deprecate/remove "static" props that are passed to <Route> components when this lands. They're confusing, and we don't need them any more.

[appsforartists]

@mjackson I take it you're talking about removing the ability to go

<Route 
  handler              = { myHandler } 
  arbitraryHandlerProp = { blah } 
/>

I'm working on a project using ReactRouter on both the client and server, and I'm trying to find the right way to pass data into the route tree. I've put together a library called Ambidex that takes a set of <Routes /> and returns a mach stack that will render it. I'm using stack.map(domain, ambidexInstance) to make sure the requests for each one are routed correctly, and ReactRouter.renderRoutesToString to generate the HTML.

In this project, each domain is effectively running the same app with some minor configuration changes. I need to be able to access those changes from within the common app's components to make sure to render the correct variant. Here's a flowchart, though I'm not sure if it's helpful:

                             |----------------|----------------|           
                             |   variant_1    |   variant_n    |           
                             |________________|________________|           
                                    |                  |                   
                              customization      customization             
                                    |                  |                   
                                    V                  V                   
                             |---------------------------------|           
                    |------> |     common elements of app      | <------|  
                    |        |---------------------------------|        |
                    |               |                 |                 |
                    |            settings          settings             |
                    |               +                 +                 |
                    |             routes            routes              |    
                settings_1          |                 |             settings_n
                    |               V                 V                 |
                    |        |----------------------------------        |
                    |        |            ambidex              |        |
                    |        |----------------------------------        |
                    |               |                 |                 |
                    |               |                 |                 |
                    -----------------                 -------------------

The settings for each instance are generated dynamically, but the common app can only require code statically. Inside each component, I need to be able to do things like
<img src = { settings.STATIC_URL + "/logo.svg" } /> and have it resolve to the correct URL.

I tried having the common app call Ambidex.getSettings() inside render, where getSettings can look in the call stack and figure out which version of settings to return, but that doesn't work when renderRoutesToString is async.

If I could use transferPropsTo to pass the settings into <Routes/>, which would them pass them to the appropriate route.@handler that could work. However, it sounds like you're backing away from supporting that sort of thing.

On the client, I'd just close-over the appropriate settings to make sure they're always in-scope. Unfortunately, I don't know of any way to pass local scope into another module in Node.

I'd love some advice here. Is there a good way of passing props into a view when it's being rendered on the server? Anything else jumping out at you that could be a good solution?

Please let me know if there's anything I can clarify. It's hard to explain all this in a comment field on a Friday evening if you haven't seen the app. 😃

Thanks!

[mjackson]

@appsforartists I'd recommend against using renderRoutesToString just yet. That PR is a loooooooong way behind the current master branch, so it's hard to say what's broken and what will work when using it.

[appsforartists]

Thanks @mjackson. It's just a prototype for now, so I'm not too concerned about stability yet. I'm more interested in figuring out how to pass data down from my server into the route handlers, so I'll be ready when renderRoutesToString is. 😀

Do you have any input there?

[mridgway]

Hey, guys. I'm the author of dispatchr and I'd love to help where I can. We're using dispatchr and other libraries to handle routing right now but we like the syntactic sugar the react-router gives within components to define routes and determine subcomponents rather than handling that outside of React. To give you some ideas of how we're handling server and client flux flows, you can see our flux-examples repo, specifically the routing example.

@mjackson I think there's room to converge on some of our ideas here.

[mjackson]

Geez, all these comments and now the whole story around props has changed with 0.11 (see the upgrade guide). I'm gonna close this for now, but thank you everyone for your contributions to the conversation.

@mridgway Welcome to react-router :) If you'd like to get involved, please do! We can always use more help. We try to keep on top of the GH issues as much as possible in the hopes that it will be easy for newcomers to see where they can pitch in if they'd like to. The vast majority of conversation around this project happens on GH and in the #rackt channel on freenode.

Happy hacking! :)