API suggestions for better DX

[rdickert]

First of all, thanks for the awesome work on this package! Having taught a couple of OK GROW! GraphQL workshops at the GraphQL Summit and elsewhere, I've come up with a few suggestions on how to make react-apollo easier to learn and use. Those suggestions are currently embodied by a modified graphql() function in our react-apollo-helpers package. Instead of this (from the Apollo docs):

const CommentPageWithData = graphql(submitComment, {
  props: ({ ownProps, mutate }) => ({
    submit: ({ repoFullName, commentContent }) => mutate({
      variables: { repoFullName, commentContent },
      optimisticResponse: {
        __typename: 'Mutation',
        submitComment: {
          __typename: 'Comment',
          postedBy: ownProps.currentUser,
          createdAt: new Date,
          content: commentContent,
        },
      },
    }),
  }),
})(CommentPage);

We can use this:

const CommentPageWithData = graphql(submitComment, {
  options: {
    optimisticResponse: ({ commentContent }, ownProps) => ({
      __typename: 'Comment',
      postedBy: ownProps.currentUser,
      createdAt: new Date,
      content: commentContent,
    }),
  },
})(CommentPage);

These changes could go into react-apollo itself. The only reason we created our own wrapper was to iterate on ideas rapidly and also leave room to diverge in incompatible ways, but as you'll see there's only one breaking change. I've discussed some of this previously with @stubailo.

Sorry, this may be a bit much to digest at once. I'm happy to make individual issues (and possibly PRs) for any/all of these, but I want to see if there is interest first. The general theme is to use more of the info in the graphql document to reduce repetition, and to replace the props() option with something less complex. Here's a list of the exact changes in react-apollo-helpers:

1. Use the operation name as the default name of the prop (not data or mutate). It's intuitive and helps the dev begin to understand why operation names are relevant. Allow overriding with the name attribute (as now), but I'd anticipate most people won't use it most of the time. The package also will default to the schema query/mutation name (which is always present) if no operation name is specified. This would be a breaking change for anyone who relies on the data or mutation prop names. I believe it is only breaking change suggested. When used with the acdlite's recompose library, things compose really nicely.
2. deprecate use of the props function
3. Set variables to the document arguments as default for mutations so you will generally not have to set them explicitly
4. For mutations, set options at the root level of graphql's 2nd argument, not inside a props function. options for both query and mutation become the apollo-client options object for their respective operations (watchQuery, mutate).
5. For optimisticResponse and variables (if used), allow dev to specify as a function – that will be passed (ownProps, args) – rather than an object, so they can use those values to assemble an object. This replaces having them passed to the props function and feels more intuitive to me.
6. Allow optimisticResponse to omit the root object { __typename: 'Mutation' }, providing only the actual contents of the object we are simulating (plus its __type - would be nice to remove the need for this as well by querying the schema).
7. In updateQueries, destructure the prev arg handed to the function so that the dev can access the result with prev.queryName rather than prev.mutationResult.data.queryName

I'd love comments on this and to see some/all of this work migrate into the core react-apollo graphql() function if people like it.

[jbaxleyiii]

@rdickert Thank you so much for the thoughtful DX improvement ideas!

A few thoughts:

Use the operation name as the default name of the prop (not data or mutate).

