Proposal: use transition.wait(promise) instead of returning promise from willTransitionTo

[mjackson]

In #261, @ewiner said:

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

I've been thinking about this a lot lately. Currently, we tell users that they can return a promise from willTransitionTo to temporarily pause the transition if they need to do something async. As implemented, this has the following effects:

• All transitions are async, whether the user needs it to be async or not
• This causes two renders on the initial page load, which we've had quite a few questions about

In #307 @gaearon suggests that he'd like to opt-out of async behavior entirely!

All of this has me thinking we should probably make it more explicit for people to opt-in to async behavior, using a side-effect instead of a return value.

transition.wait( promise )

Instead of looking for a promise to be returned from willTransitionTo, if the user wants the transition to be async, they need to call transition.wait(promise). This function causes the transition to wait for the promise to resolve before proceeding.

Instead of:

willTransitionTo: function (transition, params, query) {
  return doSomethingAsync().then(function () {
    transition.abort();
  });
}

we now do

willTransitionTo: function (transition, params, query) {
  transition.wait(
    doSomethingAsync().then(function () {
      transition.abort();
    });
  );
}

Users that don't need async behavior simply never call transition.wait, so the transition is sync for them. The pros are:

• People can easily opt-out of async transitions. Just don't ever call transition.wait
• Opting-in to async transitions is more explicit
• willTransitionTo has a side-effect instead of a significant return value, so it is more consistent with the rest of React

Feedback?

Edit: Updated example code to show how you might abort a transition asynchronously.

[mjackson]

I should also mention this API would be consistent inside willTransitionFrom as well, in case (for some reason) you need to do something async there.

[gaearon]

I really like this. It fits better with React's convention IMO.

[ryanflorence]

👍

[mjackson]

Related: #270

[ewiner]

👍 too

[jquense]

So while these lifestyle hooks are returning into a black box (the router) this seems like it may leave Zalgo cracks.

this change would allow for mixing sync/async (which is the point). it seems fine if you are doing all one or the other but mixing between routes could lead to inconsistent behaviour in components?

I'm not sure that inconsistency is large enough to warrant one or the other, but it seems the natural path will be to use the sync version until you need the async one. Only someone with a bunch of foresight and knowledge will choose just one approach (or know not mix the method in any one route)... what have y'all thoughts been?

[mjackson]

@theporchrat It's a lot easier to explain to users that their hooks are async because they have a transition.wait somewhere in there than to keep explaining about how the hooks are async by default. I agree that the most natural path will be to use sync until async is needed, which may not be a bad thing.

[ryanflorence]

