Alternative APIs discussed for Query component

[leoasis]

This PR adds a couple alternative APIs as discussed in the contrib meeting to see which API is the one we want to go forward with respect to ease of use for simple use cases, ergonomics, and flexibility to accommodate to advanced use cases as well.

The alternative APIs are built on top of the current one since the one implemented right now is the most flexible one. If we settle with a different API than the base one, we will change the implementation to optimize for that API instead.

The PR modifies an example that exercises the Query component, and now does the same thing with the different APIs, to see how the same code looks with them.

We have 3 different APIs:

• Query: This is the one currently implemented, and has a single render prop (currently children, but there was a discussion about using render instead. Please comment below why you think this should change/stay as is) that receives the data, error, loading and other client methods to handle the entire lifecycle of the request in a single function. This is the most flexible option as you can do whatever you want inside this function, and you can handle any combination of these flags (having data while loading new data, having errors and data, loading and errors, just data, etc).
• QueryWith3Props: This was another API that was suggested. It provides 3 props, renderLoading, renderError and render. If just render is used, it behaves the same as Query with children: it is executed for each case in the lifecycle of the request. If renderLoading is also used, the render function will no longer be called when the loading flag is true, and renderLoading will be called instead. Same thing with renderError.
• QueryWith4Props: This variation provides 4 props: renderLoading, renderError, renderResult and render. This component is meant to be used either by passing the 3 props renderLoading, renderError and renderResult, or by only passing render. In the latter case, it behaves the same as Query. In the former case, renderLoading is called when the loading flag is true, renderError is called when there's an error, and renderResult is called otherwise (that is, when the data is there without any errors and without loading). If you attempt to use any invalid combination, for example, render with renderLoading or with any other prop, an error is shown in the console to warn about this and render is used.

Let me know if I misunderstood any of the API alternatives we discussed, or if there's something missing or wrong, and I'll update the PR. You can also use this to play around with the API.

Notes:

• To make sure the latest react-apollo code is installed in the example app, first run the ./script/prepare-package.sh script and then run yarn in the example app folder.
• @peggyrayzis let me know if what you had in mind for the renderLoading and renderError cases is correct here. I recall we talked about using renderLoading only for the initial load for example, but right now I'm just using data.loading which is not exactly that (fetchMore would cause the loading flag to be true while having data). If this should indeed be just the initial loading (networkStatus equal to 1 I assume?), what should we do about the other states?
• I found a bug (I think) where refetch doesn't trigger an update in the query component after an error has occurred. I will try and track this to see what's the issue. To reproduce you can click the "Cause Error" button, and then hit "refetch", and verify nothing happens (it should show the results again IIUC).

cc @peggyrayzis, @stubailo, @jbaxleyiii, @rosskevin, @excitement-engineer

[Gregoirevda]

Render prop and children as a function have been debated in the new context API proposal of ReactJS: reactjs/rfcs#2

I personally prefer children as a function. I see a benefit to add a loading and error prop, so the Query component knows if they're handled at component level. This way you could pass a default function to handle all errors and loading, but still be able to customise per component.

Will use the default error and loading fn's provided

<Query>
 { ({data}) => /**/}
</Query>

<Query loading={() => <div> Special loading </div>}>
 { ({data}) => /**/}
</Query>

<Query error={() => <div> Special error </div>}>
 { ({data}) => /**/}
</Query>

<Query 
   error={() => <div> Special error </div>} 
   loading={() => <div> Special Loading</div>}
>
 { ({data}) => /**/}
</Query>

With a children prop, this is probably a QueryWith2Props solution

[rosskevin]

I am in agreement and I like @Gregoirevda's suggestion:

• children as function for data
• briefer named error and loading props to provide for component controlled rendering are a good suggestion.

[kandros]

IMHO the component should pass loading to the render function and the implementation should be on him

Personally I don't use Query directly,I export it as I would have done with HOCs this is what I'm doing so far.

Personally i prefer to use render prop instead of children but it pretty much the same

class AddToSelection extends Query<addToSelectionMutation> {}

type ApolloProps = QueryResult<addToSelectionMutation>;
type CustomProps = {};

type Props = {
  variables: addToSelectionMutationVariables,
  render: (x: ApolloProps & CustomProps) => React.ReactNode
};