I actually was pushing for this during the move to the current API. When we went to make the changes in our app, we ran into it actually making things more confusing. One of the reasons is the documentation around operation names tends to be PascalCase (including the official docs). We also had our queries in different files and shared between a few components so it was harder in practice to utilize the operation names. From a server side debugging / tracing, the operation name tended to be descriptive of the query, not the client side usage. (i.e. GetAccountsFromName vs accounts. This made for awkward props this.props.GetAccountsFromName.accounts. From a learning perspective I totally get it! From usage in our production apps, we never ended up using them so we took it out.

deprecate use of the props function

Why is this? In my opinion, this is one of the best parts of the current API

Set variables to the document arguments as default for mutations so you will generally not have to set them explicitly

Agreed! We used to have this, but I think it got removed during the refactor to using more AC. We should open and track an issue for supporting this (possibly in AC?)

For mutations, set options at the root level of graphql's 2nd argument, not inside a props function.

I'm not sure I follow?

Allow optimisticResponse to omit the root object { typename: 'Mutation' }, providing only the actual contents of the object we are simulating (plus its type - would be nice to remove the need for this as well by querying the schema).

Agreed! I believe this is an AC issue however

In updateQueries, destructure the prev arg handed to the function so that the dev can access the result with prev.queryName rather than prev.mutationResult.data.queryName

Agreed! I believe this is an AC issue however

I'd love to better understand the issues students are having in learning. Would you be able to help me understand where they get tripped up?

[tmeasday]

Hi @rdickert!

Some good ideas here, I'll give my 2c about a few of them:

1. Use the operation name as the default name of the prop

This one is tricky. I think an earlier iteration of RA did do this, but we decided against it for a couple of reasons:

1. It requires reading the query which RA manages to avoid doing otherwise.
2. It means that the output is inconsistent which makes it harder for things built on graphql

Still it might be worth considering if it's indeed true that most people use props or name

2. deprecate use of the props function

I'm not sure this makes sense; isn't it true that people still may want to create completely custom prop shapes that aren't achievable via our other options?

If what you mean is "don't require using props to achieve common use cases", then I can see an argument for it.

3. Set variables to the document arguments as default for mutations so you will generally not have to set them explicitly

Do you mean the callback arguments? I think Object.assign({}, arguments, ownProps) probably makes sense as default variables, yeah.

4. For mutations, set options at the root level of graphql's 2nd argument

Isn't this already possible?

5. For optimisticResponse and variables (if used), allow dev to specify as a function

This seems like a good idea. Maybe let's open a separate issue about this as I think the details could be discussed a bit.

6. Allow optimisticResponse to omit the root object

This seems a bit magic to me. How does Apollo know the type name of the root mutation (Apollo doesn't have the schema right now)? I think maybe this whole feature would need to wait for AC to get the schema (which I think is planned).

Also it would require reading the query again to know the field name. Maybe that's OK though ?

7. In updateQueries, destructure the prev arg handed to the function so that the dev can access the result with prev.queryName rather than prev.mutationResult.data.queryName

I think this is something to discussion on AC, RA should just pass this API through I think.

[rdickert]

Thanks for the thoughtful responses, @jbaxleyiii!

1. Use the operation name as the default name of the prop

That's the most cogent argument I've heard on this. Thanks for clarifying. Pascal case on operation names doesn't click for me, but that's not my call and I'm sure I can adapt. I think for common CRUD mutations, it really would still make sense to have standard names that are always used (e.g., deleteTodo), but I'll think on this some more. Certainly, your example is compelling.

2. deprecate use of the props function

I probably overstated my case here, and I do think it's a very clever way to give access to everything you could want to build your HOC. I also wouldn't want to take that away from people who need it. However – and definitely correct me if this is wrong – I think you have to use props() if you want to use variables (i.e., virtually all mutations) or set updateQueries or optimisticResponse. The typical call to props() will only ask for the creation of a single prop (I see this as a virtue in the API, not a deficit). So creating a function that always returns an object that will only ever have one property feels awkward and error-prone. It was definitely very off-putting to a couple of experienced coders I showed, so it's not just me. From this point of view, these three lines (copied from above) are basically 100% boilerplate:

  props: ({ ownProps, mutate }) => ({
    submit: ({ repoFullName, commentContent }) => mutate({
      variables: { repoFullName, commentContent },

For people just asking for a single prop with a name, default variables, updateQueries, and optimisticResponse (and again, especially beginners, but that's a lot of us!), it'd be really nice to have a way to do this without using props(). I really like the pattern of passing a function the way we do for props(); I'd just prefer to do it for each option separately (default variables, updateQueries, and optimisticResponse, maybe even name (then I could choose to implement operation-name-as-default if I want) as in my example.

If 6 & 7 will be fixed by AC, that makes sense to wait for, rather than patching a fix here.

I'm happy to discuss beginner's issues any time 😊. My general take is the the client side of GraphQL is much harder than the server schema/resolvers (perhaps not surprising). updateQueries is difficult (I haven't tried the reducer option yet – that looks promising), and optimisticUpdates is noisy with its typenames etc. For many, the whole idea of HOCs is quite mind-bending, but you get a really great reaction when they start to get it. The pattern I describe in my post seems to click pretty well for that. Honestly, people in our classes are often learning es6, react, and Apollo all together – things are changing fast out there.

[tmeasday]

@rdickert re the boilerplate:

I think you can just call this.props.mutate({ variables: { repoFullName, commentContent } }) in the child component (if not this is a bug).

However I do agree that it should be possible to set optimisticResponse and updateQueries functions directly, without having to use props.

[rdickert]

howdy @tmeasday!

If what you mean is "don't require using props to achieve common use cases", then I can see an argument for it.

Yup, that's a much better way to put it. 😊

Isn't this already possible? (4)

I don't think it is if you need access to the args, ownProps or result used to call the operation - am I wrong?

For 5, 6, 7, I can open up a separate issue (or perhaps on AC). I'll see what additional comments you guys may have if any, but I'm excited to smooth some rough edges so people will get into this more easily! I agree that it's better if RA passes through AC as much as possible, but can RA pass back args? Maybe it could. It definitely can't pass ownProps. I'm sure that's why the props() solution came to be - it walks a line between giving you the tools you need and not monkeying with the core api.

Our package inspects the document for a couple of values – is there some reason this would be a bad thing?

My general feeling is that we should use the document as the main source of info, and duplicate things from it as little as possible. It feels like there's a lot of repetition and ceremony in client-side Apollo code, so just trying to eliminate that where possible.

[tmeasday]

I don't think it is if you need access to the args, ownProps or result used to call the operation - am I wrong?

Right, so again, I think probably the signature for options should probably be similar to that of optimisticResponse et al. Or maybe we don't need them to be separate functions if options takes all these arguments we need. Let's open a separate issue for this discussion, with concrete ideas.

[tmeasday]

Definitely open 6. + 7. on AC, I don't think you need ownProps for these functions so really it's a AC problem.

[stale[bot]]

This issue has been automatically labled because it has not had recent activity. If you have not received a response from anyone, please mention the repository maintainer (most likely @jbaxleyiii). It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

[stale[bot]]

This issue has been automatically closed because it has not had recent activity after being marked as no recent activyt. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to React Apollo!

[acomito]

I think having a list of the "absolute basic CRUD" situations would be helpful. I also really really like how papaparse displays their API with a logic progression of the common questions/pitfalls they probably get on github:

http://papaparse.com/

It would have saved me a lot of time when getting started if there was a simple to digest "oh you need to update, do this" "oh you need to cut down on latency, here is optimisticResponse", "oh you need your optimisticResponse to do Y? just do this", etc etc. The apollo docs really are amazing, but at least in 1.0 it was kind of overwhelming how many options there were for simple CRUD stuff.