If I remember right, we made it async so we could do some sort of async validation (and we were just copying whatever ember did when we weren't sure what to do).

I think maybe it doesn't make as much sense to do that in React. The only validation I can think of anymore is authentication, and that workflow has always been sync so far for me:

1. check some auth module/store for a session
2. not there
3. redirect
4. login
5. retry transition

As for validating a model you fetch from the server, you can redirect to a NotFoundRoute or just change the UI if the ajax call returns a 404. We don't prescribe loading data in the hook anyway, so that kind of validation isn't recommended there.

Maybe we just kill async transition hooks altogether until real use-cases pop up.

[gaearon]

I would also add that, transitioning to Flux, you usually replace async with fire-and-forget throughout the application anyway.

[ghempton]

As someone with a large Ember app who is potentially exploring switching (at least in part) to React and RR, I would be remiss if I didn't chime in here.

Async routing is important for a lot of reasons (Ember's router was initially sync as well). Canonical examples include:

• Loading Code
• Authentication
• Pre-loading data
• Exposing a dialog to the user

This would also benefit developers who are not using the flux pattern.

[mjackson]

@rpflorence The auth workflow for me has always been async. I need to actually send a request to the server in order to find out if I have a valid session. I don't see how you can avoid that.

[ryanflorence]

@ghempton

1. Loading code (https://github.com/rackt/react-router/blob/master/examples/partial-app-loading/app.js)
2. Authentication (https://github.com/rackt/react-router/blob/master/examples/auth-flow/app.js
3. Pre-loading data (Proposal: Replace AsyncState with getAsyncProps #261 (comment))
4. Exposing a dialog to the user: I'm not sure what this has to do with async transitions but maybe you mean being able to cancel a transition (https://github.com/rackt/react-router/blob/master/examples/transitions/app.js#L30)

None of those require an async transition with React and this router. Care to help me understand if I'm missing something?

[ryanflorence]

@mjackson you make the round-trip once, and then hang on to the information.

Anyway, happy to have transition.wait.

[mjackson]

@rpflorence In your auth example (2) you're hanging on to the auth token in localStorage. But that's kind of faking it. You don't really know if it's any good until you try and use it.

[mjackson]

@gaearon how are you doing auth with fire and forget?

[ghempton]

@rpflorence

1. I echo @mjackson's concerns
2. That example uses confirm which is sync. Seems like most dialogs these days are custom async ones.

[mjackson]

💡

I guess you could just do anything async that you need to do using getAsyncProps ?

[ghempton]

Also, with regards to code loading: the holy grail for me would be to load route definitions async. E.g. If I route to /admin/user/1, all of the routes underneath /admin would not be known in advance. This feels like it would require async– but I am also just starting to explore react-router.

[mjackson]

// In my app, getCurrentUser is an async call because it needs to
// send a cookie to the server to find out if the user is auth'd.

// Here's how I do it with transition.wait:

var App = React.createClass({

  statics: {

    willTransitionTo: function (transition, params, query) {
      transition.wait(
        checkAuthAndStoreItSomewhere().then(function (auth) {
          if (auth == null)
            transition.redirect('login');
        });
      );
    }

  },

  getInitialState: function () {
    return {
      auth: getAuthFromWhereverWeStoredIt()
    };
  },

  render: function () {
    // this.state.auth is always here.
  }

});

// Here's how I'd try to do it with a combination of getAsyncProps
// and componentWillReceiveProps:

var NO_AUTH = 'not sure how else to do this';

var App = React.createClass({

  statics: {

    getAsyncProps: function (params, query) {
      return {
        auth: checkAuth().then(function (auth) {
          // What do we do here when we're not auth'd?
          return auth || NO_AUTH;
        })
      };
    }

  },

  componentWillReceiveProps: function (nextProps) {
    // This is only gonna work client-side. We can't use the
    // Router.* methods on the server, at least not with FB's dispatcher.
    if (nextProps.auth === NO_AUTH)
      Router.replaceWith('login');
  },

  render: function () {
    if (this.props.auth == null)
      return null; // Render nothing, just like above.

    // use this.props.auth
  }

});

[jquense]

It doesn't seem obvious to me why one would want sync transitions for routes. In the best case (no server activity) you are waiting for the end of micro task. this seems super unlikely to cause any performance issues (how quickly and often are people switching routes???)

the double render issue seems like an indication of bad component construction on the part of the user, not a problem with the router (who is depending on render call counts????).

I'm certainly no expert but reading peoples use cases and I'm still not getting where the hurt is. It feels more natural to me that route transitions be async, since they are analogous to server routes.

The getAsyncProps approach it pushes concerns into my views I wouldn't normally want there.

It does seem like if the concern is to reduce user questions about whats happening, making the async explicit definitely would help, and it is more idiomatic React.

Ya'll certain that mixing both approaches in an application won't cause odd, hard-to-debug, inconsistencies down in route handlers?

[mjackson]

Ya'll certain that mixing both approaches in an application won't cause odd, hard-to-debug, inconsistencies down in route handlers?

This is my primary concern as well, but I think we can ease some of the pain of async debugging by using promises. Notice I didn't say:

willTransitionTo: function (transition) {
  transition.pause();
  doSomethingAsync(transition.resume);
}

;)

With promises we have a consistent way of propagating errors (as long as we don't screw it up, which, admittedly I've done before. Good thing is once we get our code fixed users usually don't keep having those problems).

<Routes onTransitionError> will always be available for users to handle errors. The only difference between sync/async will be that we need to use a setTimeout to break the promise chain for people that use transition.wait (which we're already doing, btw) so we don't swallow errors.