Improve HOC API

[ksmth]

After discussing a bit with @helfer we agreed that it'd be better to discuss this as a GitHub issue, so here we go. 😄

After working with Apollo for a while and getting to know both apollo-client and react-apollo a bit better, I'm think there's room to improve the API for react-apollo and get it to the same level as apollo-client. I'm not familiar with all the reasoning going into each decision, but I assume that everything is in place for some reason or another and I don't doubt anyones best intentions. Still, there are a few things I'd like to see discussed.

1. Single Argument
apollo-client follows the single argument pattern with most of its API and having this inconsistency between the projects feels a bit off at times. I'm proposing replacing graphql(query, config) with graphql(config) with the possible convenience of graphql(query). In effect we'd add another property named query to the config object.

// before:
graphql(gql``, { ... });

// after:
graphql({ query: gql``, ... });

2. Nested Configs
Another constant source of confusion and switching back and forth between code and documentation is the nesting of config.options. It's hard to memorise which property belongs to which object and I'm arguing it'd be easier to use the HOC API if it was flattened into one config object. There are few things to consider however, as config.options can be a function, while config itself right now does not support this. To retain the same functionality, the whole config would also have to support it, but there are a few properties that need to be taken care of, such as name, withRef, alias, etc. The easy route would be to print a warning / error when a change to these properties is detected and notify that changing these values isn't supported (yet).

// before:
graphql(query, {
  props:  props => ({
    onLoadMore: () => props.data.fetchMore({ ... }),
  }),
  options: props => ({
    variables: { ... }
  })
});

// after:
graphql(query, props => ({
  props: {
    onLoadMore: () => props.data.fetchMore({ ... }),
  },
  variables: { ... }
}));

3. Pass Mutation Data like Query Data
Right now, there's no easy way to get a mutation result displayed in a component, if the mutation result isn't reflected anywhere else. i.e. if a mutation result includes a field like success or error, with the current API you have to find some work around to pass that information along. I think it would be beneficial to pass a mutation result along the same way we pass a query's data, with all fields etc.

[stubailo]

Note that the props and options functions actually take different props! One is the input props and the other is output. So those can't be merged in the way proposed.

[ksmth]

@stubailo Yes, you're right. Here's another proposal to draw a clear line between react-apollo and apollo-client:

graphql(QueryOptions | MutationOptions | (ownProps) => MutationOptions, ReactApolloOptions)

Where QueryOptions and MutationOptions is what gets passed to client.query and client.mutate. In case there are any restrictions (i.e. not changing the query or so), then a warning should be triggered.

[akomm]

@ksmth Regarding Point 3.:

I see a (semi-)problem here. Mutations are different from queries regarding execution. Mutations can be executed parallel multiple times, given they pass result as props, they would overwrite each other in an unpredictable order. That should be considered. Except there is already some "execution-ordering" for mutations there, which executes them one after another.

[wmertens]

I made a similar comment on the slack:

• make things like graphql(query, {options: props => ({variables: {foo: props.bar}})}) more pleasant
• allow marking queries as on-demand, like mutations
• allow changing internal query state (like variables) without having to wrap
• allow reading loading state and last result from mutations, like queries
• allow passing options as the first arg with the query as the query attribute

looks like the first and last item are already requested here, but the rest not.

