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

Proposal: remove requirement to define variables #251

Closed
gucki opened this issue Dec 6, 2016 · 10 comments
Closed

Proposal: remove requirement to define variables #251

gucki opened this issue Dec 6, 2016 · 10 comments
Labels
👻 Needs Champion RFC Needs a champion to progress (See CONTRIBUTING.md) 🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)

Comments

@gucki
Copy link

gucki commented Dec 6, 2016

To use variables one has to define them at the top the query like this:

    query($count: Int, $name: String) {
        root {
            ... on RootLevel {
                users(first: $count, name: $name) {
                    name
                }
            }
        }
    }

This seems completely redundant to me, as the schema clearly defines which variables exist together with their types. So it should be acceptable to omit the variables competely:

    query {
        root {
            ... on RootLevel {
                users(first: $count, name: $name) {
                    name
                }
            }
        }
    }
@esseswann
Copy link

esseswann commented Dec 6, 2016

+1

@rmosolgo
Copy link

rmosolgo commented Dec 6, 2016

I think the specification of the query's requirements is a feature worth keeping.

Let's say I'm trying to reuse a specific query string in a new context (maybe I'm rewriting a native view). Here's the query string without variable definitions:

query ListAllItems {
  items {
    item {
      ...ItemFields
      ...ItemDetailFields
    }
  }
}

What variables does this query require? Do I have any avenue to discover the requirements other than traversing the fragments? (Tooling could be built for this, does it exist already?)

@gucki
Copy link
Author

gucki commented Dec 6, 2016

Some tooling/ code for it seems to exist in react-relay, because it figures out the variable defintions itself when it builds the query sent to the server.

I'm not totally against keeping the ability to add the variable definitions, but at least it should be optional. Probably like this:

query ListAllItems {
  items {
    item {
      ...ItemFields
      ...ItemDetailFields
    }
  }
}

query ListAllItems($name, $code) {
  items {
    item {
      ...ItemFields
      ...ItemDetailFields
    }
  }
}

query ListAllItems($name: String, $code: Int) {
  items {
    item {
      ...ItemFields
      ...ItemDetailFields
    }
  }
}

@OlegIlyenko
Copy link
Contributor

I would suggest to check out this in-depth discussion around this topic:

#204 (comment)

especially "Always Globals, Implicit" section.

@stubailo
Copy link
Contributor

stubailo commented Dec 6, 2016

Last time I talked with Lee, he said part of the goal was to avoid needing to traverse the whole query and fragments to validate the variables. But it does seem like something that could be optional, especially since it's possible to make dev tools that fill in the declaration for you.

@esseswann
Copy link

esseswann commented Dec 8, 2016

I predict that at one point query composeability and isolation of variables will become so critical that someone will make a library conceptually similar to css-modules which would hash variable names

@leebyron
Copy link
Collaborator

leebyron commented Dec 9, 2016

I think this is worth considering, variable definition is definitely one of the top stumbling points when learning GraphQL.

But it's important to recognise that simply making the variable definitions optional does not simplify things, but instead shifts the complexity into other areas of the GraphQL system. We've strived for a net-most-simple system while designing GraphQL.

For example, defining your variables up front simplifies the validation logic both at static time by knowing if your definition of variables and usage of variables align, as well as reduces execution time by coercing your variable values once at the beginning of query execution rather than requiring a separate full query evaluation to extract variable types.

Another case where the defined variables is really useful is in client-side code generation. For example, graphql query text can be translated into a type-safe query runner that ensures you've provided all the right variables before sending the query over the network. We've built this at Facebook for our native apps, a non type safe variation of this is part of Relay, and I know Apollo's iOS and Android clients are considering the same. Having all variables defined up front makes this code generation a 1:1. Needing to go collect the variables first requires walking the whole query, which for more complex apps could add considerable build time cost.

I think a potential solution to those issues is to just require defining variables when using tools or servers that rely on them, but that also could introduce support skew, which can be problematic for building an ecosystem of tools.

I see the value in making GraphQL easier to use in these cases though, so I certainly wouldn't rule this out, but anyone who would like to champion this proposal and draft an RFC should consider its effect on the GraphQL validator and executor and be aware of implications for tool and server authors.

@leebyron leebyron added the 👻 Needs Champion RFC Needs a champion to progress (See CONTRIBUTING.md) label Dec 9, 2016
@leebyron
Copy link
Collaborator

leebyron commented Dec 9, 2016

Some thoughts if this does become an RFC:

Validation should still assert that if you do define variables, that all variables in use are defined. You definitely shouldn't be able to define some variables and not others.

@gucki
Copy link
Author

gucki commented Dec 12, 2016

I think it's important to separate the concerns into user (writing graphql code) concerns and implementer (implementing the graphql parser/ executor) concerns.

As a user/ developer writing graphql I want it to be easy, understandable and maintainable as possible. Most of the time using proper variable naming is enough to achive this goal. Having to define the type of each variable is (mostly) boilerplate, making it harder to write and read. Here are a short example:

query($countryId: ID!, $userId: ID!, $hasUser: Boolean!) {
  country(id: $countryId) {
    id
    name
    description
    createdAt
  }

  user(id: $userId) @include(if: $hasUser) {
    id
    fullName
  }
}

Of course, as with other languages, it'd be great to have tools which aid the user, ex by displaying the type of the variables, jumping to the type definition upon clicking etc. But this should not be a concern of the spec of the language imo.

I didn't dive into any graphql parser/ executor implemenation, but I'm not sure why removing the variable definitions should make it slower. The executor could coerce a variable once it is used for the first time, mark it used and memoize its coerced value. This is probably a bit more complex to implement, but not slower in the end. If a coercion fails (it should never happen happen in production, right?) at most some queries got executed whose results get discarded.

The second example above also shows a problem of the current up-front coercion (please correct me if there's a better way): if id on user is defined as required in the schema (which makes total sense), you cannot pass an empty user id even if the user query is guarded by an include and never gets executed if the id would be null. So to work around this you have to pass some non-null value for user id (ex. empty string), just to make the type checker happy. With lazy-coercion this shouldn't be a problem.

@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

Rejecting since this did not turn into a proposal and this issue is aging. However a future champion could still reintroduce this as a proposal.

@leebyron leebyron closed this as completed Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👻 Needs Champion RFC Needs a champion to progress (See CONTRIBUTING.md) 🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)
Projects
None yet
Development

No branches or pull requests

6 participants