export const MutationAddToActiveSelection: React.SFC<Props> = ({
  variables,
  render
}) => {
  return (
    <AddToSelection variables={variables} query={MUTATION}>
      {({ loading, data, error, ...apolloProps }) => {

        return render({
          loading,
          data,
          error,

          ...apolloProps
        });
      }}
    </AddToSelection>
  );
};


export const withAddToSelection = Component =>
  ({variables, ...props}) =>
    <AddToSelection 
    variables={variables}>
    render={{(data) =>
        <Component {...props} addToSelection={data}/>
    }}
    />

[leoasis]

More feedback found in #1565

[jbaxleyiii]

Okay I've been thinking on this for quite a while now, and I've come to the conclusion that the multiple named render props are going to be troublesome. The original goal of them was to lower the learning curve for new users and to reduce some level of repetition found when checking loading / error / data status. After trying out a few attempts and seeing the work and discussions here, I think we should ditch the loading, error props and go with either render or function as child that gets the following:

{
  loading: boolean,
  networkError: Error | null,
  errors: GraphQLErrors[] | null,
  data: GraphQLResult | null
}

Function as child vs render

This is a pretty lively debate in the react community currently. While there is no clear (that I can see) consensus, the two points as I've seen most widely articulated are that

1. render is easier to teach and read
2. children is special / already how people think to render subtrees.

Personally, I'm a bigger fan of render as a prop. FAC (Function as children) is proplematic to teach, potentially harder to type (the type signature for the children prop is pretty consistent across all built-in React components - Component | SFC | string | etc), and can lead to pitfalls if used in a prop + children method.

I know this feels like a lot of bikeshedding over a simple portion of the API, but most Apollo users are React users currently. This change will be mostly serving first time users since the only noticiable win over the HOC is readability (arguable) and dynamic operations (rarely used). If react-router, formik, etc are all teaching render as a prop, I would hate to have to teach how to use children instead. The potential for <Query children={}>{() => foo}</Query> makes me nervous as well because I can see it happening based on a lot of tutorials / tweets I've already seen around the "just component" movement.

Arguments to the function

The proposed shape (above) is a break from the current HOC offerings but with good reason. I'm actively trying to improve error handling in Apollo Client and bring distiction between network (link) errors and data (graphql) errors. The new error policy is one such iniative. In splitting these errors out, React Apollo will be ahead of the next work of AC and not have to make a breaking change if/when that becomes the only way to access errors. The single error prop was designed around the merged ApolloError.

I'm also interested in possibly exposing client as the second argument as an escape hatch + merging of the withApollo and graphql HOCs. This is super helpful for manual cache interactions which may become more and more common with new features.

API preview

<Query
  query={gql``}
  variables={}
  render={({ loading, networkError, data }) => // }
/>

@leoasis @Gregoirevda @rosskevin @excitement-engineer

[jbaxleyiii]

@kandros I'm confused by your example? Why not use the HOC?

[rosskevin]

@jbaxleyiii react-final-form has taken the approach of allowing the user to pick their approach for rendering prop/component/children:
https://github.com/final-form/react-final-form/blob/master/src/index.d.ts#L58

I'm not sure it is good or bad, but I'm firmly in the function as children. I feel strongly a render prop is an ambiguous/rendundant representation of the purpose represented by children. Regardless of that discussion, it is worth noting that react-final-form is extremely thoughtful in design (successor to redux-form) and allows all three options through their utility renderComponent

[kandros]

@jbaxleyiii if you refer to my withAddToSelection is there for backward compatibility and strictly related to my project.
The main reason I use Query is that is much much much easier to reason about and typecheck with TypeScript

[jamesplease]

👍 to dropping the error and loading props. One of the benefits to keeping it as a single prop allows for composition of the components, like so:

<Composer
  operations={[
    <Query
      skip={boolean}
      options={QueryOptions}
    />,
    <Mutation
      options={MutationOptions}
    />
  ]}
  render={([queryResult, mutationResult]) => {
    return (
      <div>
        stuff
      </div>
    );
  }}
/>

(There are more details on this in #1565 )

—-

If react-router, formik, etc are all teaching render as a prop, I would hate to have to teach how to use children instead.

Consistency is definitely a good thing. My prediction is that these libraries will one day change their APIs to be consistent with whatever the new context API ends up looking like. You may have seen it in the context RFC PR, but they’re also considering avoiding the JSX/Component altogether. Something like:

context(‘blah’).read(val => <MyComponent val={val} />)

I think that this option is worth consideration, too, as it avoids the whole “which prop is it” problem. It may seem too similar to the HoC, though 🙂

I’m not sure what this project’s approach to breaking changes is, but if 2.1 is released before the new context API is finalized, then it could be worth considering if you would make a breaking change to bring it in alignment with the new context.

[leoasis]

I agree on having a single prop to handle all cases. I think that's the path reason-apollo decided to go as well, and also relay. Also, the other use cases can be built quite easily on top of this.

@jbaxleyiii maybe the properties of the object you specified to receive in the render prop was not meant to be exhaustive, but we'd need to also have access to the other functions that the client provides, right? Things like fetchMore, refetch, start/stop polling, etc. Or is that what the second argument is meant for? Also, I'd imagine networkStatus to be part of that list of properties.

Regarding which one to use, render or children, I don't have strong opinions, but since I'm slightly in favor of children, I'd like to add a reason so maybe it will help with the final decision (though if we decide to go with render that's fine for me).

When using children, you write it as the content of the tag, which forces you to close it below when the function ends. Since this particular function could take a couple of lines (it's mostly not a one-liner), using children (vs render) would help improve the readability by having a clearer boundary of the component where the render prop belongs.

So in that sense, I think children is easier to read thanrender. render may still be easier to teach since it's just a prop without any syntax sugar, though that's something that is also commonly taught and widely used as well.

In the end, we can use whatever, reason-apollo uses function as children while relay uses the render prop, and we can find as many examples for the former as for the latter.

Whatever we decide, I'm definitely against having it both ways. That's even more confusing, and it is harder to type with any type checker (you need to allow one or the other, but not both and definitely not nothing).

What @jmeas is nice as well, if the community will follow what the React team will do with the Context API, that will end all discussions (maybe?), but for now since that's not seen in the wild, it will have to be taught, and may be even more confusing to users.

[excitement-engineer]

@jbaxleyiii I have also been thinking about the various render props and their use case and I am also in favor of removing them and simplifying the component API:)

children vs render prop

I don’t personally have any preference for the attribute used for rendering. I am in favor of simply picking one and moving on:) Relay’s query renderer also only supports a the ‘render’ attribute and it works fine! Whichever one we choose, it is really trivial to wrap the Query component to use children instead of render or vice versa in your own codebase.

making a distinction between errors

If I understand correctly the network errors relate to something like the internet being down whilst the ‘errors’ can be used if the actual request came through but the graphql response contains an error.

Having two error props adds some complexity to the API, is this desirable from a user standpoint? What use cases does this distinction in the errors open up?

For my personal use case I want to know whether there is any error and if so then show the approriate UI (which I can imagine is the most common use case). From first glance it seems that making a distinction in errors would complicate this.

[jaydenseric]

@jbaxleyiii

I'm also interested in possibly exposing client as the second argument as an escape hatch + merging of the withApollo and graphql HOCs.

This would be great.

[jbaxleyiii]

Thanks for all the counterpoint! I'm sold, children it is!

[jamesplease]

This question may be out of scope for this PR (my apologies if so), but are there plans to add in a component version of compose? I do not see this in the roadmap, and I noticed here that query composition is used as one use case of that method.

I've created a component version of compose here that could be used as a reference, or for testing out the new Query, etc. components.

[rosskevin]

@jmeas I do not think we would build/export anything like that from this library - it is out of scope. It is my intent to remove the existing compose a.k.a. flowright from here on the next breaking release for just that reason.

[jbaxleyiii]

@leoasis since we are good with the current API on master, I'm going to close this one out. Thank you for your thoughtful explorations in this space and helping us come to a well thought out consensus!

[jamesplease]

I know that this issue has been closed, but it seems likely that the new Context API will go with JSX + children, so the API discussed here should be consistent with that ✌️ Just thought I'd pop in here to share that.

[mengqing]

@rosskevin I understand the idea behind removing compose from the current API, but what's your recommendation on how to structure nested query components as @jamesplease mentioned? When you have more than two queries, the nested components become really messy.

Also, how do you handle batch queries? We use compose a lot of batching multiple queries together

[aboglioli]

@mengqing You can use this to compose components.