Skip to content

AsyncState mixin - wait for all routes to finish async operations #176

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
jakobz opened this issue Aug 7, 2014 · 20 comments
Closed

AsyncState mixin - wait for all routes to finish async operations #176

jakobz opened this issue Aug 7, 2014 · 20 comments

Comments

@jakobz
Copy link

jakobz commented Aug 7, 2014

I have several RouteHandlers in hierarchy that use AsyncState mixin. When transitions occurs, these RouteHandlers renders immediately after their async operations completed. This leads visually to "partial" transitions, when components appears sequentially. Even with ajax-delay spinners in place of loading components, it looks glitchy.

It would be nice to have an option to transition at once, after all async operations completed. Until there are operations to finish, app should not navigate to a new page, so user will not see a blank or incomplete page.

It will be also useful to have an ability to render an progress indicator somewhere on the page. This can be supported with something like props.isLoading on the parent route, which is true until all children async operations are done.

@ryanflorence
Copy link
Member

In Ember, if you click a link to a route with slow async operations for the model hook (equivalent to our AsyncState) it doesn't transition until the data all lands.

We discovered in user testing this is really bad because users have no idea anything is happening. Users end up clicking the link multiple times and think the app is clunky. In a normal web page experience (not single-page-app), even if the content is still on the page, the browser has a spinner, the address bar changes, etc.

Its important you provide the user with some feedback. Would love to see an example on jsbin, or a jing screencast or something of what is feeling glitchy.

@jakobz
Copy link
Author

jakobz commented Aug 7, 2014

As for me, showing a blank page during transitions is not the best way to provide feedback to users. I personally would prefer to show ajax spinner animation. Another one would like to have some fancy CSS animation. It would be nice to have a way to customize this.

@mjackson
Copy link
Member

mjackson commented Aug 7, 2014

@jakobz If you really want to wait until everything is done, just return a single property in your async state object and return everything when you're done.

getInitialAsyncState: function (params, query, setState) {
  return {
    finishedLoading: new Promise(function (resolve, reject) {
      // Nest async behavior as deep as you like. When you're done, do:
      setState(myState);

      // In the meantime, your render() method can show a spinner while
      // this.state.finishedLoading == null.
    })
  };
}

Just to be clear, your render method would look like this:

render: function () {
  if (this.state.finishedLoading == null)
    return <Spinner/>;

  return <div/>;
}

@mjackson mjackson closed this as completed Aug 7, 2014
@jakobz
Copy link
Author

jakobz commented Aug 7, 2014

That's how my code is working now. The problem with this approach is that a blank page is displayed during transitions, which cause flickering.

What I want is a way to keep current page displayed until all async operations for the next page are done.

@ryanflorence
Copy link
Member

Why is your page blank?

@jakobz
Copy link
Author

jakobz commented Aug 7, 2014

By "blank" I mean that only spinner animation is visible during transition. I want to have spinner over previous page while next page is loading. Not a spinner on a blank page. This will make transition much smoother visually, as there will be no "blank" page step. Moreover, that's how transitions in non-SPA sites looks like.

Here is an example of this issue and solution for it in the similar Async mixin: https://github.com/andreypopp/react-async#deferring-rendering-of-async-components

@ryanflorence
Copy link
Member

I wonder if you can leverage React's CSSTransitionGroup for this.

The debate about keeping the old view around or moving to the next w/o data has no correct answer. Native apps (especially on mobile) opt to move to the new screen w/o data than stay on the old one which is the kind of experience I'm after in a browser app.

@monsanto
Copy link

monsanto commented Sep 3, 2014

A way to put a spinner over the current page while loading would be excellent. I wish this point could be reconsidered. Behavior matching what the browser does by default shouldn't be as difficult to do as it is now.

@ryanflorence
Copy link
Member

By "current" you mean "previous"? The page that had a link clicked?

If so, I don't suspect we will ever make this easy (or even possible?). It is a far better user experience to tell the user what you're doing than leave the old content on the page.

There are times to look to old server-rendered apps as a model for behavior, and there are times to look at "native" apps. In this regard, I absolutely believe the better experience is native.

Tell the user what you're loading, or give them a blank page with a loader. The old content is completely irrelevant.

@monsanto
Copy link

monsanto commented Sep 3, 2014

Personally I can't stand the "blank out the page and show a spinner" approach. If the latency is low, the transitions look really flickery, like a novice dev forgot to batch their DOM calls to avoid a repaint or something.

More to the point, it doesn't seem right to me for a routing component to dictate how my UI should look. You're saying if I want my UI to work similar to Gmail, I'm out of luck, and should pick a different router. Fair enough, I guess.

@ryanflorence
Copy link
Member

What would you suggest we do?

@ryanflorence
Copy link
Member

I suppose we could do something like:

