Add setVariables to data

[wmertens]

I found setVariables in apollo client and that seems to be super useful for updating a query based on internal state.

However, right now it seems to be missing from data as provided by graphql(). Is there a reason? Can it be included?

[calebmer]

The correct way to change variables in React is to pass in a new set of variables to props. react-apollo doesn’t keep variables in state so the container component can’t actually change its own variables. Only the parent component can change variables by passing in a new set of variables. You could try calling refetch with some new variables, but I’m not sure that will work…

This may be worth reconsidering.

[stubailo]

I'm not sure if it would be good to start introducing a lot of state into the React HoC - perhaps we should suggest an approach where state is kept outside? https://dev-blog.apollodata.com/simplify-your-react-components-with-apollo-and-recompose-8b9e302dea51#.jxb1nyyih

Alternatively we could make state a primary function for stuff like keeping pagination variables etc.

[wmertens]

I don't understand how refetch can work but setVariabnes not?

…

On Wed, Feb 22, 2017, 3:32 AM Sashko Stubailo ***@***.***> wrote: I'm not sure if it would be good to start introducing a lot of state into the React HoC - perhaps we should suggest an approach where state is kept outside? https://dev-blog.apollodata.com/simplify-your-react-components-with-apollo-and-recompose-8b9e302dea51#.jxb1nyyih Alternatively we could make state a primary function for stuff like keeping pagination variables etc. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#472 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlk-l9pUeY-1C9R8t-uCaS3pg0S1Yks5re55TgaJpZM4MH7gn> .

[wmertens]

Also, HoC typically have state, eg Redux' connect.

…

On Wed, Feb 22, 2017, 7:05 AM Wout Mertens ***@***.***> wrote: I don't understand how refetch can work but setVariabnes not? On Wed, Feb 22, 2017, 3:32 AM Sashko Stubailo ***@***.***> wrote: I'm not sure if it would be good to start introducing a lot of state into the React HoC - perhaps we should suggest an approach where state is kept outside? https://dev-blog.apollodata.com/simplify-your-react-components-with-apollo-and-recompose-8b9e302dea51#.jxb1nyyih Alternatively we could make state a primary function for stuff like keeping pagination variables etc. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#472 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlk-l9pUeY-1C9R8t-uCaS3pg0S1Yks5re55TgaJpZM4MH7gn> .

[calebmer]

@wmertens I’m not even sure if refetch works correctly, I’d be surprised if it did. Not surprised if it fetched with a new set of variables, but surprised if those variables persisted after a component re-render.

It isn’t so much about whether or not we should introduce state into an HoC, but more so that we have already made the decision that variables is a function of the component’s props. I’m open to considering how we could change that decision, however.

[wmertens]

@calebmer IMHO variables should be initialized and updated from props, but setVariables and refetch should update the variables. These latter two are super useful for e.g. displaying a list of items from a query and then updating the query with search parameters.

This can be done in a completely backwards compatible manner, presumably at virtually no runtime cost.

Not having setVariables means that a simple graphql(...)(MyComponent) has to be wrapped in a component that does nothing besides provide callbacks to put its state into props, so that the display component can provide the new query variables by calling the callbacks. This is rather hostile to developers that are not used to writing code in that way, and actually takes away an existing feature from apollo-client…

[calebmer]

The problem with that is that when the variables prop changes we are in the awkward position of needing to either blow away the variables set with setVariables, or merge them together 😣

[wmertens]

My first instinct would be to replace. Perhaps there can be a second parameter to the options function with the current state, for if the dev wants custom merging?

…

On Wed, Mar 1, 2017 at 4:42 PM Caleb Meredith ***@***.***> wrote: The problem with that is that when the variables prop changes we are in the awkward position of needing to either blow away the variables set with setVariables, or merge them together 😣 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#472 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlp9a0_39ZZrAXx4cd8Bia9uxPZ7Wks5rhZIDgaJpZM4MH7gn> .

[helfer]

I really think that variables should be given by props and not set internally in the component. If you have a specific use-case where this would be useful, please add a clear description here: apollographql/apollo-client#1391

[amannn]

I just stumbled across this issue. I was actually pleasantly surprised to see that variables can be passed to refetch which will update the query and pass the new variables via this.props.variables to the new component. However, I've seen that these variables are reset at some point which I couldn't really narrow down.

If react-apollo doesn't want to support this, maybe it would make sense to warn/error on refetch(variables) when options.variables is set on the HOC (is that the only case where variables can be reset while the component is mounted?). I really thought this was a bug.

Anyhow, the withState HOC of recompose is really helpful for this use case:

compose(
  withState('sort', 'onUpdateSort', 'created,desc'),
  graphql(query, {options: ({sort}) => ({variables: {sort}})})
)(SomeComponent);

This works fine for me, although it would be nice to not having to wrap another HOC around. Maybe it would make sense to provide state if options is specified as an object, but not if it is set to a function and sets variables. By doing that, we'd never end up in a situation were we have to decide if/how variables should be merged, but can reduce boilerplate for simpler use cases.

[joshuataylor]

Refetch is fantastic until you need to delete a variable that is fetched. You can't just set foo: null, because foo might have a null value.

[wmertens]

Foo:undefined?

…

On Sat, Sep 2, 2017, 7:32 PM Josh Taylor ***@***.***> wrote: Refetch is fantastic until you need to delete a variable that is fetched. You can't just set foo: null, because foo might have a null value. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#472 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlnA9dHGEKqKQ9Jd3hZqBXhFsD4_Oks5seZFAgaJpZM4MH7gn> .

[joshuataylor]

Yeah, ended up doing that, sorry forgot to loop back here and update it. The http://facebook.github.io/graphql/#sec-Null-Value caught me out :).