It also seems to me that there are no action items coming from this issue :(

[tizmagik]

allow changing internal query state (like variables) without having to wrap

+1

[blocka]

One idea that keeps coming up is giving control over to the component as to when to execute the query. Sometimes, it only makes sense to do this based on the internal state of a component, so skip can't be used.

As a practical example, I have a component called Calendar. It maintains it's own state, and upon mount will call a callback with the current month and year. I have a component which is using that calendar, and that component is wrapped with graphql. Once things are going, I can update the variables with the new year and month, but the initial call to graphql, which is out of my control, won't have access to those variables.

In this case I would like to "delay" the query until I have the necessary data from internal state to make it.

[ShockiTV]

@blocka You can always create HoC which would track calendar state and provide it in props also with callback methods you call in calendar component. That way you can compose it on top and use that HoC's generated prop in skip

Regarding other discussion:
"allow marking queries as on-demand, like mutations" - you have withApollo client.query(whateverQuery)
last result of mutation does not make sense, not sure how we would batch similar mutations to get "last" one. Reacts one way flow should be used here - all results could be extracted from normalized cache. If you want sideeffects, use redux-saga or something else to monitor redux store for mutation results (I am now not sure if action names are exported and stable when I remember reducer updates to be being deprecated).

I could clearly see that there will be use-cases for helper which would check if some mutation with variable set is being processed. That 1 I would vote for to be implemented on global level. It could be done 3rd party using redux or some context passing Provider + HoC, and tracking calls, but 1st hand support would be nice.

[wmertens]

@ShockiTV you can also just put your apollo-client instance in Redux and then use it without react-apollo. The point of this issue is that right now, react-apollo is clumsy and we'd love it to be not clumsy, without having to use other tools.

Last result of mutation is handy, and multiple results just have multiple different names. If you have a button to send something to the server, it is nice to just look at a prop to render a "can send/sending/sent successfully/sending error" in the interface, without having to hook onto the mutation promise and manage state.

All things you are proposing are workarounds for react-apollo not a being a good API currently.

[tmeasday]

Some thoughts:

1. I agree it makes sense to be consistent with AC, I think graphql(opts) with shorthand graphql(query) seems nice.

2. @ksmth I don't quite understand how this comment resolves the issue @stubailo mentioned. Apart from the typing problem the real issue is that one function (generating options including variables) runs before the query, and the other (the props mapping) runs after. I can't see how you could combine them.

3. The issue of automatically including mutation results has come up a lot. I can understand why people want it but I think comments like @wmerten's gloss over a lot of inherent issues involved in trying to combine something imperative like a mutation with something declarative like graphql().

Last result of mutation is handy, and multiple results just have multiple different names

What does this mean? The issue @ShockiTV mentioned is what happens if the same mutation runs multiple times. Do we just throw away earlier results? What if you hadn't yet shown them to the user? I think at the least we would need to provide some configuration about how we handle this.

I suspect there is something useful we could do to make mutations easier to handle in the typical case. But I don't think it's a trivial exercise coming up with something intuitive and bulletproof.

[ShockiTV]

Problem with the mutations last result, or even kinda HoC global loading state is that there can be more of them running.
If I am doing my own API with recompose I would prepare provider which would keep hashtable of running mutations something like this:

{
  [addPlayer]: { variables: { name: "Whatever"}},
  [addPlayer]: { variables: { name: "Another"}},
}

with key being som pointer or actual mutation object or some normalized foreign key etc.

And than the container level HoC would export helper function to iterate/filter this, or it will keep list of his own mutations in state and subscribe to their change feeds.
Still user would benefit from this raw subscribed slice.

Example - we query list of players and also create mutation to add players 1 by 1. Than we add 2 different players with optimistic updates and we want them to be disabled till server confirms.
But if we have global mutation isRunning prop, we would have to disable whole list, or add in optimistic response something what we are not requesting...
Better would be if user have some way how to do this.props.mutations.isRunning(addPlayer, { name: "Another" })

That would be nice if react-apollo is doing something like this. It would enable us to have clear api without forcing people to wrap every 1 item list to different mutation container.
If people are forced to wrap every 1 of items separately, than yes simple .processing and maybe also .mutationResult could be possible. I just dont think it is the correct way.

Another gimmick of React Apollo for me was the propType check. graphql-anywhere is nice for components, but direct graphql HOC childs are different. As during loading, there are no result props, but if they are not .isRequired, airbnb-eslint is forcing me to define default props, what is annoying and counterproductive as I have HoC to show loader in case of them not being present || this.props.loading. That is why I spent some time playing around and fixed it. Little helper which I believe can help a lot of people who want to have all according to best practises and out of the box. And lazy DRY :-D But I am not typescript person, so no PR from me possible.
So it would be nice if react-apollo can take over it.
https://medium.com/@ShockiTV/checking-proptypes-for-apollo-client-containers-a673167e84a9

[tmeasday]

@ShockiTV - (re propTypes) that seems cool! Maybe we should take the discussion to a separate issue or even a PR?

[tmeasday]

@ShockiTV -- even something as complex as that doesn't handle all scenarioes--what if the mutation isn't idempotent (same mutation w/ same vars can lead to different results)? It's not impossible that the user would want to call the exact same mutation more than once.

[ShockiTV]

@tmeasday the original propType issue was https://github.com/apollographql/graphql-anywhere/issues/42

And yes, few mutations with same variables could be tricky, but I am not sure I see the possible solution. Even if the actual mutation return promise and some uid which can be used to track actual instance, we would have to somehow save it higher in react tree so component can use it after re-render etc. And pass it to correct child component in list, or some way how child component could retrieve correct instance state.

[wmertens]

On mutation props: how about keeping each invocation Promise in an array, and the last invoked is simply the last one in the array? Add in a timestamp and invocation arguments as a bonus. Make this optional with `asArray` (default true?). So if you have the mutations `addPerson` and `removePerson`, you would get two array props with the invocations. Maybe the props could also have the method `.markDone(index)` to remove invocations you no longer care about. On default props: if that rule is annoying you instead of helping, why not disable it for this case?

…

On Tue, Aug 15, 2017, 9:50 AM Tom Coleman ***@***.***> wrote: @ShockiTV <https://github.com/shockitv> -- even something as complex as that doesn't handle all edge cases--what if the mutation isn't idempotent (same mutation w/ same vars can lead to different results)? It's not impossible that the user would want to call the exact same mutation more than once. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#764 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWluo2NWByikIiF1b16beLpBMsZSQiks5sYT-hgaJpZM4Nzteo> .

[jbaxleyiii]

apollographql/apollo-client#2090 (comment)

[stale]

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!

[OllieJennings]

Is it also possible to get #615 into 2.0? l believe it's quite an important issue

[gforge]

I recently stumbled upon Micheal Jackson's relaunch of the render-prop, https://cdb.reacttraining.com/use-a-render-prop-50de598f11ce. Perhaps some of the inherent complexity in the graphql() object could be remedied with render-props?

[wmertens]

@gforge I really dislike renderProps, but that said, the v2.1 API will indeed allow you to use a component HOC instead of a decorator: https://github.com/apollographql/react-apollo/blob/62d7b31df6b117feaedd704382de14de985470b7/ROADMAP.md#apollo-component (From #1164)

[stale]

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]

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!