<Route waitForAsyncState={true}/>. I don't like the complexity it adds to our code, but if people really want to mimic server-rendered application behavior we could do it.

I find that facebook's new UI that shows some blank elements where data is going fill in to be a better experience than gmail's waiting, especially on lossy connections. I regularly click links several times because I don't know if it actually worked.

@ryanflorence
Copy link
Member

this doesn't seem so bad:

var Handler = React.createClass({
  mixins: [AsyncState],

  statics: {
    willTransitionTo: function(transition, params) {
      this.promise = fetchCourse(params);
      return this.promise;
    },

    getInitialAsyncState: function() {
      return {
        course: this.promise
      };
    }
  }
});

@agundermann
Copy link
Contributor

Wouldn't it be possible to store the old route handler in state and letting the new handler signal when it's done?
Something like

// Parent
componentWillReceiveProps: function(nextProps){
    if(nextProps.activeRouteHandler !== this.props.activeRouteHandler){
        this.setState({
            pendingRouteHandler: this.props.activeRouteHandler
        });
    }
},
handleAsyncStateLoaded: function(){
    this.setState({
        pendingRouteHandler: null
    });
},
render: function(){
    if(this.state.pendingRouteHandler){
        return <Spinner><this.state.pendingRouteHandler /></Spinner>
    }
    return <this.props.activeRouteHandler onAsyncStateLoaded={this.handleAsyncStateLoaded} />;
}


// Child Handlers
hasLoaded: function(){
    return this.state.data; // or whenever the async loading is done
},
componentDidUpdate: function(prevProps, prevState){
    if(this.props.onAsyncStateLoaded && !this.state.hasNotifiedParent && this.hasLoaded()){
        this.props.onAsyncStateLoaded();
        this.setState({
            hasNotifiedParent: true
        });
    }
}

Edit: the above example doesn't work as is because the new active route must be rendered in some way (e.g. inside a hidden div; or by rendering them normally and passing a 'hidden' prop) to load the async state. Moreover, keys need to managed in some way to prevent the recreation of the child handlers which would result in discarding and reloading the whole state. Other than that, the approach seems to work in a small example. If there's any interest, I can post the updated code, but the general idea is the same.

@ryanflorence
Copy link
Member

when we have server side rendering, we will be waiting for all async stuff to happen before rendering, maybe we can easily plug-in an option for client-side transitions to use the same code path.

@monsanto
Copy link

monsanto commented Sep 3, 2014

What would you suggest we do?

I conjecture that there are two fundamental ways of handling transitions, which together cover the full space of this problem:

  1. Switch to the next screen immediately; that screen is responsible for loading data and showing progress information (where progress information can just mean a spinner in the simplest case). Data is stored in state.
  2. The current screen is responsible for loading data and showing progress information; the next screen is constructed with the data as props.

Option 1 is all that react-router supports at the moment: routes take components, and components have embedded within them promises which update the state. Router.transitionTo() switches immediately to the next component.

@rpflorence's workaround has the problem that the current screen isn't (easily) given control of showing progress information; all of the code happens on the (statics of) component for the next screen. It would also be nice to avoid mutation (I don't even know what "this" refers to on statics off the top of my head).

If react-router were to also support Option 2, IMO the cleanest way is for the routes to take promises that return components. Alternatively, you could allow *Promise keys similar to RRouter. Router.transitionTo() would be changed to return a promise that fulfills when we are ready to switch screens.

This opens up some new doors. I can easily make a Link variant that overlays the current screen with a spinner/progress bar, and removes the overlay when the transitionTo promise is removed (like Discourse/Google Analytics). Or if I wanted to be silly I could even put the spinner next to the link that was clicked itself (like Facebook's sidebar).

(As an aside, Option 2-style components also have the nice property that they don't have to keep state in sync with props, a React anti-pattern that forces you to implement componentWillReceiveProps or use a hack like addHandlerKey.)

@ryanflorence
Copy link
Member

I don't even know what "this" refers to on statics off the top of my head

statics, like any other object with methods.

@ryanflorence
Copy link
Member

workaround has the problem that the current screen isn't (easily) given control of showing progress information

Your screens would have to know everything about every other screen, since you can transition from anywhere to anywhere.

You would want access to this data from a different module, and you'd need it to be at the application root UI, it would be a coordination nightmare otherwise.

@ryanflorence
Copy link
Member

I'm currently open to leaving the old screen in place until the next screen's data has all resolved, and then provide a handler to <Routes/> to do whatever you want before and after transitions.

@ryanflorence
Copy link
Member

(As an aside, Option 2-style components also have the nice property that they don't have to keep state in sync with props, a React anti-pattern that forces you to implement componentWillReceiveProps or use a hack like addHandlerKey.)

Yeah, that's going away: #261

@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 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

No branches or pull requests

5 participants