React-apollo: Feature request: assistance in not rendering wrapped component till data.loading is true

[jbaxleyiii]

When I make a component like this:

export default graphql(keyValueQuery)(CreateProjectFormLayout)

I have to put this into the render() of CreateProjectFormLayout, in order to avoid issues with the requested data elements not being present yet:

render() {
    return(this.props.data.loading? null : {/* the rest of the form */} )
}

It would be good if the graphql() provided this assistance itself as part of the job it does in wrapping.

(Note: this ticket emerged out of slack, here: https://apollostack.slack.com/archives/react-apollo/p1476619845000682)

From https://github.com/apollostack/apollo-client/issues/793

[tmeasday]

Is this a dupe of https://github.com/apollostack/react-apollo/issues/268?

[GreenAsJade]

Proposal

Preamble

(Terminology: presentational-component means "the component wrapped by a container in the
container-component pattern. As distinct from any old generic react component.)

One reason why this feature request seems like a Good Idea(TM) is that right now using data.loading and data.error in a presentational-component somewhat breaks the container-component pattern.

It's kind of like passing store/state/dispatch into a presentational-component while using redux. We don't do that because it's unwarranted coupling.

In the same way that connect()(PresentationalComponent) makes a container, and uses mapStateToProps and mapDispatchToProps to ensure that the presentational-component is decoupled from redux, graphql()(PresentationalComponent) makes a container, and uses options.props to decouple the react-apollo data layer concerns from the presentational-component (1).

Therefore, it makes sense that the container (data-layer) specifics of "loading" and "errors" be decoupled in the same way. Not only will it provide a DRY means to handle them (the original motivation for this feature request) but it will be a clean way with respect to container-component pattern.

Suggested solution

graphql() options object supports two new keys:

• renderLoading
• renderError

renderLoading = (data) => (<StatelessLoadingDisplay data=data>)
renderError = (data) => (<StatelessErrorDisplay data=data>)

IE each of these keys is a function that is called when the corresponding condition is true (the query is loading or the query had an error) which passes in the data and expects back a component that is rendered in place of the wrapped presentational-component under that condition.

---

(1) Indeed options.props might be better named options.mapProps, since it is a mapping function, not a prop-object! This would feel more natural to someone coming from redux...

[GreenAsJade]

@tmeasday It does appear to be a duplicate, though coming from a slightly different direction.

268 started from the premise of "do this for me automatically" and tacked on the idea of customising, wheras this starts from the premise of "help me handle this by decoupling" and could easily be extended to "handle the automatic case"

(I didn't know about #268 till just now :) )

[tmeasday]

I don't know if I really agree that dealing with what to show while waiting for the data to load isn't the concern of the presentational component. The component doesn't need to know _why_ the data is loading (a GraphQL query is in flight; data is being loaded from localstorage; we are conversing with aliens, who knows?), but's not a technical detail that is irrelevant to the UI.

In my experience if you are following a component "scaffolding" approach (as espoused by tools like react-storybook), you definitely want to have loading stories/scaffolds for your components so you can test out what they look like during this (typically hard to replicate in development) state. In fact I commonly quote this exactly as an example of why the scaffolding approach is so great.

Anyway, I'm not against adding those options as a development shortcut but I wanted to say that.

[GreenAsJade]

Yeah - I agree this could be argued. The fundamental question is whether the presentational component should care or not that there's even a concept of data loading.

I can see an argument that presenting "we haven't got you dinner menu yet" is part of the presentational responsibility of a "dinner menu display component".

My argument is that a presentational component that takes it's data to be presented on props is decoupled from where that data comes from and how and when it gets there. Don't render my presentational component if you haven't got the data :) Deal with those messy concerns some place else.

Fortunately, this is only part of the motivation for this request.

There's the other half of the motivation remaining :) : DRY ... how to avoid every presentational component having basically the same loading and error handling code.

And possibly this brings in the #268 motivation as well: I don't want _any_ code, just don't render me at all when this is happening :)

[tmeasday]

I would just split the presentational component up into two, and then maybe reuse the loading handling bit.

Anyway, this is all a bit academic ;)

[GreenAsJade]

Another consideration supporting not to ask the presentational-component to handle loading and errors this is that it means the presentational-component can't specify "isRequired" on its propTypes.

[jnwng]

@GreenAsJade re: our discussion in slack, here's the wrapper component we're using here to make sure the presentational component doesn't have to deal with the loading state:

const { compose } = require('underscore');
const { graphql } = require('react-apollo');

const waitFor = require('js/lib/waitFor');

const mapProps = (config = {}) => ({ data: { loading, ...restOfData }, ...restOfArgs}) => {
  if (loading) {
    return { loading };
  } else if (config.props) {
    return config.props({ data: restOfData, ...restOfArgs});
  } else {
    return { data: restOfData, ...restOfArgs };
  }
};

const waitForGraphQL = (query, config) => {
  return compose(
    graphql(query, {
      ...config,
      props: mapProps(config),
    }),
    waitFor(({ loading }) => !loading)
  );
};

module.exports = waitForGraphQL;
module.exports.mapProps = mapProps;

where the waitFor module is just a simple HOC that does not render the underlying component until the callback is true. example usage looks like

module.exports = waitForGraphQL(gql`query {}`, {
  props: ({ data: { someData }) => ({ someKey: someData})
});

the underlying component _never_ receives a loading state, which allows us to safely do that data destructuring without having to check if loading is around. YMMV though, as this particular solution does not deal with errors.

[sedubois]

+1, would be great for Apollo to handle Loading/Error renders as pointed out in https://github.com/apollostack/react-apollo/issues/268.

[abhiaiyer91]

@jbaxleyiii @jnwng should react-apollo ship that loading HOC with props for custom loading components?

[jbaxleyiii]

@abhiaiyer91 I don't personally think it should. One of the things that react does so well is controlling the rendering of components. If we start adding in an API to react apollo to pass / render components, I think we loose the power of react in some ways.

React Router v2 => v4 (and their talk about it) really highlight why internal lifecycles amount library apis is against the react paradigm

[abhiaiyer91]

@jbaxleyiii Cool with me then. Lets close this issue!

[colvint]

I have a little util to handle loading and error states like so:

import React from 'react'

import LoadingIndicator from 'components/LoadingIndicator'
import ErrorIndicator from 'components/ErrorIndicator'

export default function withLoading(WrappedComponent) {
  const LoadingComponent = (props) => {
    const { loading, error } = props.data

    if (loading) return (<LoadingIndicator />)
    if (error) return (<ErrorIndicator error={error} />)

    return (<WrappedComponent {...props} />)
  }

  return LoadingComponent
}

[GreenAsJade]

Nice!

[jbaxleyiii]

This would be a great external utility or documented (using things like recompose makes this easy!)

[Gregoirevda]

I have a more extended util, that takes care of static properties and the skip config option:

import React from 'react';
import {graphql} from 'react-apollo';

import LoadingComponent from '../component/LoadingComponent';

export const waitForGraphql = (query, config) => (WrappedComponent) => {
  const GraphQLHandler = (props) => {
    //If skip in config, pass if true
    if(config && config.skip && config.skip(props))
      return <WrappedComponent {...props}/>;

    let propertyName = 'data'; //default property name used by Apollo
    if(config && config.name && (typeof config.name === 'string'))
      propertyName = config.name; //Can be changed by config object

    if(props[propertyName].loading) {
      return <LoadingComponent />
    } else if(props[propertyName].error) {
      console.log(props[propertyName].error);
      return <div>Error</div>
    }
    return <WrappedComponent {...props}/>
  };
  //Wrapped component will loose it's static properties,
  //Transfer them to the returned graphQLHandler function
  Object.keys(WrappedComponent)
    .map(key =>
      GraphQLHandler[key] = WrappedComponent[key]
    );
  return graphql(query, config)(GraphQLHandler);
};

that can be used like:

const ComponentWithData = waitForGraphql(query, config)(Component);
export default ComponentWithData;

@stubailo Are you interested in a pull request with this feature?