Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't error if optional field not present #561

Closed
deoqc opened this issue Aug 19, 2016 · 16 comments
Closed

Don't error if optional field not present #561

deoqc opened this issue Aug 19, 2016 · 16 comments

Comments

@deoqc
Copy link

deoqc commented Aug 19, 2016

Sometimes I have a form with not all fields required, and it will be used as inputs to a mutation.

const query = gql`
  mutation myForm(
    $name: String!
    $tags: [String!]
  ) {
    createSomethingAwesome(
      name: $name
      tags: $tags
    ) {
       id
       someField
    }
`

But I actually need to pass an empty tags to variables. If I pass only variables: { name } it will error.

Should apollo-client take care of this?

Right now I'm doing variables: { tags: [], ...formFields } instead of variables: formFields since formFields won't have tags key if it is empty.

@stubailo
Copy link
Contributor

I don't think Apollo Client should take care of this. IMO the correct way to solve this is to pass tags: null as a variable. Is that an acceptable solution? I consider this a limitation of GraphQL itself, rather than Apollo Client, and the workaround doesn't seem so bad.

@stubailo
Copy link
Contributor

Perhaps we could analyze the variables and automatically pass null for ones that the developer doesn't pass, but I think that would actually be worse than the current behavior, which at least warns you when you forget to pass a variable.

Although, perhaps we could use the nullability ! to determine if we should pass null by default.

@deoqc
Copy link
Author

deoqc commented Aug 19, 2016

Yeah, the thing is the field is optional and if you don't pass it is not warn, it is an error.

I can always pass the empty/null args, as I am doing now, but I wondered if wouldn't it be a feature apollo-client would like to have. I have no argument against if you think that wouldn't.

@stubailo
Copy link
Contributor

My only reservation is that it's more code to maintain, so if it's not super valuable it might not be worth it.

@kojuka
Copy link

kojuka commented Aug 19, 2016

Heres how we approached it. Not sure this is correct but it solved that issue for us.

We found that we needed to change our mutation schema on the server to use GraphQLInputObjectType: More info here: http://graphql.org/docs/api-reference-type-system/#graphqlinputobjecttype

Then in your mutation schema you'd define formInput as a GraphQLInputObjectType. Finally client side your mutation would look something like this:

mutation myForm(
    $formInput: formInput!
  ) {
    createSomethingAwesome(
      formInput: $formInput
    ) {
       id
       someField
    }

Hope that works.

@deoqc
Copy link
Author

deoqc commented Aug 19, 2016

@webular it's a good approach, just tested and it works.

But I usually prefer just use GraphQLInputObjectType when some input just make sense when grouped (like geocode={lat, lon}), but it clearly shows that if you really want this feature, you could get it somehow from the GraphQL specs.

I think I can close it now @stubailo .

@deoqc deoqc closed this as completed Aug 19, 2016
@stubailo
Copy link
Contributor

OK, works for me!

@deoqc
Copy link
Author

deoqc commented Aug 20, 2016

I don't want to be annoying but just saw that graphiql doesn't error in my situation above, I think it is a apollo-client trait.

@deoqc deoqc reopened this Aug 20, 2016
@stubailo
Copy link
Contributor

Ok in that case it might be a bug.

@deoqc
Copy link
Author

deoqc commented Sep 21, 2016

Any update about this (possible) bug?

@helfer
Copy link
Contributor

helfer commented Sep 21, 2016

@deoqc This is not a high priority for us at the moment, but if you submit a PR with a failing test case, it would help us fix it faster.

@deoqc
Copy link
Author

deoqc commented Sep 21, 2016

Ok will look into it.

Just asked 'cause it is annoying/error prone to always fill missing optional variables, and have this inconsistent behaviour between apollo and graphiql.

@glasser
Copy link
Member

glasser commented Nov 30, 2016

I've been reading the GraphQL spec and it doesn't really seem clear about whether or not leaving off the value for a variable which is null is allowed or not. However, the NOTE in https://facebook.github.io/graphql/#sec-Null-Value does suggest that it should be allowed:

The same two methods of representing the lack of a value are possible via variables by either providing the a variable value as null and not providing a variable value at all.

@glasser
Copy link
Member

glasser commented Nov 30, 2016

Although maybe this is a duplicate of #852, which refers to graphql/graphql-spec#83 which only added the concept of null-ness to GraphQL after the discussion in this issue!

@helfer
Copy link
Contributor

helfer commented Nov 30, 2016

@glasser: yes, this is something that I will need to add to the 1.0 roadmap. Thanks for the reminder!

@helfer
Copy link
Contributor

helfer commented Dec 5, 2016

Closing this in favor of #852

@helfer helfer closed this as completed Dec 5, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants